Closed
Bug 1383299
Opened 7 years ago
Closed 7 years ago
Consider initiating a speculative connection in order to prepare Necko to give the parser in the content process data faster upon navigations from the url bar
Categories
(Firefox :: Address Bar, enhancement)
Firefox
Address Bar
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: jj.evelyn)
References
Details
Attachments
(4 files)
We need to speed up this case for the Quantum Release Criteria: Type in www.youtube.com in the location bar and press enter. Right now smaug has been seeing a long delay before Necko gives DOM and parser any data which causes latency in first non blank paint. We believe part of this is due to the WebNavigation:LoadURI message being sent from the parent process to the content process as part of the above use case. This means the initial Necko channel won't be created before the child process, in response to the WebNavigation:LoadURI message, IPCs back to the parent process. This delay which is roughly measured by FX_TAB_REMOTE_NAVIGATION_DELAY_MS is around 50ms per telemetry. It would be nice to experiment with creating a speculative connection around here <https://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/components/remotebrowserutils/RemoteWebNavigation.js#75> before sending this message to the content process to see if we can expedite the Necko side of things a bit.
Comment 1•7 years ago
|
||
I guess this is just a connection right now, but please be careful about doing anything here that impacts service worker. Until we finish our multi-e10s refactor we will not be able to support this kind of stuff very well. Also, there is a spec change that sites can use to help with navigation loads like this. See bug 1290958. Also, see the PlzNavigate work blink has done in this area.
Comment 2•7 years ago
|
||
Should this be a blocker for bug 1363772?
Comment 3•7 years ago
|
||
Evelyn, This is now QRC. Do you think if it's something you can do for fx57?
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p1]
Assignee | ||
Comment 4•7 years ago
|
||
I have a couple of bugs on hand that are related to this topic - bug 1355443 and bug 1355451. bug 1348275 also contribute to this case if the url had been auto-completed, which is very likely happening on Youtube. Ehsan, all these bugs try to set up connection before loadURIWithOptions(), and I feel we've covered many common cases. Do you think we need more? (In reply to Jean Gong from comment #3) > Evelyn, This is now QRC. Do you think if it's something you can do for fx57? If above bugs are good enough, then yes, they will be landed in fx57 timeframe.
Flags: needinfo?(ehsan)
Reporter | ||
Comment 5•7 years ago
|
||
(In reply to Evelyn Hung [:evelyn] from comment #4) > I have a couple of bugs on hand that are related to this topic - bug 1355443 > and bug 1355451. bug 1348275 also contribute to this case if the url had > been auto-completed, which is very likely happening on Youtube. > > Ehsan, all these bugs try to set up connection before loadURIWithOptions(), > and I feel we've covered many common cases. Do you think we need more? Bug 1355443 seems to me like a fairly special case and can probably be dealt with separately as your patch is already doing. Both bug 1348275 and bug 1355451 sound like special cases of this bug, as far as I can tell the only differences is about how the awesome bar entry result is picked. We have a few different possibilities (and I'm sure I'm going to forget some): typing in something in the url bar and press enter without waiting for autocomplete to do anything, wait for auto complete and activate it by keyboard, wait for autocomplete and activate it by mouse, etc.) I think they all should eventually go through loadURIWithOptions() in the parent process (please double check that, I may be wrong here but I can't find any other code path in the parent process side which sends the WebNativation:LoadURI message to the child.) Can we perhaps perform the speculative connection centrally in loadURIWithOptions() instead of addressing each case individually? If not, we should think about what code paths exist and how many of them we have covered so far and where we should stop, but I very much prefer if we find one central place to initiate the speculative connection to avoid having to fix too many of these bugs. :) (sorry the need for this came up so late -- if we had realized this is an issue for youtube navigation a couple of months ago we could have saved you a lot of work as you were preparing to work on this stuff.)
Flags: needinfo?(ehsan)
Comment 6•7 years ago
|
||
It seems to me this should not be a necko bug, but a front-end bug. Evelyn, could you select a proper component for this bug? Thanks.
Flags: needinfo?(ehsan)
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #5) > (In reply to Evelyn Hung [:evelyn] from comment #4) > > I have a couple of bugs on hand that are related to this topic - bug 1355443 > > and bug 1355451. bug 1348275 also contribute to this case if the url had > > been auto-completed, which is very likely happening on Youtube. > > > > Ehsan, all these bugs try to set up connection before loadURIWithOptions(), > > and I feel we've covered many common cases. Do you think we need more? > > Bug 1355443 seems to me like a fairly special case and can probably be dealt > with separately as your patch is already doing. > > Both bug 1348275 and bug 1355451 sound like special cases of this bug, as > far as I can tell the only differences is about how the awesome bar entry > result is picked. We have a few different possibilities (and I'm sure I'm > going to forget some): typing in something in the url bar and press enter > without waiting for autocomplete to do anything, wait for auto complete and > activate it by keyboard, wait for autocomplete and activate it by mouse, > etc.) I think they all should eventually go through loadURIWithOptions() in > the parent process (please double check that, I may be wrong here but I > can't find any other code path in the parent process side which sends the > WebNativation:LoadURI message to the child.) Can we perhaps perform the > speculative connection centrally in loadURIWithOptions() instead of > addressing each case individually? > loadURIWithOptions() might be the central place for navigating to a url, I will double check that. IMO the trade off of doing speculative connect centrally v.s. distributively in UI code is how *early* we want to set up the connection. The assumption I believe is, if we want the speculative connection being very useful, we should initiate it as early as possible so the connection is ready before the page fetching. Thus we do it when we *guess* the user is going to navigate to this url. So is it early enough to do it centrally in the place like loadURIWithOptions()? I checked the telemetry FX_TAB_REMOTE_NAVIGATION_DELAY_MS and it seems 50ms isn't in average case?
Component: Networking → Location Bar
Product: Core → Firefox
Reporter | ||
Comment 8•7 years ago
|
||
(In reply to Evelyn Hung [:evelyn] from comment #7) > IMO the trade off of doing speculative connect centrally v.s. distributively > in UI code is how *early* we want to set up the connection. The assumption I > believe is, if we want the speculative connection being very useful, we > should initiate it as early as possible so the connection is ready before > the page fetching. Thus we do it when we *guess* the user is going to > navigate to this url. So is it early enough to do it centrally in the place > like loadURIWithOptions()? I checked the telemetry > FX_TAB_REMOTE_NAVIGATION_DELAY_MS and it seems 50ms isn't in average case? FX_TAB_REMOTE_NAVIGATION_DELAY_MS is the delay to talk to the child process as far as I can tell which is what we want to get ahead of, that is, we want to tell Necko to start the connections before the child process gets a chance to initiate the Necko connection. Initiating the speculative connection centrally vs in the UI code should both have roughly the same latency, since the UI code usually directly calls addTab or loadURIWithFlags which is what call loadURIWithOptions. The difference between the two should be in the nano to microsecond range.
Flags: needinfo?(ehsan)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #8) > Initiating the speculative connection centrally vs in the UI code should > both have roughly the same latency, since the UI code usually directly calls > addTab or loadURIWithFlags which is what call loadURIWithOptions. The > difference between the two should be in the nano to microsecond range. No, in bug 1348275 and bug 1355451 we initiate the speculative connection earlier. bug 1348275 did it before the user hits enter key on the location bar, and bug 1355451 did on "mousedown" event, which is about hundred ms earlier than mouseup event (and mouseup triggers loadURIWithOptions). Adding speculative connection code here is easy, and (I believe) necko will properly control these connections(*), so I made this patch to catch more cases that we currently didn't cover in UI code. I tried to evaluate the before and after by adding profiler markers, but failed to understand how much performance gain of this patch. Profiling network performance is complicated. :( * I was told that for http/2, necko will keep just one connection(to a domain) alive no matter how much time we call speculative connect to the domain. For other cases, there is a limit that, for a domain, the connection upper bound is 6. Mike, could you help review this patch? :)
Assignee | ||
Updated•7 years ago
|
Attachment #8891965 -
Flags: review?(mconley)
Reporter | ||
Comment 11•7 years ago
|
||
(In reply to Evelyn Hung [:evelyn] from comment #10) > (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from > comment #8) > > Initiating the speculative connection centrally vs in the UI code should > > both have roughly the same latency, since the UI code usually directly calls > > addTab or loadURIWithFlags which is what call loadURIWithOptions. The > > difference between the two should be in the nano to microsecond range. > > No, in bug 1348275 and bug 1355451 we initiate the speculative connection > earlier. bug 1348275 did it before the user hits enter key on the location > bar, and bug 1355451 did on "mousedown" event, which is about hundred ms > earlier than mouseup event (and mouseup triggers loadURIWithOptions). Oh of course, you're right! I wasn't thinking of that. > Adding speculative connection code here is easy, and (I believe) necko will > properly control these connections(*), so I made this patch to catch more > cases that we currently didn't cover in UI code. > I tried to evaluate the before and after by adding profiler markers, but > failed to understand how much performance gain of this patch. Profiling > network performance is complicated. :( Perhaps someone who was running these YouTube tests can help. Andrew can you please help find out who that person is? Evelyn, do you have a try build for them to test? > * I was told that for http/2, necko will keep just one connection(to a > domain) alive no matter how much time we call speculative connect to the > domain. For other cases, there is a limit that, for a domain, the connection > upper bound is 6. We should double check with someone on the Necko team that this is indeed the case before landing this patch, IMO.
Flags: needinfo?(overholt)
Comment 12•7 years ago
|
||
(In reply to Evelyn Hung [:evelyn] from comment #10) > (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from > comment #8) > > Initiating the speculative connection centrally vs in the UI code should > > both have roughly the same latency, since the UI code usually directly calls > > addTab or loadURIWithFlags which is what call loadURIWithOptions. The > > difference between the two should be in the nano to microsecond range. > > No, in bug 1348275 and bug 1355451 we initiate the speculative connection > earlier. bug 1348275 did it before the user hits enter key on the location > bar, and bug 1355451 did on "mousedown" event, which is about hundred ms > earlier than mouseup event (and mouseup triggers loadURIWithOptions). > > Adding speculative connection code here is easy, and (I believe) necko will > properly control these connections(*), so I made this patch to catch more > cases that we currently didn't cover in UI code. > I tried to evaluate the before and after by adding profiler markers, but > failed to understand how much performance gain of this patch. Profiling > network performance is complicated. :( Andrei was working on the YouTube testing so maybe he can help. Maybe someone on the Necko team has thoughts (see comment 11)?
Flags: needinfo?(overholt)
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(afilip)
Comment 13•7 years ago
|
||
Nick--see comment 11. If we try to speculatively connect to the same origin a bunch of times in HTTP/1, how many actual connections do we open? re: performance testing. Yes, it is hard to test networking performance :) Ideally we'd test all the bits from the UI click to the channel being done loading. But as long as we trust speculative loading (do we have tests for it Nick?) even simply knowing that we've called the speculative API earlier is probably good enough here if it's hard to write a full test.
Flags: needinfo?(jduell.mcbugs) → needinfo?(hurley)
Comment 14•7 years ago
|
||
(In reply to Jason Duell [:jduell] (needinfo me) from comment #13) > Nick--see comment 11. If we try to speculatively connect to the same origin > a bunch of times in HTTP/1, how many actual connections do we open? We limit speculative connections to 6 per origin (see network.http.speculative-parallel-limit). Some addons appear to override this, though - uBlock origin appears to have reduced mine to 0. I should fix that :) > re: performance testing. Yes, it is hard to test networking performance :) > Ideally we'd test all the bits from the UI click to the channel being done > loading. But as long as we trust speculative loading (do we have tests for > it Nick?) even simply knowing that we've called the speculative API earlier > is probably good enough here if it's hard to write a full test. We do have functional tests for nsISpeculativeConnect - netwerk/test/unit/test_speculative_connect.js, and yes, just knowing that we've called the API is good enough - that's how we test the predictor, for example.
Flags: needinfo?(hurley)
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #11) > Perhaps someone who was running these YouTube tests can help. Andrew can > you please help find out who that person is? Evelyn, do you have a try > build for them to test? Yes, here it is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b69b248410e03424b418730e8fb5d11ad55e2aa2
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ehung
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8891965 [details] Bug 1383299 - Ensure we start to set up network connection before content process requests. https://reviewboard.mozilla.org/r/162988/#review169912 Thanks evelyn! Let's try this out. ::: toolkit/components/remotebrowserutils/RemoteWebNavigation.js:77 (Diff revision 1) > }, > loadURIWithOptions(aURI, aLoadFlags, aReferrer, aReferrerPolicy, > aPostData, aHeaders, aBaseURI, aTriggeringPrincipal) { > + // We know the url is going to be loaded, let's start requesting network > + // connection before the content process asks. > + // Note that we might have had setup the speculative connection in some Nit: "have had" -> "have already"
Attachment #8891965 -
Flags: review?(mconley) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•7 years ago
|
||
Thanks, Mike. I found a test failure which is likely caused by this patch. I'm investigating now. https://treeherder.mozilla.org/logviewer.html#?job_id=120904707&repo=try&lineNumber=2587
Assignee | ||
Comment 19•7 years ago
|
||
So the test case exposes a problem in my patch - the `aTriggeringPrincipal` isn't the correct setting for this speculative connection. What we want is `loadingPrincipal` or something equivalent that will eventually pass to necko when doing the page load. If I understand the code correctly, unfortunately at this moment in the chrome JS we don't know what principal will be used later; the docshell of the content tab will apply a set of complicated rules to set it correctly when asking uriloader to fetch the page. Hi :smaug, the intention of this bug is to initiate a speculative connection *before* sending `WebNavigation:LoadURI` IPC message to browser-child.js. We need to set the correct principal which matches the one that docshell will pass to nsURILoader, so that the connection will be used for that page loading. Do you have any suggestion?
Flags: needinfo?(bugs)
Comment 20•7 years ago
|
||
In this case the rules for the principal shouldn't be that complicated. Can't you use the same triggeringPrincipal as what "WebNavigation:LoadURI" uses? And if that is null, create a null principal with right origin attributes? ckerschb may have an opinion here.
Flags: needinfo?(bugs) → needinfo?(ckerschb)
Comment 21•7 years ago
|
||
(In reply to Evelyn Hung [:evelyn] from comment #19) > So the test case exposes a problem in my patch - the `aTriggeringPrincipal` > isn't the correct setting for this speculative connection. What we want is > `loadingPrincipal` or something equivalent that will eventually pass to > necko when doing the page load. Why is aTriggeringPrincipal not correct? In my opinion it should - I think I remember that aTriggeringPrincipal can be null there [1], is that potentially the problem? Out of curiosity: Why are we only doing that for http loads? Don't you also want to include https? [1] https://hg.mozilla.org/mozilla-central/annotate/a385d2425a76/toolkit/components/remotebrowserutils/RemoteWebNavigation.js#l87
Flags: needinfo?(ckerschb)
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #21) > (In reply to Evelyn Hung [:evelyn] from comment #19) > > So the test case exposes a problem in my patch - the `aTriggeringPrincipal` > > isn't the correct setting for this speculative connection. What we want is > > `loadingPrincipal` or something equivalent that will eventually pass to > > necko when doing the page load. > > Why is aTriggeringPrincipal not correct? In my opinion it should - I think I > remember that aTriggeringPrincipal can be null there [1], is that > potentially the problem? > If I understand it correctly, the semantic meaning of `aTriggeringPrincipal` is the principal of the *source* who triggers this URL loading, which might be different from the principal we use for loading this URL, especially on their OriginAttributes field. After tracing the code and play with a few real cases, here is my findings: 1. OriginAttributes is part of the key to lookup available connections in necko. Therefore we need to make sure the principal we use for setup speculative connection has the same origin attributes as we use for loading the url. 2. In the cases I tested (normal, private browsing and container modes), it seems they are the same. That makes me to think that I could just use `aTriggeringPrincipal`, but it might be a coincidence. I didn't see any logic in the code to enforce this. I'm not sure when and why the aTriggeringPrincipal is null, but in this case I plan to create an OriginAttributes object with userContextId and privateBrowsingId inferred from the current context. :ckerschb and :mconley, what do you think? btw, we can ignore the test case failure in comment 18. I found it didn't set a triggeringPrincipal which is easy to fix.
Flags: needinfo?(mconley)
Flags: needinfo?(ckerschb)
Comment 23•7 years ago
|
||
(In reply to Evelyn Hung [:evelyn] from comment #22) > If I understand it correctly, the semantic meaning of `aTriggeringPrincipal` > is the principal of the *source* who triggers this URL loading This was my understanding as well. > > I'm not sure when and why the aTriggeringPrincipal is null, but in this case > I plan to create an OriginAttributes object with userContextId and > privateBrowsingId inferred from the current context. > This sounds right to me, but let's wait for ckerschb's confirmation.
Flags: needinfo?(mconley)
Assignee | ||
Comment 24•7 years ago
|
||
(In reply to Evelyn Hung [:evelyn] from comment #22) > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #21) > > (In reply to Evelyn Hung [:evelyn] from comment #19) > > > So the test case exposes a problem in my patch - the `aTriggeringPrincipal` > > > isn't the correct setting for this speculative connection. What we want is > > > `loadingPrincipal` or something equivalent that will eventually pass to > > > necko when doing the page load. > > > > Why is aTriggeringPrincipal not correct? In my opinion it should - I think I > > remember that aTriggeringPrincipal can be null there [1], is that > > potentially the problem? > > > > If I understand it correctly, the semantic meaning of `aTriggeringPrincipal` > is the principal of the *source* who triggers this URL loading, which might > be different from the principal we use for loading this URL, especially on > their OriginAttributes field. After tracing the code and play with a few > real cases, here is my findings: > > 1. OriginAttributes is part of the key to lookup available connections in > necko. Therefore we need to make sure the principal we use for setup > speculative connection has the same origin attributes as we use for loading > the url. > > 2. In the cases I tested (normal, private browsing and container modes), it > seems they are the same. That makes me to think that I could just use > `aTriggeringPrincipal`, but it might be a coincidence. I didn't see any > logic in the code to enforce this. > I just found this bug 1348801 and the code which might be related: http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/browser/base/content/utilityOverlay.js#275-284 It looks to me that the rule is enforced?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 27•7 years ago
|
||
(In reply to Mike Conley (:mconley) - (PTO starting Aug 11 - Back Aug 17) from comment #23) > (In reply to Evelyn Hung [:evelyn] from comment #22) > > If I understand it correctly, the semantic meaning of `aTriggeringPrincipal` > > is the principal of the *source* who triggers this URL loading > > This was my understanding as well. That is correct. The triggeringPrincipal is the principal that initiates loading of a particular URI. > > > > I'm not sure when and why the aTriggeringPrincipal is null, but in this case > > I plan to create an OriginAttributes object with userContextId and > > privateBrowsingId inferred from the current context. > > > > This sounds right to me, but let's wait for ckerschb's confirmation. Within the loadInfo of a channel the triggeringPrincipal is *never* null, because we use some fallback mechanism within docshell to make sure the triggeringPricnipal is never null. In the code you are updating however the triggeringPrincipal might still be null (haven't passed through docshell yet). Ideally the triggeringPrincipal should be never null at that point, but I think we are not completely there yet - however, we are working on it - see Bug 1333030 for progress. To sum it up, your approach seems to provide the correct interim solution till Bug 1333030 landed and the triggeringPrincipal should never be null at that point in the code.
Flags: needinfo?(ckerschb)
Assignee | ||
Comment 28•7 years ago
|
||
Thank you for the explanation, :ckerschb. Mike, could you take a look of my latest patch and the fix of the test case? Thank you.
Flags: needinfo?(mconley)
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8896223 [details] Bug 1383299 - Fix test case error, https://reviewboard.mozilla.org/r/167520/#review175060 Seems okay, but maybe let's get jkt to review this too in case there's something I'm missing (I'll be honest - the triggeringPrincipal needing to hold onto a userContextId, and the addTab param also taking a userContextId seems a bit redundant - I assume one existing implies the other, no?) ::: browser/components/originattributes/test/browser/head.js:40 (Diff revision 1) > * > * @return tab - The tab object of this tab. > * browser - The browser object of this tab. > */ > async function openTabInUserContext(aURL, aUserContextId) { > + let originattributes = { Nit: `originattributes` -> `originAttributes`
Attachment #8896223 -
Flags: review?(mconley) → review+
Updated•7 years ago
|
Attachment #8896223 -
Flags: review?(jkt)
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8891965 [details] Bug 1383299 - Ensure we start to set up network connection before content process requests. https://reviewboard.mozilla.org/r/162988/#review175064 Still looks good to me! Thanks!
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8896223 [details] Bug 1383299 - Fix test case error, https://reviewboard.mozilla.org/r/167520/#review175072 > I'll be honest - the triggeringPrincipal needing to hold onto a userContextId, and the addTab param also taking a userContextId seems a bit redundant This code seems correct, :ckerschb actually wrote the paper on origin attributes and will understand this better than I do certainly in terms of how the principals get passed betweem document loads. My understanding is that the tab needs the origin attribute for subsequent tab loads and the UI itself and isn't implied from the principal. Docshell also doesn't touch the tab either. However :baku and :ckerschb could answer this question much better. It looks like you are loading the correct userContextId (I didn't test the code itself) however if you want a better answer for :mconley's question add :baku to the mix.
Attachment #8896223 -
Flags: review?(jkt) → review+
Assignee | ||
Comment 32•7 years ago
|
||
Thanks, Mike and jkt. Yeah, addTab() of tabbrowser.xml takes aUserContextId explicitly given by its caller. In the case it's not given, it will be inferred from the mCurrentTab but not from triggeringPrincipal [1]. That sounds to me that they don't have to be consistent, although in practice they seems to be always consistent. I'd like to not change this design (if this is by design) in my patch because it seems irrelevant to my topic. [1] http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/browser/base/content/tabbrowser.xml#2478-2481
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 35•7 years ago
|
||
Pushed by ehung@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/871476099df7 Ensure we start to set up network connection before content process requests. r=mconley https://hg.mozilla.org/integration/autoland/rev/0ed17c747e28 Fix test case error, r=jkt,mconley
I had to back these out for xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=124172760&repo=autoland https://hg.mozilla.org/integration/autoland/rev/0db3beef788d https://hg.mozilla.org/integration/autoland/rev/94dbd3b4e5e5
Flags: needinfo?(ehung)
Assignee | ||
Comment 38•7 years ago
|
||
Error call stacks: 09:37:05 INFO - PID 6684 | JavaScript error: resource://gre/modules/PrivateBrowsingUtils.jsm, line 49: TypeError: aWindow is null 09:37:05 INFO - Unexpected exception NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "aWindow is null" {file: "resource://gre/modules/PrivateBrowsingUtils.jsm" line: 49}]'[JavaScript Error: "aWindow is null" {file: "resource://gre/modules/PrivateBrowsingUtils.jsm" line: 49}]' when calling method: [nsIWebNavigation::loadURIWithOptions] 09:37:05 INFO - loadURIWithFlags/<@chrome://global/content/bindings/browser.xml:150:15 09:37:05 INFO - _wrapURIChangeCall@chrome://global/content/bindings/browser.xml:48:15 09:37:05 INFO - loadURIWithFlags@chrome://global/content/bindings/browser.xml:149:13 09:37:05 INFO - loadURI/<@chrome://global/content/bindings/browser.xml:114:15 09:37:05 INFO - _wrapURIChangeCall@chrome://global/content/bindings/browser.xml:48:15 09:37:05 INFO - loadURI@chrome://global/content/bindings/browser.xml:113:13 09:37:05 INFO - loadURL@resource://testing-common/ExtensionXPCShellUtils.jsm:137:5 09:37:05 INFO - async*loadContentPage@resource://testing-common/ExtensionXPCShellUtils.jsm:682:12 09:37:05 INFO - test_contentscript_xrays@/Users/cltbld/tasks/task_1503071262/build/tests/xpcshell/tests/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_xrays.js:69:27 Hmm... I didn't know that there are xpcshell tests calling `loadURI`. These tests run with windowless browser. The triggeringPrincipal seems to be missing, so that when we tried to create OA and detected if the browser was in private mode, it failed to get `aBrowser.contentWindow`. http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/toolkit/modules/PrivateBrowsingUtils.jsm#45 I would like to fix this test fail by modifying PrivateBrowsingUtils.jsm, and ask for another review.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 42•7 years ago
|
||
(In reply to Evelyn Hung [:evelyn] from comment #39) > Created attachment 8899676 [details] > Bug 1383299 - Fix xpcshell test failures, > > Some xpcshell tests run with _windowless_ browser and calls its LoadURI(). > It then failed to get `aBrowser.contentWindow` when we tried to create OA > and detected if the browser was in the private mode. > > Review commit: https://reviewboard.mozilla.org/r/170978/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/170978/ Hi Dao, I see you reviewed related code in PrivateBrowsingUtils.jsm before. This patch is just a one-line fix to xpcshell test failures which run in _windowless_ browser. I thought `!aBrowser.isConnected` might imply `!aBrowser.contentWindow` because the comments said so, but I'm not sure if this is always true.
Flags: needinfo?(ehung)
Assignee | ||
Comment 43•7 years ago
|
||
(In reply to Evelyn Hung [:evelyn] from comment #42) > ... I thought `!aBrowser.isConnected` might imply > `!aBrowser.contentWindow` because the comments said so, but I'm not sure if > this is always true. I mean `!aBrowser.isConnected` can be implied from `!aBrowser.contentWindow` so we could just check `!aBrowser.contentWindow`?
Assignee | ||
Comment 44•7 years ago
|
||
try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99e05c9adae4a0e2208cbbb77559a2336df0daab
Comment 45•7 years ago
|
||
mozreview-review |
Comment on attachment 8899676 [details] Bug 1383299 - Fix xpcshell test failures, https://reviewboard.mozilla.org/r/170978/#review176298 ::: toolkit/modules/PrivateBrowsingUtils.jsm:38 (Diff revision 1) > return this.privacyContextFromWindow(aWindow).usePrivateBrowsing; > }, > > isBrowserPrivate(aBrowser) { > let chromeWin = aBrowser.ownerGlobal; > - if (chromeWin.gMultiProcessBrowser || !aBrowser.isConnected) { > + if (chromeWin.gMultiProcessBrowser || !aBrowser.isConnected || !aBrowser.contentWindow) { I think you can get rid of the !aBrowser.isConnected check now. ::: toolkit/modules/PrivateBrowsingUtils.jsm:42 (Diff revision 1) > let chromeWin = aBrowser.ownerGlobal; > - if (chromeWin.gMultiProcessBrowser || !aBrowser.isConnected) { > + if (chromeWin.gMultiProcessBrowser || !aBrowser.isConnected || !aBrowser.contentWindow) { > // In e10s we have to look at the chrome window's private > // browsing status since the only alternative is to check the > // content window, which is in another process. If the browser > // is lazy then the content window doesn't exist. Please update this comment.
Comment 46•7 years ago
|
||
Is there anyone else who can take a look here (see comment 12)?
Flags: needinfo?(cchiorean)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 48•7 years ago
|
||
Hi Dao, I updated the patch and addressed your comments. Please take a look, thanks!
Flags: needinfo?(dao+bmo)
Comment 49•7 years ago
|
||
mozreview-review |
Comment on attachment 8899676 [details] Bug 1383299 - Fix xpcshell test failures, https://reviewboard.mozilla.org/r/170978/#review176728
Attachment #8899676 -
Flags: review?(dao+bmo) → review+
Comment 50•7 years ago
|
||
You don't need to add needinfo requests for review requests.
Flags: needinfo?(dao+bmo)
Comment 51•7 years ago
|
||
Pushed by ehung@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5790215bddcc Ensure we start to set up network connection before content process requests. r=mconley https://hg.mozilla.org/integration/autoland/rev/244e2a482db2 Fix test case error, r=jkt,mconley https://hg.mozilla.org/integration/autoland/rev/780cc8989f5d Fix xpcshell test failures, r=dao
Comment 52•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5790215bddcc https://hg.mozilla.org/mozilla-central/rev/244e2a482db2 https://hg.mozilla.org/mozilla-central/rev/780cc8989f5d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 53•7 years ago
|
||
We run a benchmark for two builds ,one with the improvements related to the speculative connection Bug and another one for an older branch, 21 August nightly.I didn't changed the parallel connection default value(network.http.speculative-parallel-limit;6) and for getting the response time I used the graphic capture card to get the number of frames and in the end I converted them in milliseconds. https://docs.google.com/spreadsheets/d/1CyVIWlTQGoJtnKBwc_0pw3vLQA3SUE3gXh7p30wZywA/edit#gid=0 - the link to the benchmark results and the speculative connection updates can be found in Build:20170824100243 against the older build Build:20170821100350
Flags: needinfo?(cchiorean)
Flags: needinfo?(afilip)
Comment 54•7 years ago
|
||
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•