1478823 - Load /favicon.ico as early as possible

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

[Dave Townsend [:mossop]]

The favicon selection code has a problem in that it only considers new or changed favicons in the set to load since it was last run and doesn't necessarily consider them in document order. The patch here changes it to always consider every icon link in the document in document order.

Another issue is that we wait until the page finishes loading before even starting to load /favicon.ico if no other icon is found. This patch starts loading /favicon.ico as early as possible and cancels it if another icon is found.

Comment 1

[Dave Townsend [:mossop]]

Attachment: Bug 1478823: Consider all favicons when selecting which to load and load /favicon.ico as early as possible.

The favicon selection code has a problem in that it only considers new or changed
favicons in the set to load since it was last run and doesn't necessarily consider
them in document order. This changeset changes it to always consider every icon
link in the document in document order.

It also starts loading /favicon.ico as early as possible and cancels it if
another icon is found.

Comment 2

[Marco Bonardo [:mak]]

Could you please elaborate on how the new version addresses this problem I commented about on the first version:
Off-hand this risks to break pages putting badges on favicons. Those pages tend to add new link rel="icon" at a later time after load to notify that some entries are new/unread. We should always prefer these later additions as best icon.

The current code only considers the newly added icons in subsequent runs to better support that case, while it looks like the patch, for each link additon, considers again all of the existing icons.
So, it sounds like it could pick a non-up-to-date icon, just because it has a better format/resolution.

Comment 3

[Dave Townsend [:mossop]]

(In reply to Marco Bonardo [::mak] from comment #2)
> Could you please elaborate on how the new version addresses this problem I
> commented about on the first version:
> Off-hand this risks to break pages putting badges on favicons. Those pages
> tend to add new link rel="icon" at a later time after load to notify that
> some entries are new/unread. We should always prefer these later additions
> as best icon.
> 
> The current code only considers the newly added icons in subsequent runs to
> better support that case, while it looks like the patch, for each link
> additon, considers again all of the existing icons.
> So, it sounds like it could pick a non-up-to-date icon, just because it has
> a better format/resolution.

Sorry I missed that comment. (In reply to Marco Bonardo [::mak] from comment #2)
> Could you please elaborate on how the new version addresses this problem I
> commented about on the first version:
> Off-hand this risks to break pages putting badges on favicons. Those pages
> tend to add new link rel="icon" at a later time after load to notify that
> some entries are new/unread. We should always prefer these later additions
> as best icon.
> 
> The current code only considers the newly added icons in subsequent runs to
> better support that case, while it looks like the patch, for each link
> additon, considers again all of the existing icons.
> So, it sounds like it could pick a non-up-to-date icon, just because it has
> a better format/resolution.

Sorry, I missed your question before.

So here's the problem. The current implementation will do different things based on the timing of events. So if a webapp sets a badged icon within 500ms of us parsing the rest of the icons we'll do something different to if it sets it 500ms after. That doesn't seem right to me but maybe that is the way it has to be to maintain compatibility with the web?

We should see this be a problem currently. If a page loads then later sets a badged icon we use the badged icon. If you then navigate to a new URL and then go back we restore the page from the bfcache which causes us to send out DOMLinkAdded events for every link in the document which means we might not use the badged icon this time around.

I guess the question is do we want the icon we display to be entirely based on the DOM content or do we need to use timing to make badged icons work?

Comment 4

[Marco Bonardo [:mak]]

(In reply to Dave Townsend [:mossop] from comment #3)
> So here's the problem. The current implementation will do different things
> based on the timing of events. So if a webapp sets a badged icon within
> 500ms of us parsing the rest of the icons we'll do something different to if
> it sets it 500ms after.

Right, though it sounds like a real edge case, it's likely being the js code setting the badged icon, if we didn't even finish parsing it's less likely to do it (not impossible, just improbable).

> We should see this be a problem currently. If a page loads then later sets a
> badged icon we use the badged icon. If you then navigate to a new URL and
> then go back we restore the page from the bfcache which causes us to send
> out DOMLinkAdded events for every link in the document which means we might
> not use the badged icon this time around.

I think that's ok, provided the page once restored will set the icon again.

> I guess the question is do we want the icon we display to be entirely based
> on the DOM content or do we need to use timing to make badged icons work?

I'm not sure how we could base on dom content unless we completely change (again) the way we pick icons.

Again, this problem imo should be split into showing and storing.
We want to always show a badged icon, but ideally we never want to store it.
Currently that's not the case. Though, it may also be non-trivial to recognize such icons, since there's no recognized standard. We could at maximum say "it's an icon that is not in the initial set of <link>s provided by the page".

Comment 5

[Dave Townsend [:mossop]]

Attachment: Bug 1478823: Add a chrome DOM event signalling that the head element's children have been parsed.

Comment 6

[Dave Townsend [:mossop]]

Attachment: Bug 1478823: Start loading the default favicon as soon as the head element is complete.

Comment 7

[Dave Townsend [:mossop]]

Ok based on the discussion I've pared this down to just loading /favicon.ico earlier to help alleviate bug 1473514

Comment 8

[Phabricator Automation]

Comment on attachment 8996783 [details]
Bug 1478823: Start loading the default favicon as soon as the head element is complete.

Marco Bonardo [::mak] (Away 9-26 Aug) has approved the revision.

https://phabricator.services.mozilla.com/D2613

Comment 9

[Phabricator Automation]

Comment on attachment 8996782 [details]
Bug 1478823: Add a chrome DOM event signalling that the head element's children have been parsed.

Boris Zbarsky [:bz] (no decent commit message means r-) has approved the revision.

https://phabricator.services.mozilla.com/D2612

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5386a64522c
Add a chrome DOM event signalling that the head element's children have been parsed. r=bzbarsky

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/741a654c2f4c
Start loading the default favicon as soon as the head element is complete. r=mak

Comment 12

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 741a654c2f4c (bug 1478823) for mochithest failures at toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html

Backout: https://hg.mozilla.org/integration/autoland/rev/25c8ddd2fe538d8765e260af4eb8a9494e22c809

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=741a654c2f4c183a14c717f3367828f5003078e5

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=191963871&repo=autoland&lineNumber=4565

15:15:11     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html | correct ip for http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_image_bad.png - Expected: 127.0.0.1, Actual: 127.0.0.1 
15:15:11     INFO - Buffered messages finished
15:15:11     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html | Test timed out. 
15:15:11     INFO -     reportError@SimpleTest/TestRunner.js:121:7
15:15:11     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
15:15:11     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
15:15:11     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
15:15:11     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
15:15:11     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
15:15:11     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
15:15:11     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
15:15:11     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
15:15:11     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
15:15:11     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
15:15:11     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
15:15:11     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
15:15:11     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
15:15:11     INFO -     TestRunner.runTests/<@SimpleTest/TestRunner.js:381:9
15:15:11     INFO -     promise callback*TestRunner.runTests@SimpleTest/TestRunner.js:368:5
15:15:11     INFO -     RunSet.runtests@SimpleTest/setup.js:194:3
15:15:11     INFO -     RunSet.runall@SimpleTest/setup.js:173:5
15:15:11     INFO -     hookupTests@SimpleTest/setup.js:266:5
15:15:11     INFO - parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5
15:15:11     INFO - getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11
15:15:11     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
15:15:11     INFO -     hookup@SimpleTest/setup.js:246:5
15:15:11     INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=%2Fvar%2Ffolders%2F_t%2F053z6dhx5rg5_6bvcdf_slt400000w%2FT&cleanupCrashes=true:11:1
15:15:12     INFO - Not taking screenshot here: see the one that was previously logged
15:15:12     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html | Extension left running at test shutdown 
15:15:12     INFO -     ExtensionTestUtils.loadExtension/<@SimpleTest/ExtensionTestUtils.js:109:7
15:15:12     INFO -     executeCleanupFunction@SimpleTest/SimpleTest.js:1227:19
15:15:12     INFO -     SimpleTest.finish@SimpleTest/SimpleTest.js:1240:5
15:15:12     INFO -     killTest@SimpleTest/TestRunner.js:130:7
15:15:12     INFO -     delayedKillTest@SimpleTest/TestRunner.js:157:47
15:15:12     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:157:7

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/c5386a64522c

Comment 14

[Dave Townsend [:mossop]]

Attachment: Bug 1478823: Start loading the default favicon as soon as the head element is complete.

Comment 15

[Phabricator Automation]

Comment on attachment 8998034 [details]
Bug 1478823: Start loading the default favicon as soon as the head element is complete.

Marco Bonardo [::mak] (Away 9-26 Aug) has approved the revision.

https://phabricator.services.mozilla.com/D2803

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4073ae1a277f
Start loading the default favicon as soon as the head element is complete. r=mak

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4073ae1a277f

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Shouldn't you check that the head element actually is in document? By creating dummy html element and then using innerHTML to add head there, as an example, one gets HTMLSharedElement::DoneAddingChildren called without the element being in document.

Comment 19

[Dave Townsend [:mossop]]

(In reply to Olli Pettay [:smaug] from comment #18)
> Shouldn't you check that the head element actually is in document? By
> creating dummy html element and then using innerHTML to add head there, as
> an example, one gets HTMLSharedElement::DoneAddingChildren called without
> the element being in document.

Perhaps for correctness. I'm struggling to think of what problem not doing this would cause. I think the worst case is that it would cause us to start loading the root favicon before we encounter the real icons in the document which would then override it and be loaded normally. This is what we used to do before we moved loading favicons to the content process. Anyway I've filed bug 1483222 to clean it up.

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Is there developer documentation for this change? Is this undoing the page load performance optimization enabled at https://bugzilla.mozilla.org/show_bug.cgi?id=1247843, which prioritized actually loading the page over loading the (mostly useless favicon)? If I want the page to be loaded first, and the favicon "whenever", how do I tell Firefox to do this?

Comment 21

[Marco Bonardo [:mak]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #20)

Is there developer documentation for this change? Is this undoing the page load performance optimization enabled at https://bugzilla.mozilla.org/show_bug.cgi?id=1247843, which prioritized actually loading the page over loading the (mostly useless favicon)? If I want the page to be loaded first, and the favicon "whenever", how do I tell Firefox to do this?

I don't think this really undoes that, Bug 1247843 acted at the network fetch level (using tailing to queue favicon loads), while this bug is about the order in which we look for icon definitions in the head of the document. The load still happens later, afaict.