Closed Bug 1480319 Opened 2 years ago Closed 2 years ago

Minimize the amount of ContentLinkHandler code loaded by default

Categories

(Firefox :: General, enhancement, P1)

Product:
Firefox
Component:
General
Version:
57 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [overhead:44K])

Attachments

(1 file)

Bug 1480319: Split favicon loading code out of ContentLinkHandler.jsm.
59 bytes, text/x-review-board-request
mossop
: review+
Details 
ContentLinkHandler contains a lot of code devoted to loading favicons, which is loaded in every content process, all the time.

While it will generally always be needed for any content process that hosts a top-level HTTP document, it generally shouldn't be needed for many about: pages, and should never be needed out-of-process iframes.

That code should all be split out into a separate module, and loaded only when we actually need to load a favicon.
(apparently I botched my commit and lost half of my changes the first time)
Triage: p1 as there are patches in active development
Priority: -- → P1
Comment on attachment 8996920 [details]
Bug 1480319: Split favicon loading code out of ContentLinkHandler.jsm.

https://reviewboard.mozilla.org/r/260910/#review268744

I'm not seeing where the event listeners for the page are added now, is there a file missing here?

Either way I want to see this rebased on top of the changes that just landed in bug 1478823
Attachment #8996920 - Flags: review?(dtownsend)
Comment on attachment 8996920 [details]
Bug 1480319: Split favicon loading code out of ContentLinkHandler.jsm.

https://reviewboard.mozilla.org/r/260910/#review269538
Attachment #8996920 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa14dcca91fb46a6e44a68d2b0370dd0eff8d400
Bug 1480319: Split favicon loading code out of ContentLinkHandler.jsm. r=Mossop
Whiteboard: [overhead:18K] → [overhead:44K]
https://hg.mozilla.org/integration/mozilla-inbound/rev/0161b3bbc9d21cb915ffb3fe6b0aead720141c0b
Bug 1480319: Follow-up: Don't try to guess favicons during WebRequest test. r=bustage CLOSED TREE
https://hg.mozilla.org/integration/mozilla-inbound/rev/186ad2d171c1f2348703b119ad5983515ead58a7
Bug 1480319: Follow-up: Re-enable favicon guessing during WebRequest tests. r=Mossop
Backed out for mochitest failures on test_ext_webrequest_filter.html.

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/e19fc7991514a647468e3b80c798db58b06cf15b

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=186ad2d171c1f2348703b119ad5983515ead58a7&selectedJob=194194128

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=194202843&repo=mozilla-inbound&lineNumber=5529

00:16:43     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_filter.html | correct tabId - Expected: 46, Actual: 46 
00:16:43     INFO - Buffered messages finished
00:16:43     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_filter.html | Test timed out. 
00:16:43     INFO -     reportError@SimpleTest/TestRunner.js:121:7
00:16:43     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
00:16:43     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:43     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:43     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:43     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:43     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:43     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:43     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:43     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:43     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:43     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:43     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:43     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:43     INFO -     TestRunner.runTests/<@SimpleTest/TestRunner.js:381:9
00:16:43     INFO -     promise callback*TestRunner.runTests@SimpleTest/TestRunner.js:368:50
00:16:43     INFO -     RunSet.runtests@SimpleTest/setup.js:194:3
00:16:43     INFO -     RunSet.runall@SimpleTest/setup.js:173:5
00:16:43     INFO -     hookupTests@SimpleTest/setup.js:266:5
00:16:43     INFO - parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5
00:16:43     INFO - getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11
00:16:43     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
00:16:43     INFO -     hookup@SimpleTest/setup.js:246:5
00:16:43     INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=c%3A%5Cusers%5Ctask_1534375853%5Cappdata%5Clocal%5Ctemp&cleanupCrashes=true:11:1
00:16:44     INFO - Not taking screenshot here: see the one that was previously logged
00:16:44     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_filter.html | Extension left running at test shutdown 
00:16:44     INFO -     ExtensionTestUtils.loadExtension/<@SimpleTest/ExtensionTestUtils.js:109:7
00:16:44     INFO -     executeCleanupFunction@SimpleTest/SimpleTest.js:1227:19
00:16:44     INFO -     SimpleTest.finish@SimpleTest/SimpleTest.js:1240:5
00:16:44     INFO -     killTest@SimpleTest/TestRunner.js:130:7
00:16:44     INFO -     delayedKillTest@SimpleTest/TestRunner.js:157:47
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:157:7
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:44     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
00:16:44     INFO -     TestRunner.runTests/<@SimpleTest/TestRunner.js:381:9
00:16:44     INFO -     promise callback*TestRunner.runTests@SimpleTest/TestRunner.js:368:50
00:16:44     INFO -     RunSet.runtests@SimpleTest/setup.js:194:3
00:16:44     INFO -     RunSet.runall@SimpleTest/setup.js:173:5
00:16:44     INFO -     hookupTests@SimpleTest/setup.js:266:5
Flags: needinfo?(kmaglione+bmo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5746fe67e2d252035ca8257bd36d6b40d80fde39
Bug 1480319: Split favicon loading code out of ContentLinkHandler.jsm. r=Mossop
https://hg.mozilla.org/mozilla-central/rev/5746fe67e2d2
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1483910
Flags: needinfo?(kmaglione+bmo)

This is an extremely minor issue report, but I've noticed that Firefox 74.0 is -- unreliably, perhaps due to a race condition -- sometimes making requests for a favicon.ico at a base domain path (GET /favicon.ico) despite the page specifying alternate, specific rel="icon" paths.

The investigation I'd performed so far is visible at https://github.com/grocy/grocy/pull/692 and I tracked this back via the LinkHandlerChild call (at tag FIREFOX_NIGHTLY_74_END) to the commit associated with this bug ( https://hg.mozilla.org/mozilla-central/annotate/f53ecf8eee2a0d2a7705d7ea3ec8682028e922a0/browser/actors/LinkHandlerChild.jsm#l72 ).

(In reply to James Addison from comment #15)

This is an extremely minor issue report, but I've noticed that Firefox 74.0 is -- unreliably, perhaps due to a race condition -- sometimes making requests for a favicon.ico at a base domain path (GET /favicon.ico) despite the page specifying alternate, specific rel="icon" paths.

The investigation I'd performed so far is visible at https://github.com/grocy/grocy/pull/692 and I tracked this back via the LinkHandlerChild call (at tag FIREFOX_NIGHTLY_74_END) to the commit associated with this bug ( https://hg.mozilla.org/mozilla-central/annotate/f53ecf8eee2a0d2a7705d7ea3ec8682028e922a0/browser/actors/LinkHandlerChild.jsm#l72 ).

Please file a new bug for this issue.

(In reply to Dave Townsend [:mossop] (he/him) from comment #16)

Please file a new bug for this issue.

Thanks - I've identified https://bugzilla.mozilla.org/show_bug.cgi?id=1186324 as an existing bug for the same issue.

You need to log in before you can comment on or make changes to this bug.