User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:43.0) Gecko/20100101 Firefox/43.0 Build ID: 20160105164030 Steps to reproduce: Tossed realfavicongenerator.net into WebPageTest. My results visible here: http://www.webpagetest.org/result/160212_K0_7Y5/2/details/ (Imported from https://bugzilla.mozilla.org/show_bug.cgi?id=751712#c7) Actual results: Firefox kicks off downloads for every specified <link rel="icon"> as soon as the parser finds them, which blocks downloading of actual content until those requests finish. (Additionally, these requests do not appear in the Network tab of Devtools, which masks the issue.) Expected results: Arguably there's another bug here, where DevTools don't show icon downloads, but eh. Downloading icons at the beginning of the loading process like this is a pretty major performance slowdown; as the number of icons goes up, the more HTTP connections are used up for files that users won't even see. Sites trying to maximize compatibility with tools like RealFaviconGenerator can even tie up all 8 allowable simultaneous connections, which for spotty or high-latency networks, blocks page rendering to an unacceptable extent. As the creator of RealFaviconGenerator puts it: > More and more favicon generators create several PNG icons: > - RealFaviconGenerator > - Favic-o-matic > - Faviconit > - Epicfavicongenerator > - Favicon-generator > - ... > > RealFaviconGenerator alone created nearly 500,000 favicons in a year and a half. The corresponding WordPress plugin has more than 60,000 active installs. But even one icon download is a hiccup that makes Firefox slower than other browsers, which tends to be a bad thing. Internet Explorer, Chrome, Safari, and Opera all download icons after onLoad, which seems reasonable.

Patrick, for reference see also those other too bugs, mainly bug 685740.

(In reply to Honza Bambas (:mayhemer) from comment #1) > Patrick, for reference see also those other too bugs, mainly bug 685740. (Actually, forget bug 890618 at all)

Patrick, stealing this from you now, since this is closely related to bug 685740. I want to delay all loads that we can delay using some central point. The rel icon references are definitely candidates.

Loading (a new profile) http://realfavicongenerator.net/ shows the sources refers: <link rel="icon" type="image/png" href="/the_favicon/favicon-32x32.png" sizes="32x32"> <link rel="icon" type="image/png" href="/the_favicon/favicon-16x16.png" sizes="16x16"> But I don't see any of the requests to these URLs being made.

Firefox desktop apparently doesn't use link rel=icon at all. I don't see a request made. If I have a live example where a preload happens, please provide it. Closing as invalid for now.

It does, but unfortunately DevTools don't show favicon requests for some reason. If viewed through a proxy or WebPageTest, the network requests are plainly visible: https://www.webpagetest.org/result/160212_K0_7Y5/2/details/

(In reply to tigt from comment #6) > It does, but unfortunately DevTools don't show favicon requests for some > reason. If viewed through a proxy or WebPageTest, the network requests are > plainly visible: https://www.webpagetest.org/result/160212_K0_7Y5/2/details/ I can't reproduce it. Putting a breakpoint to nsHttpChannel::AsyncOpen doesn't trigger for icons. I was also tracing the parser code and we DON'T do any preloads or loads for rel=icon links, sorry. Can you give me some page where you can see these requests happen?

Ah! It's happening on the parent process, not on the child (also reason why it's not listed in devtools). Catching it now. Not sure why not when I was first playing with this bug a long ago...

Note for me: There are two requests, from the same callstack... xul.dll!mozilla::net::nsHttpChannel::AsyncOpen(0x00000235d98ebf10, 0x0000000000000000) Line 6157 C++ xul.dll!mozilla::net::nsHttpChannel::AsyncOpen2() Line 6356 C++ > xul.dll!imgLoader::LoadImage(0x00000235d9713b00, 0x00000235d6939c00, 0x00000235d6939c00, RP_Unset, 0x00000235d9f59a20, 0x00000235d6441690, 0x00000235d6327190, 0x00000235d9616ce0, 0x00000235d78cc000, 5120, 0x0000000000000000, 41, {...}, false, 0x00000235d4b90f48) Line 2270 C++ xul.dll!nsContentUtils::LoadImage(0x00000235d9713b00, 0x00000235d9616ce0, 0x00000235d78cc000, 0x00000235d9f59a20, 0x00000235d6939c00, RP_Unset, 0x00000235d6327190, 5120, {...}, 0x00000235d4b90f48, 41, false) Line 3721 C++ xul.dll!nsImageBoxFrame::UpdateImage() Line 256 C++ xul.dll!nsCSSFrameConstructor::InitAndRestoreFrame({...}, , , 0x00000235d4b90ec0, true) Line 5093 C++ xul.dll!nsCSSFrameConstructor::ConstructFrameFromItemInternal({...}, {...}, 0x00000235d96988b0, {...}) Line 4012 C++ xul.dll!nsCSSFrameConstructor::ConstructFramesFromItem({...}, , 0x00000235d96988b0, {...}) Line 6363 C++ xul.dll!nsCSSFrameConstructor::ConstructFramesFromItemList({...}, , 0x00000235d96988b0, {...}) Line 10995 C++ xul.dll!nsCSSFrameConstructor::ContentRangeInserted(0x00000235d9616bc0, , 0x00000235d9616d70, 0x00000235d8988160, false, true, 0x0000000000000000) Line 8388 C++ xul.dll!nsCSSFrameConstructor::RecreateFramesForContent(0x00000235d9616ce0, false, REMOVE_CONTENT, 0x0000000000000000) Line 10068 C++ xul.dll!nsCSSFrameConstructor::MaybeRecreateFramesForElement(0x00000235d9616ce0) Line 9667 C++ xul.dll!mozilla::GeckoRestyleManager::RestyleElement(0x00000235d9616ce0, , nsChangeHint_Empty, {...}, 3, {...}) Line 240 C++ xul.dll!mozilla::RestyleTracker::ProcessOneRestyle(, , , {...}) Line 94 C++ xul.dll!mozilla::RestyleTracker::DoProcessRestyles() Line 257 C++ xul.dll!mozilla::GeckoRestyleManager::ProcessPendingRestyles() Line 579 C++ xul.dll!mozilla::PresShell::DoFlushPendingNotifications() Line 4195 C++ xul.dll!nsRefreshDriver::Tick(, {...}) Line 1892 C++ xul.dll!mozilla::RefreshDriverTimer::TickDriver(0x00000235d7864000, 1501447241237973, {...}) Line 329 C++ xul.dll!mozilla::RefreshDriverTimer::TickRefreshDrivers(1501447241237973, {...}, ) Line 300 C++ xul.dll!mozilla::RefreshDriverTimer::Tick(1501447241237973, {...}) Line 322 C++ xul.dll!mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers({...}) Line 762 C++ xul.dll!mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver({...}) Line 677 C++ xul.dll!mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() Line 521 C++ xul.dll!nsThread::ProcessNextEvent(, 0x00000091561ff480) Line 1447 C++ And the request context is different (this is loaded by the xul window) than the document request context. Hence can't simply use the Tail flag :(

The first though is to expose the request context id on the xul element side by the content loading principal (nsContentUtils::GetContentPolicyTypeForUIImageLoading) and either set it on the xul loadgroup or change the loadgroup of this favicon load artificially.

This is a duplicate of bug 685740, comment 3, first of the two described requests. I roughly tried to alter (prefix) the url with both moz-anno:favicon: and page-icon: at [1] but that just broke the favicon load when referred with link rel=icon. Marco? I think trying to solve this + your concerns at [0] would be the best solution for this bug. The idea from comment 10 is not that easy to achieve since the request context id is quite a network-y thing, not exposed on documents and windows that at [2] we would have an access to (it's the place we serialize the loading principal later used on the parent process to be replaced on the favicon loading channel). Other way could be to store to xul the top level outer window id which is something known to channels and could be associated with the request context id. I only need to find the central place where the win id is known first and associate the rc with it. Probably something to add to doc shell (which links directly to its load group, which links to its request context - and it's a non-changing chain after it's established, tho, the win id change on reload). [0] https://bugzilla.mozilla.org/show_bug.cgi?id=685740#c2 [1] https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/image/imgLoader.cpp#755 [2] https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/browser/base/content/browser-places.js#1610

All I said in https://bugzilla.mozilla.org/show_bug.cgi?id=685740#c2 is still valid. At this time we don't have a good solution for PB mode, making the Places icon protocols redirect to the network may create privacy/security concerns. Creating a temporary store for PB mode is feasible, but requires resources we are lacking (it actually depends on the priority/importance of this work compared to all the other things we have to do). We can't just use that Places protocol in the tabs, because in PB mode it would have no icon to return. We could maybe use it the window is not pb and we are not in permanent pb mode. But then the tab should listen to icon changes through an observer, and reload the image once the fetch is complete. It's not just a trivial replacement. In the end the bug seems to point out we should just delay the favicon load, and that sounds like a solution that may help in general. All of the code doing that is in tabbrowser.xml IIRC.

The patch is trying to pass the content-document -> docshell -> loadgroup -> requestcontextid via a xul attribute the same way we are passing the content loadingprincipal serialization. It can then be simply set on the loading parent channel what makes it eligible for simple tailing via Tail class-of-service attribute (bug 1358060) and fixes this bug nicely. If anyone can give me some clues if I'm on the right path (passing the attribute) and what all is needed to do to pass requestcontextid through the right <xul:image> element, please let me know, it would be greatly appreciated. I think the starting point is at: https://dxr.mozilla.org/mozilla-central/rev/ef9a0f01e4f68214f0ff8f4631783b8a0e075a82/browser/base/content/tabbrowser.xml#7460

No time to work on this now.

Let's try to find an owner for this and fix it for 57. This bug has a low change for causing a regression.

(In reply to Honza Bambas (:mayhemer) from comment #15) > change for causing a regression. *chance*

I'd like to work on this bug.

Note that, if you plan to touch ContentLinkHandler or tabbrowser, you may clash with bug 1352459 where Activity Stream will likely change the way we fetch icons. There I'm also suggesting to fetch icons on a timer (for other reasons), I still don't know what will be the final incarnation of that work, but it was worth notifying about here here to avoid bitrotting each other in the last weeks of 57.

Honza, is that enough if we just lower the priority and add throttle flag to the channel loading favicon? At a quick glance, it seems to me trying to requestcontextid is not that easy. Thanks.

(In reply to Kershaw Chang [:kershaw] from comment #19) > Honza, is that enough if we just lower the priority and add throttle flag to > the channel loading favicon? It's not enough to completely use the potential here - we know these icon requests are of very low priority and tailing is ideal way to do with them. However, we can do at least something, as you suggest, throttle and lower the priority, yes. But please file a new bug for that and leave this open. I'd like to sit one day and pass correctly the request context id up so Tail will then simply work. > > At a quick glance, it seems to me trying to requestcontextid is not that > easy. I think we are not that far to do it. I would not try to figure this out myself, though, it's better to ask around and find people that wrote the code carrying the loading principal serialization or anyone who is familiar with these parts. They could help. > > Thanks.

Summary: - Add contentRequestContextID property to browser.xml - This contentRequestContextID will be used when useDefaultIcon is called. Hi Marco, Could you take a look at this patch? Please let me know if I am on the right track. Thanks.

Summary: - Get the request context ID of the document load group, when <link rel="icon”> is found in the page. - Add new argument requestContextID to setIcon - Pass requestContextID via XUL attribute

This patch eventually calls SetRequestContextID to set request context ID to the channel used for loading a favicon.

Comment on attachment 8905817 [details] [diff] [review] Part1: Add new property - contentRequestContextID Review of attachment 8905817 [details] [diff] [review]: ----------------------------------------------------------------- off-hand it looks ok, I may not be the best person for a final review on this though, maybe mconley?

Comment on attachment 8905820 [details] [diff] [review] Part2: Set request context ID for the channel used to load favicon Review of attachment 8905820 [details] [diff] [review]: ----------------------------------------------------------------- it looks correct afaict. Is there any way to write a test for this? ::: browser/base/content/tabbrowser.xml @@ +983,5 @@ > ? aLoadingPrincipal > : Services.scriptSecurityManager.getSystemPrincipal(); > + let requestContextID = aRequestContextID > + ? aRequestContextID > + : 0; = aRequestContextId || 0; ::: toolkit/components/places/mozIAsyncFavicons.idl @@ +54,5 @@ > * Principal of the page whose favicon is being set. If this argument > * is omitted, the loadingPrincipal defaults to the nullPrincipal. > + * @param [optional] aRequestContextID > + * RequestContextID here is used to inform Necko of how to link the > + * favicon request with other requests in the same tab. s/RequestContextID here is //

Comment on attachment 8905817 [details] [diff] [review] Part1: Add new property - contentRequestContextID Please see comment #21 and take a look. Thanks.

(In reply to Marco Bonardo [::mak] from comment #25) > Comment on attachment 8905820 [details] [diff] [review] > Part2: Set request context ID for the channel used to load favicon > > Review of attachment 8905820 [details] [diff] [review]: > ----------------------------------------------------------------- > > it looks correct afaict. Is there any way to write a test for this? > Thanks for the review. I'll write a test for this.

This patch is basically all about adding aRequestContextID argument to nsContentUtils::LoadImage. Hi baku, could you take a look at this patch? This patch is a bit like part3 patch in bug 1348050, which was reviewed by you. Thanks.

Summary: - Fix comments in the previous run. Please take a look. Thanks.

Comment on attachment 8905817 [details] [diff] [review] Part1: Add new property - contentRequestContextID Review of attachment 8905817 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsILoadGroup.idl @@ +80,5 @@ > /** > * Context for managing things like js/css connection blocking, > * and per-tab connection grouping. > */ > + readonly attribute unsigned long long requestContextID; You'll probably want someone from Necko to sign-off on this change. ::: toolkit/content/browser-child.js @@ +187,5 @@ > json.synthetic = content.document.mozSyntheticDocument; > json.inLoadURI = WebNavigation.inLoadURI; > + json.requestContextID = content.document.documentLoadGroup > + ? content.document.documentLoadGroup.requestContextID > + : 0; What does 0 mean here? In the non-remote case, presumably if documentLoadGroup does not exist, content.document.documentLoadGroup.requestContextID will throw, and browser.xml will return null... shouldn't we do the same thing here?

Comment on attachment 8905817 [details] [diff] [review] Part1: Add new property - contentRequestContextID Review of attachment 8905817 [details] [diff] [review]: ----------------------------------------------------------------- Minus the null vs 0 thing in browser-child.js, this looks okay to me. Thanks!

Test steps: 1. Create a tab with network.http.tailing.enabled set to true. 2. Check whether the http channel that is used to load favicon has a correct tail flag. 3. Repeat step1, but with network.http.tailing.enabled set to false. Please take a look. Thanks.

(In reply to Mike Conley (:mconley) (:⚙️) from comment #30) > Comment on attachment 8905817 [details] [diff] [review] > Part1: Add new property - contentRequestContextID > > Review of attachment 8905817 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/base/nsILoadGroup.idl > @@ +80,5 @@ > > /** > > * Context for managing things like js/css connection blocking, > > * and per-tab connection grouping. > > */ > > + readonly attribute unsigned long long requestContextID; > > You'll probably want someone from Necko to sign-off on this change. > I'll do it. Thanks. > ::: toolkit/content/browser-child.js > @@ +187,5 @@ > > json.synthetic = content.document.mozSyntheticDocument; > > json.inLoadURI = WebNavigation.inLoadURI; > > + json.requestContextID = content.document.documentLoadGroup > > + ? content.document.documentLoadGroup.requestContextID > > + : 0; > > What does 0 mean here? In the non-remote case, presumably if > documentLoadGroup does not exist, > content.document.documentLoadGroup.requestContextID will throw, and > browser.xml will return null... shouldn't we do the same thing here? 0 means that requestContextID is not set from networking point of view. I agree with you that the behavior here should be the same as the non-remote case. I'll fix this.

Comment on attachment 8905817 [details] [diff] [review] Part1: Add new property - contentRequestContextID Nick, Please just see the change in nsILoadGroup.idl. Is there any concern for exposing requestContextID to script? Thanks.

Comment on attachment 8905817 [details] [diff] [review] Part1: Add new property - contentRequestContextID Review of attachment 8905817 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsILoadGroup.idl @@ +80,5 @@ > /** > * Context for managing things like js/css connection blocking, > * and per-tab connection grouping. > */ > + readonly attribute unsigned long long requestContextID; This is fine, since it's staying readonly. The [noscript] bit was leftover from when the rcid was an nsID (a type not usable from script).

Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment on attachment 8907140 [details] [diff] [review] Part2: Set request context ID for the channel used to load favicon Review of attachment 8907140 [details] [diff] [review]: ----------------------------------------------------------------- It looks ok, my only doubt is the bitrotting with bug 1352459 that landed and then was backed out, and there's risk that either this delays that patch, or unbitrotting that patch may lose the changes done here. I'd suggest to wait for that one to land if possible since it does a larger refactoring. Moreover that patch delays favicons by 100ms, and that could also somehow make this less urgent.

Comment on attachment 8907450 [details] [diff] [review] Part4: Test case Review of attachment 8907450 [details] [diff] [review]: ----------------------------------------------------------------- There are a few things that could be done through existing helpers ::: browser/base/content/test/performance/browser.ini @@ +19,5 @@ > skip-if = (os == 'linux') || (os == 'mac' && !debug) # Disabled on Linux and OS X opt due to frequent failures. Bug 1385932 and Bug 1384582 > [browser_windowclose_reflows.js] > [browser_windowopen_reflows.js] > skip-if = os == 'linux' # Disabled due to frequent failures. Bug 1380465. > +[browser_favicon_load.js] Looks like the tests here are kept alphabetically ordered, so please keep that sorting. ::: browser/base/content/test/performance/browser_favicon_load.js @@ +6,5 @@ > + > +const { classes: Cc, Constructor: CC, interfaces: Ci, utils: Cu } = Components; > + > +XPCOMUtils.defineLazyModuleGetter(this, "Promise", > + "resource://gre/modules/Promise.jsm"); Please not, we use DOM promises, this is an old wrapper. @@ +21,5 @@ > +const THIRD_PARTY_FAVICON_URI = TEST_THIRD_PARTY_SITE + "/browser/browser/components/" + > + "originattributes/test/browser/file_favicon.png"; > + > +let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal(); > +let makeURI = Cu.import("resource://gre/modules/BrowserUtils.jsm", {}).BrowserUtils.makeURI; Just use Services.io.newURI @@ +45,5 @@ > + } > + } > + }; > + > + Services.obs.addObserver(observer, "places-favicons-expired"); Services.obs.addObserver(function observer() { Services.obs.removeObserver(observer, "places-favicons-expired"); resolve(); }, "places-favicons-expired"); @@ +46,5 @@ > + } > + }; > + > + Services.obs.addObserver(observer, "places-favicons-expired"); > + faviconService.expireAllFavicons(); Just use PlacesUtils.favicons.expire... (no need to getService manually) @@ +100,5 @@ > + reset(aPageURI, aFaviconURL, aTailingEnabled) { > + this._faviconReqXUL = false; > + this._faviconReqPlaces = false; > + this._faviconURL = aFaviconURL; > + this._faviconLoaded = new Promise.defer(); If you really need to use a defer, import PromiseUtils.jsm instead of Promise.jsm @@ +124,5 @@ > + }; > + > + PlacesUtils.history.addObserver(observer); > + }); > +} nit: You could use PlacesTestUtils.waitForNotification @@ +136,5 @@ > + > + let browser = gBrowser.getBrowserForTab(tab); > + await BrowserTestUtils.browserLoaded(browser); > + return tab; > +} Off-hand looks like this function is just reimplementing BrowserTestUtils.openNewForegroundTab... @@ +171,5 @@ > +registerCleanupFunction(() => { > + // Clear all cookies. > + let cookieMgr = Cc["@mozilla.org/cookiemanager;1"] > + .getService(Ci.nsICookieManager); > + cookieMgr.removeAll(); Services.cookies.removeAll @@ +178,5 @@ > + clearAllImageCaches(); > + > + let networkCache = Cc["@mozilla.org/netwerk/cache-storage-service;1"] > + .getService(Ci.nsICacheStorageService); > + networkCache.clear(); Services.cache2. @@ +194,5 @@ > + .getService(Ci.nsICacheStorageService); > + networkCache.clear(); > + > + // Clear Places favicon caches. > + await clearAllPlacesFavicons(); you could factor out a cleanup() function and use it both here and in registerCleanupFunction @@ +213,5 @@ > + .getService(Ci.nsICacheStorageService); > + networkCache.clear(); > + > + // Clear Places favicon caches. > + await clearAllPlacesFavicons(); ditto (reuse a common cleanup helper)

Fix all comments in the previous review. Thanks!

(In reply to Marco Bonardo [::mak] from comment #38) > Comment on attachment 8907140 [details] [diff] [review] > Part2: Set request context ID for the channel used to load favicon > > Review of attachment 8907140 [details] [diff] [review]: > ----------------------------------------------------------------- > > It looks ok, my only doubt is the bitrotting with bug 1352459 that landed > and then was backed out, and there's risk that either this delays that > patch, or unbitrotting that patch may lose the changes done here. I'd > suggest to wait for that one to land if possible since it does a larger > refactoring. > Moreover that patch delays favicons by 100ms, and that could also somehow > make this less urgent. Thanks for the review. I'll rebase this until bug 1352459 is landed.

Comment on attachment 8908043 [details] [diff] [review] Part4: Test case Review of attachment 8908043 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/performance/browser_favicon_load.js @@ +24,5 @@ > +let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal(); > + > +function clearAllImageCaches() { > + var tools = SpecialPowers.Cc["@mozilla.org/image/tools;1"] > + .getService(SpecialPowers.Ci.imgITools); nit: I don't think you need SpecialPowers.Cc in a mochitest-browser @@ +144,5 @@ > + Services.cache2.clear(); > + // Clear Places favicon caches. > + clearAllPlacesFavicons(); > + // Clear all image caches and network caches. > + clearAllImageCaches(); You may want to also await PlacesUtils.history.clear(); if you're adding stuff to history

(In reply to Kershaw Chang [:kershaw] from comment #28) > Created attachment 8907139 [details] [diff] [review] > Part3: Pass requestContextID to channel > > This patch is basically all about adding aRequestContextID argument to > nsContentUtils::LoadImage. > > Hi baku, could you take a look at this patch? This patch is a bit like part3 > patch in bug 1348050, which was reviewed by you. > > Thanks. @baku, I'm just asking, not pushing. If you are too busy to review this, could you suggest someone else? Thanks.

Comment on attachment 8907139 [details] [diff] [review] Part3: Pass requestContextID to channel Review of attachment 8907139 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsContentUtils.cpp @@ +10450,5 @@ > /* static */ void > nsContentUtils::GetContentPolicyTypeForUIImageLoading(nsIContent* aLoadingNode, > nsIPrincipal** aLoadingPrincipal, > + nsContentPolicyType& aContentPolicyType, > + uint64_t& aRequestContextID) can you pass a pointer here instead? @@ +10455,1 @@ > { check if aRequestContextID has been passed as pointer. ::: dom/base/nsContentUtils.h @@ +3031,5 @@ > static void > GetContentPolicyTypeForUIImageLoading(nsIContent* aLoadingNode, > nsIPrincipal** aLoadingPrincipal, > + nsContentPolicyType& aContentPolicyType, > + uint64_t& aRequestContextID); same here? ::: layout/xul/nsImageBoxFrame.cpp @@ +234,5 @@ > nsIDocument* doc = mContent->GetComposedDoc(); > if (doc) { > nsContentPolicyType contentPolicyType; > nsCOMPtr<nsIPrincipal> loadingPrincipal; > + uint64_t requestContextID; = 0 ?

Hi Marco, I've rebased this patch on bug 1352459. It'd be great if you can take a look again. Thanks.

Comment on attachment 8910144 [details] [diff] [review] Part2: Set request context ID for the channel used to load favicon Review of attachment 8910144 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/ContentLinkHandler.jsm @@ +184,5 @@ > +function getLinkRequestContextID(aLink) { > + try { > + return aLink.ownerDocument.documentLoadGroup.requestContextID; > + } catch (e) { > + return null; nit: we seem to use 0 all around as a fallback for this id, and it's passed around as an unsigned int. why null here instead of just 0?

(In reply to Marco Bonardo [::mak] from comment #46) > Comment on attachment 8910144 [details] [diff] [review] > Part2: Set request context ID for the channel used to load favicon > > Review of attachment 8910144 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/modules/ContentLinkHandler.jsm > @@ +184,5 @@ > > +function getLinkRequestContextID(aLink) { > > + try { > > + return aLink.ownerDocument.documentLoadGroup.requestContextID; > > + } catch (e) { > > + return null; > > nit: we seem to use 0 all around as a fallback for this id, and it's passed > around as an unsigned int. why null here instead of just 0? Please see comment#30. I just want to make the way we get requestContextID from documentLoadGroup consistent.

Carry r+.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a3816cf18525 Part 1: Add new property - contentRequestContextID. r=mconley, r=hurley https://hg.mozilla.org/integration/mozilla-inbound/rev/9237c64b078d Part 2: Set request context ID for the channel used to load favicon. r=mak https://hg.mozilla.org/integration/mozilla-inbound/rev/1615dd915bb8 Part 3: Set request context ID to the http channel created in imgLoader::LoadImage. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/d177f35862ab Part 4: A test case for testing whether the channel used to load favicon. r=mak

https://hg.mozilla.org/mozilla-central/rev/a3816cf18525 https://hg.mozilla.org/mozilla-central/rev/9237c64b078d https://hg.mozilla.org/mozilla-central/rev/1615dd915bb8 https://hg.mozilla.org/mozilla-central/rev/d177f35862ab

Comment on attachment 8910575 [details] [diff] [review] Part1: Add new property - contentRequestContextID, r=mconley,hurley This is a CDP bug (feature) we would like to have on 57, but this not a critical feature that is a "must have" for 57. I'll leave this to drivers to decide if it's worth uplifting or not. The patch grew up a bit beyond my expectation. Approval Request Comment [Feature/Bug causing the regression]: none [User impact if declined]: potential minor slowdown of page loads [Is this code covered by automated tests?]: only partially [Has the fix been verified in Nightly?]: no (I intent to, tho) [Needs manual test from QE? If yes, steps to reproduce]: it's no easy to verify this using only firefox dev tools or so ; one way to verify is to examine http logs after loading realfavicongenerator.net web page - requests for icons should be made after the document loaded, definitely not sooner than DOMContentLoaded has fired [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: hard to assess for me, but the changes seems straightforward enough to say it's not risky [Why is the change risky/not risky?]: based only on reading the patches roughly [String changes made/needed]: none

Comment on attachment 8910576 [details] [diff] [review] Part2: Set request context ID for the channel used to load favicon, r=mak (comment 55)

Comment on attachment 8910577 [details] [diff] [review] Part3: Pass requestContextID to channel, r=baku (comment 55)

Comment on attachment 8910578 [details] [diff] [review] Part4: Test case, r=mak (comment 55)

As 57 is all about performance, do you have some data about a potential perf gain here? Thanks

(In reply to Sylvestre Ledru [:sylvestre] from comment #59) > As 57 is all about performance, do you have some data about a potential perf > gain here? > Thanks That's a problem, I don't. We could demo this on WPT though. Gary, could you please run your web page test on this patch and find out if we speed up speed-index or some of the metrics on realfavicongenerator.net with this patch? Thanks.

Wrong Gary.

If you can prove that it is worth the risk, we can discuss but we should land that in b3 or b4 (next week). We have data which demonstrates that patches targeting performance have a 50% chance to introduce regressions

(In reply to Honza Bambas (:mayhemer) from comment #60) > (In reply to Sylvestre Ledru [:sylvestre] from comment #59) > > As 57 is all about performance, do you have some data about a potential perf > > gain here? > > Thanks > > That's a problem, I don't. We could demo this on WPT though. > > Gary, could you please run your web page test on this patch and find out if > we speed up speed-index or some of the metrics on realfavicongenerator.net > with this patch? Thanks. Gary, I think you just have to use the latest nightly (already includes this patch) to do the test. Please do the test twice, first time with "network.http.tailing.enabled" on and the other time off. Thanks.

Verified the patch works! 2017-09-25 15:22:36.551 ⁃ nsHttpChannel ⁃ 23d238ff000 ⁃ released ⁃ 200 ⁃ https://realfavicongenerator.net/the_favicon/favicon-32x32.png?v=XBr4XjEpXx ⁃ 0 2017-09-25 15:22:36.551 │ Main Thread │ Creating nsHttpChannel [this=0000023D238FF000] 2017-09-25 15:22:36.551 │ Main Thread │ HttpBaseChannel::Init [this=0000023D238FF000] 2017-09-25 15:22:36.551 │ Main Thread │ nsHttpChannel::Init [this=0000023D238FF000] 2017-09-25 15:22:36.551 │ Main Thread │ nsHttpChannel::SetPriority 0000023D238FF000 p=20 2017-09-25 15:22:36.551 │ Main Thread │ nsHttpChannel::OnClassOfServiceUpdated this=0000023D238FF000, cos=288 2017-09-25 15:22:36.551 │ Main Thread │ cos = Throttleable, Tail 2017-09-25 15:22:36.551 │ Main Thread │ nsHttpChannel::AsyncOpen [this=0000023D238FF000] 2017-09-25 15:22:36.551 │ Main Thread │ HttpBaseChannel::EnsureRequestContextID this=0000023D238FF000 id=21ac00000002 2017-09-25 15:22:36.551 │ Main Thread │ nsHttpChannel::WaitingForTailUnblock this=0000023D238FF000, rc=0000023D28770A20 ?:1134 @23d28770a20 2017-09-25 15:22:36.551 │ Main Thread │ blocked=1 2017-09-25 15:22:38.017 │ Main Thread │ nsHttpChannel::OnTailUnblock this=0000023D238FF000 rv=0 rc=0000023D28770A20 2017-09-25 15:22:38.017 │ Main Thread │ after 1466ms Thanks Kershaw!

(In reply to Kershaw Chang [:kershaw] from comment #63) > (In reply to Honza Bambas (:mayhemer) from comment #60) > > (In reply to Sylvestre Ledru [:sylvestre] from comment #59) > > > As 57 is all about performance, do you have some data about a potential perf > > > gain here? > > > Thanks > > > > That's a problem, I don't. We could demo this on WPT though. > > > > Gary, could you please run your web page test on this patch and find out if > > we speed up speed-index or some of the metrics on realfavicongenerator.net > > with this patch? Thanks. > > Gary, I think you just have to use the latest nightly (already includes this > patch) to do the test. > Please do the test twice, first time with "network.http.tailing.enabled" on > and the other time off. Thanks. Sorry for late reply. I've triggered those jobs.

(In reply to Honza Bambas (:mayhemer) from comment #60) > (In reply to Sylvestre Ledru [:sylvestre] from comment #59) > > As 57 is all about performance, do you have some data about a potential perf > > gain here? > > Thanks > > That's a problem, I don't. We could demo this on WPT though. > > Gary, could you please run your web page test on this patch and find out if > we speed up speed-index or some of the metrics on realfavicongenerator.net > with this patch? Thanks. The WPT result is available at http://10.247.28.241:8888/bug1247843_tailing.html (Mozilla internal VPN required). From the test result, I can say that this patch dramatically delays the time we start to load a favicon. Taking cnn.com as an example, firefox loads a favicon evan 10 times slower with this patch. I think this patch is really worth uplift, since the risk of this patch is quite low and the benefit is obvious. @sylvestre, what do you think?

This looks great! Bravo! However, we have data which shows that more than 50% of the uplifts for performance reasons will introduce a regression. Because our performance efforts are going to continue after 57, I would prefer to let that ride the train with 58.

This patches conflict with bug 1401777, so hopefully we can have a decision soon about the uplift, since they will both need an unbitrot if landed in reverse order.

I will conservative and not take it for 57.

Thanks for the quick reply, now I have a path to follow, I will unbitrot the other patch. (I'm sorry for the missed uplift, though I totally understand it).