Bug 174787 - Icon loader error on startup
Summary: Icon loader error on startup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-24 08:59 PDT by Michael Catanzaro
Modified: 2017-11-17 14:38 PST (History)
10 users (show)

See Also:


Attachments
Patch (2.55 KB, patch)
2017-07-25 02:32 PDT, Carlos Garcia Campos
beidson: review-
Details | Formatted Diff | Diff
Patch (4.12 KB, patch)
2017-07-25 10:33 PDT, Carlos Garcia Campos
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-07-24 08:59:28 PDT
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.
Comment 1 Carlos Garcia Campos 2017-07-25 02:27:33 PDT
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.
Comment 2 Carlos Garcia Campos 2017-07-25 02:28:44 PDT
This is not specific to GTK at all.
Comment 3 Carlos Garcia Campos 2017-07-25 02:32:52 PDT
Created attachment 316361 [details]
Patch
Comment 4 Build Bot 2017-07-25 02:35:48 PDT
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.
Comment 5 Brady Eidson 2017-07-25 10:00:02 PDT
Comment on attachment 316361 [details]
Patch

Please do not land this until I take a closer look.
Comment 6 Brady Eidson 2017-07-25 10:02:43 PDT
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
Comment 7 Brady Eidson 2017-07-25 10:10:06 PDT
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)
Comment 8 Carlos Garcia Campos 2017-07-25 10:21:37 PDT
(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.
Comment 9 Carlos Garcia Campos 2017-07-25 10:33:43 PDT
Created attachment 316375 [details]
Patch
Comment 10 Build Bot 2017-07-25 10:36:46 PDT
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
Comment 11 Build Bot 2017-07-25 10:37:07 PDT
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.
Comment 12 Brady Eidson 2017-07-25 10:47:58 PDT
(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.
Comment 13 Brady Eidson 2017-07-25 10:49:08 PDT
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.
Comment 14 Carlos Garcia Campos 2017-07-25 23:19:56 PDT
Committed r219905: <http://trac.webkit.org/changeset/219905>
Comment 15 Radar WebKit Bug Importer 2017-11-17 14:38:43 PST
<rdar://problem/35624411>