Bug 172733

Summary: CSP: Apply img-src directive to favicon loads
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, beidson, bfulgham, buildbot, cdumez, darin, japhet, mkwst, rniwa, sam
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch and layout tests
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Patch and layout tests beidson: review+

Description Daniel Bates 2017-05-30 15:01:42 PDT
For consistency we should apply the CSP img-src directive to favicon loads.
Comment 1 Daniel Bates 2017-05-30 15:02:04 PDT
Part of <rdar://problem/32082654>
Comment 2 Daniel Bates 2017-05-30 15:31:38 PDT
Created attachment 311547 [details]
Patch and layout tests
Comment 3 Build Bot 2017-05-30 16:33:07 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2017-05-30 16:33:08 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-05-30 16:53:45 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-05-30 16:53:46 PDT Comment hidden (obsolete)
Comment 7 Daniel Bates 2017-05-31 09:45:20 PDT
Created attachment 311600 [details]
Patch and layout tests
Comment 8 Darin Adler 2017-06-06 14:45:22 PDT
Comment on attachment 311600 [details]
Patch and layout tests

Why do we even have favicon support in WebKit any more? What client is using it? I am pretty sure Safari does not want to use this any more. WebKIt should just parse the filename; icon loading should be done outside WebKit.
Comment 9 Brady Eidson 2017-06-06 16:19:21 PDT
(In reply to Darin Adler from comment #8)
> Comment on attachment 311600 [details]
> Patch and layout tests
> 
> Why do we even have favicon support in WebKit any more? What client is using
> it? I am pretty sure Safari does not want to use this any more. WebKIt
> should just parse the filename; icon loading should be done outside WebKit.

This is not right. 

Moving to a system where the client app gathers URLs on its own and loads them in its own networking context(s), many of the loads would fail or return incorrect results.

While there is tons of legacy "IconDatabase" code we can remove from WebKit soon related to disk storage and load decision making, WebKit still needs to do the actual loading of the icons itself, as directed by the client app.

Safari *is* using this new client interface.
Comment 10 Brady Eidson 2017-06-06 16:24:11 PDT
Comment on attachment 311600 [details]
Patch and layout tests

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

> Source/WebKit2/ChangeLog:9
> +        Return 0 seconds for the maximum buffering time for favicons (no change in behavior).

This was going to be my comment - Looking at the source changes, it appears there's not actually a behavior change here.

What part(s) of the source changes are actually relevant here?
Comment 11 Brady Eidson 2017-06-06 16:24:56 PDT
Comment on attachment 311600 [details]
Patch and layout tests

Removing the comment seems to be the only relevant part of this? Or I'm missing something.
Comment 12 Brady Eidson 2017-06-06 16:35:51 PDT
Comment on attachment 311600 [details]
Patch and layout tests

Dan helped me realize where the behavior change was. Looks fine.
Comment 13 Daniel Bates 2017-06-09 13:45:48 PDT
Committed r218015: <http://trac.webkit.org/changeset/218015>
Comment 14 Daniel Bates 2017-06-09 16:14:54 PDT
(In reply to Daniel Bates from comment #13)
> Committed r218015: <http://trac.webkit.org/changeset/218015>

Removed CONSOLE MESSAGE line I inadvertently left in the expected result for test block-favicon.html from an earlier iteration of the test that did not call testRunner.queueReload() and committed this in <http://trac.webkit.org/changeset/218026>.
Comment 15 Sam Weinig 2017-06-09 16:44:00 PDT
We have traditionally shied away from using the term favicon, and generally just used icon (or site icon sometimes I think). I don't see a compelling reason to start using this term now.

Also, does this new type represent all the different types of icons that may be in this class:
- traditional known location site icons (favicon.ico)
- <link rel="icon"> icons
- <link rel="mask-icon"> icons
- <link rel="apple-touch-icon"> icons

(Am I forgetting any?)
Comment 16 Brady Eidson 2017-06-12 16:17:25 PDT
(In reply to Sam Weinig from comment #15)

> Also, does this new type represent all the different types of icons that may
> be in this class:...?

Yes, IconLoader is used for all of these.
Comment 17 Sam Weinig 2017-06-13 15:52:55 PDT
(In reply to Brady Eidson from comment #16)
> (In reply to Sam Weinig from comment #15)
> 
> > Also, does this new type represent all the different types of icons that may
> > be in this class:...?
> 
> Yes, IconLoader is used for all of these.

Cool cool. Can we not use the name 'favicon' then?
Comment 18 Daniel Bates 2017-06-14 21:06:20 PDT
(In reply to Sam Weinig from comment #17)
> (In reply to Brady Eidson from comment #16)
> > (In reply to Sam Weinig from comment #15)
> > 
> > > Also, does this new type represent all the different types of icons that may
> > > be in this class:...?
> > 
> > Yes, IconLoader is used for all of these.
> 
> Cool cool. Can we not use the name 'favicon' then?

Filed bug #173400 to use "icon" instead of "favicon".