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)
Tracking
()
People
(Reporter: ntim, Assigned: valentin)
References
Details
(Keywords: regression, Whiteboard: [multiviewport] [reserve-rdm] [necko-active])
Attachments
(1 file)
1.22 KB,
patch
|
mcmanus
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•8 years ago
|
||
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
Reporter | ||
Updated•8 years ago
|
Keywords: regression,
regressionwindow-wanted
Reporter | ||
Updated•8 years ago
|
status-firefox50:
--- → unaffected
status-firefox51:
--- → affected
Whiteboard: [multiviewport][triage]
Assignee | ||
Comment 2•8 years ago
|
||
Could be due to bug 1262326. Investigating.
Assignee: nobody → valentin.gosu
Whiteboard: [multiviewport][triage] → [multiviewport][triage][necko-active]
Assignee | ||
Comment 3•8 years ago
|
||
Actually, that landed in 49. If 50 is indeed unaffected, that means the cause is different, and we need a regression-window.
Reporter | ||
Comment 4•8 years ago
|
||
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
Reporter | ||
Comment 5•8 years ago
|
||
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.
Updated•8 years ago
|
tracking-e10s:
--- → ?
Updated•8 years ago
|
Updated•8 years ago
|
Priority: -- → P3
Whiteboard: [multiviewport][triage][necko-active] → [multiviewport] [reserve-rdm] [necko-active]
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 6•8 years ago
|
||
Valentin, do you still need a regressing bug?
Flags: needinfo?(valentin.gosu)
Reporter | ||
Comment 7•8 years ago
|
||
(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.
Assignee | ||
Comment 8•8 years ago
|
||
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)
Comment 9•8 years ago
|
||
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.
Reporter | ||
Comment 11•8 years ago
|
||
This affects existing RDM which uses the docshell override as well. The feature was implemented in 47 as jryans mentions.
Assignee | ||
Comment 12•8 years ago
|
||
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)
Reporter | ||
Comment 13•8 years ago
|
||
(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)
Comment 14•8 years ago
|
||
(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!
Assignee | ||
Comment 15•8 years ago
|
||
> 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)
Assignee | ||
Comment 16•8 years ago
|
||
MozReview-Commit-ID: 3rmoAfpvml0
Attachment #8800354 -
Flags: review?(mcmanus)
Comment 17•8 years ago
|
||
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+
Assignee | ||
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9de739c57c87fbfcb47f9c36523d750277c12c94 Bug 1302400 - Set docshell UA override for redirected channels in the child r=mcmanus
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9de739c57c87
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Comment 20•8 years ago
|
||
Tim, let me know if this fixes the problem for you and if I should nominate it for uplifting. Thanks!
Flags: needinfo?(ntim.bugs)
Updated•8 years ago
|
Iteration: --- → 52.2 - Oct 17
Priority: P3 → P1
Reporter | ||
Comment 21•8 years ago
|
||
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.
Reporter | ||
Comment 22•8 years ago
|
||
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+
Comment 24•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/32b9ee582156
Comment 25•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/7685b4f9ab3f
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•