Bug 174787 – Icon loader error on startup

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Michael Catanzaro]

There is some recent regression in the icon loader causing this error to print every time Epiphany is started (I'm testing with 'epiphany -p'):

ERROR: Failed to start load for icon at url /favicon.ico
../../Source/WebCore/loader/icon/IconLoader.cpp(82) : void WebCore::IconLoader::startLoading()

Of course /favicon.ico is not a very good place to try loading an icon from.

[Carlos Garcia Campos]

This is a regression of the new icon loading, it happens with pages that shouldn't have a favicon, like about pages, that's why it happens when starting ephy (about:overview is loaded). IconController::startLoader() did more checks before starting the load that DocumentLoader::startIconLoading() is not doing. It checks that the frame is the main one, the document can have an icon (document url is not empty and not about blank) and that favicon url is in HTTP family. We should do the same checks now before starting to load icons.

[Carlos Garcia Campos]

This is not specific to GTK at all.

[Carlos Garcia Campos]

Created attachment 316361 [details]
Patch

[Build Bot]

Attachment 316361 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/DocumentLoader.cpp:1624:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Comment on attachment 316361 [details]
Patch

Please do not land this until I take a closer look.

[Brady Eidson]

Comment on attachment 316361 [details]
Patch

Actually no, I can r- right now - Restricting to HTTP is wrong. This will break API tests in IconLoadingDelegate.mm

[Brady Eidson]

Comment on attachment 316361 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316361&action=review

> Source/WebCore/loader/DocumentLoader.cpp:1617
> +    if (!m_frame->isMainFrame())
> +        return;

This check is good.

> Source/WebCore/loader/DocumentLoader.cpp:1620
> +    if (document->url().isEmpty() || document->url().isBlankURL())
> +        return;

This check is good.

> Source/WebCore/loader/DocumentLoader.cpp:1627
> +        if (iconURL.protocolIsInHTTPFamily())

This will break an API test.

I know this check was in IconController before, but the new client interface is more permissive. If you don't want a non-http default favicon please make the check in your client (e.g. your API-layer icon database)

[Carlos Garcia Campos]

(In reply to Brady Eidson from comment #7)
> Comment on attachment 316361 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316361&action=review
> 
> > Source/WebCore/loader/DocumentLoader.cpp:1617
> > +    if (!m_frame->isMainFrame())
> > +        return;
> 
> This check is good.
> 
> > Source/WebCore/loader/DocumentLoader.cpp:1620
> > +    if (document->url().isEmpty() || document->url().isBlankURL())
> > +        return;
> 
> This check is good.
> 
> > Source/WebCore/loader/DocumentLoader.cpp:1627
> > +        if (iconURL.protocolIsInHTTPFamily())
> 
> This will break an API test.
> 
> I know this check was in IconController before, but the new client interface
> is more permissive. If you don't want a non-http default favicon please make
> the check in your client (e.g. your API-layer icon database)

Ok, but note that LinkIconCollector also checks the url is HTTP, so this is inconsistent.

[Carlos Garcia Campos]

Created attachment 316375 [details]
Patch

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Build Bot]

Attachment 316375 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/DocumentLoader.cpp:1624:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

(In reply to Carlos Garcia Campos from comment #8)
> (In reply to Brady Eidson from comment #7)
> > Comment on attachment 316361 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=316361&action=review
> > 
> > > Source/WebCore/loader/DocumentLoader.cpp:1617
> > > +    if (!m_frame->isMainFrame())
> > > +        return;
> > 
> > This check is good.
> > 
> > > Source/WebCore/loader/DocumentLoader.cpp:1620
> > > +    if (document->url().isEmpty() || document->url().isBlankURL())
> > > +        return;
> > 
> > This check is good.
> > 
> > > Source/WebCore/loader/DocumentLoader.cpp:1627
> > > +        if (iconURL.protocolIsInHTTPFamily())
> > 
> > This will break an API test.
> > 
> > I know this check was in IconController before, but the new client interface
> > is more permissive. If you don't want a non-http default favicon please make
> > the check in your client (e.g. your API-layer icon database)
> 
> Ok, but note that LinkIconCollector also checks the url is HTTP, so this is
> inconsistent.

Bummer - No API test coverage for that yet.

That will likely be changing soon, then.

[Brady Eidson]

Comment on attachment 316375 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316375&action=review

> Source/WebKit/UIProcess/API/glib/WebKitIconLoadingClient.cpp:39
> +        // WebCore can send non HTTP icons only when it doesn't find icons in document and
> +        // falls back to document-url/favicon.ico. We don't want non HTTP icons in that case either.

This comment will be outdated soon and I likely won't know to update it.

You might just want to mention non-HTTP icons, full stop, and not reference the default favicon case that exists today.

[Carlos Garcia Campos]

Committed r219905: <http://trac.webkit.org/changeset/219905>

[Radar WebKit Bug Importer]

<rdar://problem/35624411>