Closed Bug 1254025 Opened 7 years ago Closed 7 years ago

Source editor is blank when user agent override is applied

Categories

(DevTools :: Source Editor, defect, P2)

Product:
DevTools
Component:
Source Editor
Version:
44 Branch
Type:
defect

Tracking

(firefox46 wontfix, firefox47 fixed, firefox48 fixed, firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: karlcow, Assigned: bgrins)

References

(
URL
)

Details

(Keywords: regression, regressionwindow-wanted)

Attachments

(2 files)

controller.patch
1.51 KB, patch
Details | Diff | Splinter Review
MozReview Request: Bug 1254025 - Force the CodeMirror editor to always use a textarea;r=jdescottes
58 bytes, text/x-review-board-request
jdescottes
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details 
1. Open a clean profile of Firefox (I can reproduce with Firefox Nightly 47.0a1 (2016-03-06), Aurora 46.0a2 (2016-03-06) and the Production release 44.0.2)

2. With User-Agent Switcher Addon, change the user agent to be 
  "Mozilla/5.0 (Android 4.4.4; Mobile; rv:44.0) Gecko/44.0 Firefox/44.0"

3. Activate responsive layout mode.

4. Access http://blog.ethanvizitei.com/2008/06/using-ruby-for-imap-with-gmail.html?m=1

5. Open the Developer tools, switch to Debugger.

Expected:
Having the list of scripts.

Actual:
nothing


Note:
When opening first with the normal user agent, going to the debugger, loading the scripts, then switching the UA string and reloading, I could get the scripts.
This is basic behavior that should just work. Marking this as P1, assuming it will be confirmed.
Priority: -- → P1
Are the scripts also missing if you use the new "Custom User Agent" input inside responsive design instead of the add-on?
Flags: needinfo?(kdubost)
OK doing that:

1. Open a clean profile of Firefox 48.0a1 (2016-03-11)

2. Activate responsive layout mode.

3. Enter "Mozilla/5.0 (Android 4.4.4; Mobile; rv:44.0) Gecko/44.0 Firefox/44.0"
   in the custom User Agent Field.

4. Access http://blog.ethanvizitei.com/2008/06/using-ruby-for-imap-with-gmail.html?m=1

5. Open the Developer tools, switch to Debugger.

Scripts are here.

6. Go to the network panel. Reload. Copy Request Headers

GET /csi?v=3&s=blogger&action=item_blogspot&it=wtsrt_.0,tbsd_.4784,tbnd_.-4784&blogId=294680397865315179&e=templatesV2&rt=headEnd.5204,widgetJsBefore.5271,widgetJsStart.5279,widgetJsEnd.5323,prt.5825,ol.6024 HTTP/1.1
Host: csi.gstatic.com
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0
Accept: */*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Referer: http://blog.ethanvizitei.com/2008/06/using-ruby-for-imap-with-gmail.html?m=1
Connection: keep-alive


Ah! Custom User Agent didn't work.


Trying again, this time with  Nightly 48.0a1 (2016-03-13)

Scripts OK.
Request Headers OK.

GET /2008/06/using-ruby-for-imap-with-gmail.html?m=1 HTTP/1.1
Host: blog.ethanvizitei.com
User-Agent: Mozilla/5.0 (Android 4.4.4; Mobile; rv:44.0) Gecko/44.0 Firefox/44.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Connection: keep-alive


Good.


Let me try to reproduce my own bug. YEAH! Still happening, aka no scripts. We are reducing the parameters.

Good news. Custom User Agent Field helps to have a better behavior.
Bad news. Bad interaction in between the debugger and "User-Agent Switcher Addon"

http://chrispederick.com/work/user-agent-switcher/
https://github.com/chrispederick/user-agent-switcher/
https://addons.mozilla.org/en-US/firefox/addon/user-agent-switcher/

Issues with e10s? or something else.
Flags: needinfo?(kdubost)
I cannot reproduce this. If the addon is causing the problem, it's likely a bug with the addon, but I can't be sure. It's hard to tell without being able to reproduce.
Priority: P1 → P2
I am able to reproduce in OSX and Windows 10.

45 - Sources appear but no contents of the file displayed, clicking between them does nothing
46 & 48 - No sources appear
Seeing something similar with spoofing enabled while working on bug 1228971 right now. Browser console contains this: 

TypeError: cm is undefined

which has been reported lots of times per the count. cm is supposed to be a CodeMirror instance. There's some browser sniffing (using navigator.userAgent) in the CodeMirror library itself..?
(In reply to Hallvord R. M. Steen [:hallvors] from comment #6)
> Seeing something similar with spoofing enabled while working on bug 1228971
> right now. Browser console contains this: 
> 
> TypeError: cm is undefined
> 
> which has been reported lots of times per the count. cm is supposed to be a
> CodeMirror instance. There's some browser sniffing (using
> navigator.userAgent) in the CodeMirror library itself..?

I was seeing that error too on a dev profile and could never figure out what it was, I thought it must have to do with an addon.  It was preventing me from doing 'Edit as HTML' in the inspector (bug 1263160), I'm curious if you are having that problem too.
Confirming the "edit as HTML" problem.
(In reply to Hallvord R. M. Steen [:hallvors] from comment #8)
> Confirming the "edit as HTML" problem.

Do you know what steps you took to get into this state?  Have you switched the UA via the addon in Comment 0, or from Responsive Mode?  I didn't have the addon installed, and the problem for Bug 1263160 persisted between restarts.  I can't repro this or the other bug on my new profile using the Responsive Mode UA switcher (trying that since I never had that addon installed on the other profile).
Flags: needinfo?(hsteen)
See Also: → 1263160
I simply do this:
1) Start nightly
2) Pick a user-agent setting in the "Tools"-submenu for User Agent Switcher.
3) Load a site, open devtools while site is loading (not sure if timing matters)
4) It fails..

(Aside: I also have the RDP packed inspector installed in this profile, but not enabled)

(Another aside: I've tried to use the Browser Toolbox for debugging here, but
Flags: needinfo?(hsteen)
..trying to step through JS with the browser toolbox keeps throwing "too much recursion" errors. Right now I'm not going to dive in to debug the tool I need to use to debug the tool I need to use to debug the problem I'm meant to be looking at..)

let cm = editors.get(this) 

returns undefined, hence the script dies with the exception mentioned earlier.
That likely means the editors.set() call hasn't run. Just before the set() call, we have this line:

 cm.getInputField().controllers.insertControllerAt(0, controller(this));

I'm seeing an exception because cm.getInputField().controllers doesn't exist.. Smoking gun?
(In reply to Hallvord R. M. Steen [:hallvors] from comment #11)
> I'm seeing an exception because cm.getInputField().controllers doesn't
> exist.. Smoking gun?

Yeah, that looks like it.  OK I can repro this using "Mozilla/5.0 (Android 4.4.4; Mobile; rv:44.0) Gecko/44.0 Firefox/44.0" and http://blog.ethanvizitei.com/2008/06/using-ruby-for-imap-with-gmail.html?m=1.  So it's not a CodeMirror / UA thing at all, it's a 'controllers' / XUL thing.

One one hand I'd say let's just wrap that call in a condition, since worst case scenario the context menus will be partially broken for the editors, but on the other it seems to be fixing a symptom and not the actual problem.  However, broken context menus is *way* better than a broken debugger so I'll upload a patch for testing.
Attached patch controller.patch — — Splinter Review 
Hi Karl, can you confirm this patch makes the debugger work again?

Here's an ongoing try push that will have binaries: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8b25b13ad6e

I don't have time right now to do anything better than this, ideally we could fix this at the root of the issue though.
Flags: needinfo?(kdubost)
Brian,

* Test with the 48.0a1 (2016-04-16) Nightly. Reproducing the issue.
* Test with your try build. Issue is fixed.

Thanks! Whatever you did. It's working.
Flags: needinfo?(kdubost) → needinfo?(bgrinstead)
I tested w/o E10s enabled, still an issue.
(To clarify: not testing the binaries with Brian's workaround now!)

I've tested in Firefox 36.0 and 40.0 where it works fine. So there's some regression between the 40.0 and the 44 release Karl tested.
Keywords: regressionwindow-wanted
Comment on attachment 8741974 [details] [diff] [review]
controller.patch

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

James, what do you think about this?  As I said in Comment 12 this is only fixing a symptom of some problem caused by a custom UA, but it at least makes the debugger work again.
Attachment #8741974 - Flags: review?(jlong)
Flags: needinfo?(bgrinstead)
Investigated the same issue related to the StyleEditor for Bug 1250027 (potential duplicate).

Brian: You say this is not a CodeMirror/UA issue, but IMO the root cause is that the iframe used to load codemirror is using the custom UA. It makes CM switch from a textarea to contenteditable, and I guess contenteditable nodes do not support controllers. 

Is there a way to create the codemirror iframe without using the custom UA ? The devtools code should not be impacted by custom UA from addons. For information this addon is modifying the UA by setting "general.useragent.override".
Flags: needinfo?(bgrinstead)
See Also: → 1250027
(In reply to Julian Descottes [:jdescottes] from comment #18)
> Investigated the same issue related to the StyleEditor for Bug 1250027
> (potential duplicate).
> 
> Brian: You say this is not a CodeMirror/UA issue, but IMO the root cause is
> that the iframe used to load codemirror is using the custom UA. It makes CM
> switch from a textarea to contenteditable, and I guess contenteditable nodes
> do not support controllers. 
> 
> Is there a way to create the codemirror iframe without using the custom UA ?
> The devtools code should not be impacted by custom UA from addons. For
> information this addon is modifying the UA by setting
> "general.useragent.override".

Oh, I just hadn't figured that out - sounds like that's the actual problem!  We can use the inputStyle option to force it to always be a textarea: http://codemirror.net/doc/manual.html#option_inputStyle
Flags: needinfo?(bgrinstead)
Attachment #8741974 - Flags: review?(jlong)
By default it switches to a contentEditable when a custom UA is applied, which
causes errors when trying to insert a controller.

Review commit: https://reviewboard.mozilla.org/r/48999/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48999/
Attachment #8745353 - Flags: review?(jdescottes)
Julian, can you confirm this fixes the problem?
Comment on attachment 8745353 [details]
MozReview Request: Bug 1254025 - Force the CodeMirror editor to always use a textarea;r=jdescottes

https://reviewboard.mozilla.org/r/48999/#review45817

Thanks Brian! 

I can confirm this fixes the issues from both bug 1254025 and bug 1250027. 

(still feels wrong that a devtools iframe can be impacted by a custom UA set by an add-on. I expect fixing this is way more complicated though, so totally fine with shipping this now)
Attachment #8745353 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #22)
> (still feels wrong that a devtools iframe can be impacted by a custom UA set
> by an add-on. I expect fixing this is way more complicated though, so
> totally fine with shipping this now)

Yeah, seems we'll get bit by that again in the future when pulling in some dependency.  Although based on https://dxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#2630 I'm guessing that maybe the UA is being overridden because the CM frame is loaded with a data URI instead of a chrome URI.  If we pulled this into it's own chrome file the problem might go away also: https://dxr.mozilla.org/mozilla-central/source/devtools/client/sourceeditor/editor.js#81.  If that were the case then we'd at least know that the toolbox frame isn't being affected by the custom UA.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6d7529fafa31
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Dup bug 1261655 shows 44+ as affected. That means 47 and 48, which have not yet been released, are affected. Should we consider uplifting this trivial fix?

Joe - ni on you as Brian is out until tomorrow and we're building 47 RC1 today.
status-firefox46: --- → wontfix
status-firefox47: --- → affected
status-firefox48: --- → affected
Flags: needinfo?(jwalker)
Flags: needinfo?(bgrinstead)
Keywords: regression
So I've taken a look and talked it over with jryans, and we think an uplift is probably the right thing to do.
Do you need any help from devtools people?
Flags: needinfo?(jwalker)
Flags: needinfo?(bgrinstead)
Can you please add an uplift request to this bug.

Ritu - FYI. Trivial patch that we can uplift to fix a regression. What's the timing for RC1?
Flags: needinfo?(rkothari)
Flags: needinfo?(jwalker)
Comment on attachment 8745353 [details]
MozReview Request: Bug 1254025 - Force the CodeMirror editor to always use a textarea;r=jdescottes

Approval Request Comment
[Feature/regressing bug #]: Regressed by breaking up source editor iframes in combination with user agent add-on
[User impact if declined]: Source editors will be empty for users of the user agent add-on
[Describe test coverage new/current, TreeHerder]: New configuration tested on m-c
[Risks and why]: Low, has been on m-c ~1 month, only impacts DevTools
[String/UUID change made/needed]: None
Flags: needinfo?(jwalker)
Attachment #8745353 - Flags: approval-mozilla-beta?
Attachment #8745353 - Flags: approval-mozilla-aurora?
Comment on attachment 8745353 [details]
MozReview Request: Bug 1254025 - Force the CodeMirror editor to always use a textarea;r=jdescottes

It's not a recent regression but the fix looks simple enough to consider uplifting to Aurora48, Beta47.
Flags: needinfo?(rkothari)
Attachment #8745353 - Flags: approval-mozilla-beta?
Attachment #8745353 - Flags: approval-mozilla-beta+
Attachment #8745353 - Flags: approval-mozilla-aurora?
Attachment #8745353 - Flags: approval-mozilla-aurora+
Component: Developer Tools: Debugger → Developer Tools: Source Editor
Summary: [debugger] No scripts are loaded at all → Source editor is blank when user agent override is applied
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.