Make test_user_agent_overrides.html work in e10s

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: valentin.gosu, Assigned: valentin.gosu)

Tracking

(Blocks 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox46 wontfix, firefox47 affected, firefox48 affected, firefox49 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 3 obsolete attachments)

No description provided.
Attachment #8738348 - Flags: feedback?(hurley)
Comment on attachment 8738348 [details] [diff] [review]
Make test_user_agent_overrides.html work in e10s

Review of attachment 8738348 [details] [diff] [review]:
-----------------------------------------------------------------

Does this seem like the correct approach?
Or is there a better way of making this work in the child?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ -5306,5 @@
>      // notify "http-on-modify-request" observers
>      CallOnModifyRequestObservers();
>  
> -    // If mLoadGroup is null, e10s is enabled and this will be handled by HttpChannelChild.
> -    if (mLoadGroup) {

From what I can tell from the code and the test failing, UserAgentOverrides.jsm is never loaded in the child process, so SetLoadGroupUserAgentOverride doesn't do the right thing on the child.
So this needs to run in the parent, in order to set the proper header on outgoing requests. navigator.userAgent works because it does a sync RPC to the parent, and caches the result in SiteSpecificUserAgent.
(In reply to Valentin Gosu [:valentin] from comment #3)
> Comment on attachment 8738348 [details] [diff] [review]
> Make test_user_agent_overrides.html work in e10s
> 
> Review of attachment 8738348 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does this seem like the correct approach?
> Or is there a better way of making this work in the child?
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ -5306,5 @@
> >      // notify "http-on-modify-request" observers
> >      CallOnModifyRequestObservers();
> >  
> > -    // If mLoadGroup is null, e10s is enabled and this will be handled by HttpChannelChild.
> > -    if (mLoadGroup) {
> 
> From what I can tell from the code and the test failing,
> UserAgentOverrides.jsm is never loaded in the child process, so
> SetLoadGroupUserAgentOverride doesn't do the right thing on the child.
> So this needs to run in the parent, in order to set the proper header on
> outgoing requests. navigator.userAgent works because it does a sync RPC to
> the parent, and caches the result in SiteSpecificUserAgent.

Hrm... so I think we probably need to figure out how (if possible) to get UserAgentOverrides.jsm loaded in the child. As it stands, the change you're making will basically undo all the work done in bug 1148544 for the browser running in e10s, quite possibly causing a perf regression. The UA override caching on the loadgroup done in that bug won't work if we always do the lookup on the parent, since the parent doesn't have a loadgroup.

Alternately, we may need to revisit the decisions made over there and go with the second option I listed in bug 1148544 comment 11 (or something else).
Attachment #8738348 - Flags: feedback?(hurley)
The thing is that the way UserAgentOverrides.jsm is written, it doesn't work on the child at all. 
I'll try to see if I can use SchedulingContext as you suggested.
Ah, that is indeed problematic. Wish we'd known that when working on bug 1148544! Guess this is turning into a larger project, then (though it's good that it's being done, since it seems like UA overrides won't work in e10s at all right now)
Blocks: e10s-tests
tracking-e10s: --- → +
Moved SetLoadGroupUserAgentOverride to nsHttpChannel. And using nsISchedulingContext.userAgent to cache the user agent value, but it looks like there will still be a performance impact here. We could land this if it's really needed in e10s, but otherwise I think we need to rewrite the jsm with IPDL.
Attachment #8743339 - Flags: feedback?(hurley)
Comment on attachment 8743339 [details] [diff] [review]
Make test_user_agent_overrides.html work in e10s

Review of attachment 8743339 [details] [diff] [review]:
-----------------------------------------------------------------

So in terms of using the scheduling context, this seems reasonable. The only other thing I might consider is renaming scheduling context (to request context, perhaps?), as well as the associated service - it's no longer just for scheduling if we go this way. More busy work, I know, but I prefer that to having something that's confusingly named :)

More concerning, though, what's the source of the perf impact you're expecting from this tactic? Do we have any numbers indicating the size of the impact?

Oh, and lastly, in the commit message, the 'w' comes between the 'n' and the 'g', not at the end ;)

::: netwerk/base/SchedulingContextService.cpp
@@ +33,5 @@
>    nsID mID;
>    char mCID[NSID_LENGTH];
>    Atomic<uint32_t>       mBlockingTransactionCount;
>    nsAutoPtr<SpdyPushCache> mSpdyCache;
> +  nsCString mUserAgent;

Call this mUserAgentOverride (and change associated method names appropriately, as well)
Attachment #8743339 - Flags: feedback?(hurley)
MozReview-Commit-ID: FQS9uSRR8kd
Attachment #8743874 - Flags: review?(hurley)
Attachment #8743339 - Attachment is obsolete: true
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #10)
> More concerning, though, what's the source of the perf impact you're
> expecting from this tactic? Do we have any numbers indicating the size of
> the impact?

I mistakenly thought that there would be more than one listener of "http-on-useragent-request".
I checked, and the same number of calls to UserAgentOverrides.jsm are made, so we should be fine.
If we want to optimize it any further, I guess we could rewrite it in C++.
Comment on attachment 8743874 [details] [diff] [review]
Make test_user_agent_overrides.html work in e10s

Review of attachment 8743874 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/RequestContextService.h
@@ +21,1 @@
>                                       , public nsIObserver

nit: fix indentation changed by rename

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +683,5 @@
>      }
>  
> +    nsCOMPtr<nsIRequestContext> rc;
> +    nsresult rv = rcsvc->GetRequestContext(mRequestContextID,
> +                                              getter_AddRefs(rc));

nit: fix indentation
Attachment #8743874 - Flags: review?(hurley) → review+
MozReview-Commit-ID: FQS9uSRR8kd
Attachment #8743874 - Attachment is obsolete: true
Attachment #8738348 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ffe9793118b7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1269994
Depends on: 1302400
You need to log in before you can comment on or make changes to this bug.