Closed Bug 1442347 Opened 2 years ago Closed 2 years ago

Device list fails to load with "TypeError: XMLHttpRequest is not a constructor"

Categories

(DevTools :: Responsive Design Mode, defect, P1)

defect

Tracking

(firefox58 unaffected, firefox59 unaffected, firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(2 files)

No description provided.
Seems like the changes in bug 792808 leaves DevTools with XHR as undefined.
Assignee: nobody → jryans
Blocks: 792808
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8955282 [details]
Bug 1442347 - Clean up DevTools builtin syntax.

https://reviewboard.mozilla.org/r/224438/#review230654

Thanks for splitting this first part!

::: devtools/shared/builtin-modules.js:255
(Diff revision 1)
>    //     ... code ...
>    //   });
>    //
>    // Bug 1248830 will work out a better plan here for our content module
>    // loading needs, especially as we head towards devtools.html.
>    define(factory) {

Since we are sorting things, this one should go before DocumentFragment
Attachment #8955282 - Flags: review?(jdescottes) → review+
Comment on attachment 8955283 [details]
Bug 1442347 - Get more globals from sandbox instead of JSM in DevTools.

https://reviewboard.mozilla.org/r/224440/#review230656

Patch looks good but I can't reproduce the bug locally and verify the fix. I can see the bug on my regular nightly, but not on a local build. Do you reproduce the problem?
I was wondering why the regression had not been caught by devtools/client/responsive.html/test/browser/browser_device_modal_submit.js , maybe that's related to this?

::: commit-message-5dbbb:4
(Diff revision 1)
> +Bug 1442347 - Get XHR from sandbox instead of JSM in DevTools. r=jdescottes
> +
> +Bug 792808 changed how XHRs are constructed.  The DevTools changes there appear
> +to incomplete, as they left XHR as undefined in DevTools modules.

nit: to incomplete -> to be incomplete
Attachment #8955283 - Flags: review?(jdescottes) → review+
Comment on attachment 8955283 [details]
Bug 1442347 - Get more globals from sandbox instead of JSM in DevTools.

https://reviewboard.mozilla.org/r/224440/#review230670

Sorry didn't mean to r+ right away, I would like to understand why the issue is only visible on Nightly and didn't affect the test mentioned earlier.
Attachment #8955283 - Flags: review+
Ok that seems profile specific, on a clean profile I don't have the issue, but on my regular nightly profile I do.
I think you will love this: the issue only occurs if ADB Helper is enabled. And your fix indeed solves the issue.
Okay, just to make sure, I ran mozregression with ADB Helper installed, and it is indeed related to bug 792808:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2f76fdc4ee3ef4648e1edf001f923859b648373b&tochange=560e1ce075054819e1d5c9ae98edbbe4f34c887d

Now I'll try to bisect bits of ADB Helper to see what triggers this issue.
It's a bit challenging to trigger with add-on development workflows like about:debugging, since ADB Helper needs to be installed before builtin-modules.js runs for the first time.  Since about:debugging is a DevTools thing, opening it triggers builtin-modules.js.

For the moment, I am using the proxy file approach:

* In <profile>/extensions, place a text file named "adbhelper@mozilla.org" that contains the path to ADB Helper source
* Set extensions.legacy.enabled = true
* Set xpinstall.signatures.required = false

This path still appears to trigger the issue.  Now I'll chop up the add-on...
Comment on attachment 8955283 [details]
Bug 1442347 - Get more globals from sandbox instead of JSM in DevTools.

https://reviewboard.mozilla.org/r/224440/#review230780

Now that we understand what triggers different behaviors, we might want to land this and finish the investigation in a follow up.
The commit description could be updated to reflect our last findings. I"m not sure where you are at in your ADB Helper investigation so if you want to keep nivestigating for the moment that's fine for me as well.
Attachment #8955283 - Flags: review+
Okay, after some debugging and discussion with :bz, we have a reasonable explanation of what's happening here.

So, `XMLHttpRequest`[1] is not actually exposed on JSMs by default.  If it were, it would be marked "Exposed=System".  However, various[2] browser JSMs use `importGlobalProperties` to access it.  These days, JSMs are loaded into a shared global, so once one of these modules has called `importGlobalProperties`, it becomes accessible via any JSMs loaded into that shared global.

The reason that ADB Helper plays a role here is that it calls into the DevTools loader and builtin-modules.js very early during browser startup.  Whoever is first to trigger `builtin-modules.js` locks in these globals for all of DevTools.  So, ADB Helper causes this to happen before some other random browser module happens to import XHR for us.

Given all of this, I think we convert all of these globals to the second style (`wantGlobalProperties` on a sandbox) where possible, since that's quite a bit less mysterious.

[1]: https://searchfox.org/mozilla-central/rev/908e3662388a96673a0fc00b84c47f832a5bea7c/dom/webidl/XMLHttpRequest.webidl#56
[2]: https://searchfox.org/mozilla-central/search?q=importGlobalProp.*XMLHttp&case=true&regexp=true&path=browser
Comment on attachment 8955282 [details]
Bug 1442347 - Clean up DevTools builtin syntax.

https://reviewboard.mozilla.org/r/224438/#review230654

> Since we are sorting things, this one should go before DocumentFragment

Thanks, fixed!
Comment on attachment 8955283 [details]
Bug 1442347 - Get more globals from sandbox instead of JSM in DevTools.

https://reviewboard.mozilla.org/r/224440/#review230656

> nit: to incomplete -> to be incomplete

Thanks, I've rewritten the message with our new knowledge on the issue.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ebf2c5afd772
Clean up DevTools builtin syntax. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/1fc27d34e1b3
Get more globals from sandbox instead of JSM in DevTools. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/ebf2c5afd772
https://hg.mozilla.org/mozilla-central/rev/1fc27d34e1b3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.