Closed Bug 1302400 Opened 8 years ago Closed 8 years ago

[e10s] DocShell user agent override is no longer set on page navigation

Categories

(Core :: Networking: HTTP, defect, P1)

49 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla52
Iteration:
52.2 - Oct 17
Tracking Status
e10s + ---
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- verified

People

(Reporter: ntim, Assigned: valentin)

References

Details

(Keywords: regression, Whiteboard: [multiviewport] [reserve-rdm] [necko-active])

Attachments

(1 file)

STR:
- Open new RDM
- Select a device with custom UA
- Navigate to a website

AR:
- Network requests still have normal UA (therefore the layout of most websites is rendered as if it were a desktop site)

ER:
- Network requests should have custom UA
A few notes:
- navigator.userAgent is still overriden properly
- It's overriden on reload


Also happens in old RDM
Component: Developer Tools: Responsive Design Mode → Networking: HTTP
Product: Firefox → Core
Summary: Navigating to a website in new RDM with device enabled doesn't change user agent for network requests → DocShell user agent override is no longer set on page navigation
Whiteboard: [multiviewport][triage]
Could be due to bug 1262326. Investigating.
Assignee: nobody → valentin.gosu
Whiteboard: [multiviewport][triage] → [multiviewport][triage][necko-active]
Actually, that landed in 49. If 50 is indeed unaffected, that means the cause is different, and we need a regression-window.
Not happening with e10s disabled (I had e10s disabled on my Firefox 50). Reproducible on 50 with e10s enabled.
Summary: DocShell user agent override is no longer set on page navigation → [e10s] DocShell user agent override is no longer set on page navigation
bug 1262326 looks like a good candidate, as it's a change in 50 and 51, that affects e10s only. Still need to test in 49/48.
tracking-e10s: --- → ?
Priority: -- → P3
Whiteboard: [multiviewport][triage][necko-active] → [multiviewport] [reserve-rdm] [necko-active]
Status: NEW → ASSIGNED
Valentin, do you still need a regressing bug?
Flags: needinfo?(valentin.gosu)
(In reply to Andrew Overholt [:overholt] from comment #6)
> Valentin, do you still need a regressing bug?

This is a regressing bug, it originally worked when I implemented the feature in bug 1137681.
I have managed to reproduce it a couple of times. Indeed, reloading the page makes it work.
I'll probably have a fix for this next week. I think we'll also need some unit tests to make sure we don't regress in the future.
Flags: needinfo?(valentin.gosu)
Valentin, Jim said you're busy with another top crasher. Should we try to find someone else?

Relatedly, if RDM isn't shipping until 51, should we punt on fixing this regression until then?
Flags: needinfo?(valentin.gosu)
(In reply to Andrew Overholt [:overholt] from comment #9)
> Relatedly, if RDM isn't shipping until 51, should we punt on fixing this
> regression until then?

There is an ongoing RDM rewrite project that is currently expected to ship in 52, which I believe is the effort you're mentioning here.

However, I don't think this is relevant for the specific feature of UA overrides, because that is also implemented in the existing RDM tool we ship today.  This UA override feature in RDM first shipped to users in bug 828008 as part of Firefox 47.
This affects existing RDM which uses the docshell override as well. The feature was implemented in 47 as jryans mentions.
I haven't had time to track down the root cause of this issue yet, with 2 other top crashers, and a necko mini-work-week.
It's quite odd that the UA override doesn't work on the first load, but it does work if you reload and navigate to other pages. Maybe it has something to do with the devtools console disappearing when you first load a web page? Or maybe not.
Or maybe there is a race when importing and calling UserAgentOverrides.init(); in the main process.
Anyway, if someone is able to figure out what the main issue is, I'd love to help with the fix.
Flags: needinfo?(valentin.gosu)
(In reply to Valentin Gosu [:valentin] from comment #12)
> I haven't had time to track down the root cause of this issue yet, with 2
> other top crashers, and a necko mini-work-week.
> It's quite odd that the UA override doesn't work on the first load, but it
> does work if you reload and navigate to other pages.
Note that if you navigate to other pages with the URL bar, you can reproduce the issue. Whereas if you navigate by clicking a link, the UA override works fine.

The weirdest thing is that if you check the network monitor (in the case where it fails), the docshell override is ignored for the initial HTML page request, but the following requests (images, scripts) use the docshell override. 

> Maybe it has something to do with the devtools console disappearing when you first load a web page?
> Or maybe not.
The DevTools toolbox disappears when the tab goes from e10s to non-e10s or vice-versa. I don't think this is related, since the issue can be reproduced without having the toolbox opened (and only the RDM).

> Or maybe there is a race when importing and calling
> UserAgentOverrides.init(); in the main process.
So UserAgentOverrides.init(); is never called on Firefox Desktop, simply because UserAgentOverrides.jsm is unused on Desktop Firefox (see bug 896114), so I think it's safe to rule that out. Anyway, UserAgentOverrides doesn't take care of the DocShell override, but this code does: https://dxr.mozilla.org/mozilla-central/search?q=SetDocshellUserAgentOverride&redirect=false .

> Anyway, if someone is able to figure out what the main issue is, I'd love to
> help with the fix.
I've replaced the LOG functions with printf in nsHttpChannel::AsyncOpen and HttpChannelChild::AsyncOpen and I've noticed that AsyncOpen doesn't get called on the requests where the docshell override is ignored.
That's the main issue (since only AsyncOpen calls SetDocshellUserAgentOverride). Although I have no idea why AsyncOpen would not be called in that specific case.

Does this help?
Flags: needinfo?(valentin.gosu)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> (In reply to Andrew Overholt [:overholt] from comment #9)
> > Relatedly, if RDM isn't shipping until 51, should we punt on fixing this
> > regression until then?
> 
> There is an ongoing RDM rewrite project that is currently expected to ship
> in 52, which I believe is the effort you're mentioning here.
> 
> However, I don't think this is relevant for the specific feature of UA
> overrides, because that is also implemented in the existing RDM tool we ship
> today.  This UA override feature in RDM first shipped to users in bug 828008
> as part of Firefox 47.

Thanks for the clarification!
> Does this help?

Actually, it did. So, we do call AsyncOpen all the time, except for redirected channels.
So I can reproduce the bug if I go to test.com - It loads http://test.com with the correct UA, then the redirect loads https://test.com with the wrong UA. If I go directly to https://test.com there's no problem.

If this is the only case where we fail, the fix is quite simple, and it could even be uplifted.
Flags: needinfo?(valentin.gosu)
MozReview-Commit-ID: 3rmoAfpvml0
Attachment #8800354 - Flags: review?(mcmanus)
Comment on attachment 8800354 [details] [diff] [review]
Set docshell UA override for redirected channels in the child

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

:)
Attachment #8800354 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9de739c57c87fbfcb47f9c36523d750277c12c94
Bug 1302400 - Set docshell UA override for redirected channels in the child r=mcmanus
https://hg.mozilla.org/mozilla-central/rev/9de739c57c87
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Tim, let me know if this fixes the problem for you and if I should nominate it for uplifting. Thanks!
Flags: needinfo?(ntim.bugs)
Iteration: --- → 52.2 - Oct 17
Priority: P3 → P1
Verified fixe(In reply to Valentin Gosu [:valentin] from comment #20)
> Tim, let me know if this fixes the problem for you and if I should nominate
> it for uplifting. Thanks!

The patch fixes the issue, thanks!
Going to nominate it for uplift.
Status: RESOLVED → VERIFIED
Flags: needinfo?(ntim.bugs)
Comment on attachment 8800354 [details] [diff] [review]
Set docshell UA override for redirected channels in the child

Approval Request Comment
[Feature/regressing bug #]: regression of a bug (not sure which one)
[User impact if declined]: broken user agent emulation feature (introduced in 47) on e10s
[Describe test coverage new/current, TreeHerder]: Fixed in Nightly
[Risks and why]: Low, one line change.
[String/UUID change made/needed]: no.
Attachment #8800354 - Flags: approval-mozilla-beta?
Attachment #8800354 - Flags: approval-mozilla-aurora?
Comment on attachment 8800354 [details] [diff] [review]
Set docshell UA override for redirected channels in the child

Fix was verified on Nightly, Aurora51+, Beta50+
Attachment #8800354 - Flags: approval-mozilla-beta?
Attachment #8800354 - Flags: approval-mozilla-beta+
Attachment #8800354 - Flags: approval-mozilla-aurora?
Attachment #8800354 - Flags: approval-mozilla-aurora+
Blocks: 1262326
Version: unspecified → 49 Branch
You need to log in before you can comment on or make changes to this bug.