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)

Product:
Firefox
Component:
Address Bar
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: jj.evelyn)

References

Details

Attachments

(4 files)

Bug 1383299 - Ensure we start to set up network connection before content process requests.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1383299 - Fix test case error,
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1383299 - Fix xpcshell test failures,
59 bytes, text/x-review-board-request
dao
: review+
Details
the benchmark results for the speculative connection updates
5.64 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet
Details 
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.
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.
Should this be a blocker for bug 1363772?
Evelyn, This is now QRC. Do you think if it's something you can do for fx57?
Whiteboard: [qf] → [qf:p1]
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)
(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)
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)
(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
(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)
(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? :)
Attachment #8891965 - Flags: review?(mconley)
(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)
(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)
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)
(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)
(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: nobody → ehung
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+
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
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)
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)
(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)
(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)
(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)
(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?
(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)
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 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+
Attachment #8896223 - Flags: review?(jkt)
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 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+
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
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
Thanks evelyn! Clearing ni?.
Flags: needinfo?(mconley)
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.
(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)
(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`?
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.
Is there anyone else who can take a look here (see comment 12)?
Flags: needinfo?(cchiorean)
Hi Dao, I updated the patch and addressed your comments. Please take a look, thanks!
Flags: needinfo?(dao+bmo)
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+
You don't need to add needinfo requests for review requests.
Flags: needinfo?(dao+bmo)
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
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)
Depends on: 1397548
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: