Marionette triggers: browser/omni.ja!/components/nsBrowserGlue.js, line 675: ReferenceError: fetch is not defined

RESOLVED FIXED in Firefox 59

Status

()

defect
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Gijs, Assigned: gandalf)

Tracking

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 wontfix, firefox59 fixed, firefox60 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

a year ago
Spotted in the log listed in bug 1433024 for marionette tests:



[task 2018-01-25T02:18:10.182Z] 02:18:10     INFO -  JavaScript error: jar:file:///builds/worker/workspace/build/application/firefox/browser/omni.ja!/components/nsBrowserGlue.js, line 675: ReferenceError: fetch is not defined

https://treeherder.mozilla.org/logviewer.html#?job_id=158360654&repo=try&lineNumber=48573

This is a test merge trypush for current nightly, and I don't know if this problem is more widespread because I haven't looked for it yet and have too many other loose ends to chase, but purely at a glance and without thinking about this too hard, it looks like nsBrowserGlue.js should have Cu.importGlobalProperties(["fetch"]); and otherwise this will never work. That's... disturbing, because I don't understand how more stuff isn't broken in that case, as presumably we don't dispatch browser-ui-startup-complete. :zibi, can you take a look?
Flags: needinfo?(gandalf)
Reporter

Comment 1

a year ago
Also curious why eslint doesn't catch this.
Flags: needinfo?(standard8)
Reporter

Comment 2

a year ago
(In reply to :Gijs (lower availability until Jan 27) from comment #0)
> That's... disturbing, because I don't understand how more stuff isn't broken
> in that case, as presumably we don't dispatch browser-ui-startup-complete.
> :zibi, can you take a look?

Looks like this would "only" impact the webcontentconverter, so registerProtocolHandler and builtin web protocol/content-type handlers as nothing else depends on this observer notification...

Do we know of issues with those things with 58? I vaguely recall hearing that people were having issues with gmail and mailto: and stuff, but perhaps it's unrelated...
(In reply to :Gijs (lower availability until Jan 27) from comment #1)
> Also curious why eslint doesn't catch this.

Unfortunately, we've set ESLint up to assume that everything that isn't a .jsm has a browser environment. That makes sense for probably the majority of the files in the tree, but we obviously get it wrong for some of them like components... I've filed bug 1433419 to work out how to address this.
Flags: needinfo?(standard8)
(In reply to :Gijs (lower availability until Jan 27) from comment #0)
> That's... disturbing, because I don't understand how more stuff isn't broken
> in that case, as presumably we don't dispatch browser-ui-startup-complete.

It looks like WebContentConverter is the only one that listens to it now: https://searchfox.org/mozilla-central/search?q=browser-ui-startup-complete&case=true&regexp=false&path=
> and otherwise this will never work.

I don't understand this. In which scenario `fetch` is not available? Browser's localization wouldn't work otherwise (the result of the fetch request is registering the L10nRegistry FileSources needed to translate UI).
Comment hidden (mozreview-request)
This has been there since fx58.
Flags: needinfo?(gandalf)
Reporter

Comment 8

a year ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
> > and otherwise this will never work.
> 
> I don't understand this. In which scenario `fetch` is not available?
> Browser's localization wouldn't work otherwise (the result of the fetch
> request is registering the L10nRegistry FileSources needed to translate UI).

Well, this is essentially my question, not just a request to have the suggested change from comment #0 - if I was sure about that I would have just attached that patch myself immediately... sorry for not being clear.

What depends on this L10nRegistry thing? What's the fallback if it "doesn't work" ? As in, does this error happen all the time (I assume so)? If so, how have we missed it so far (aren't there tests?). If not, when does it break (and still, why didn't we catch it) ?
Flags: needinfo?(gandalf)
Reporter

Comment 9

a year ago
mozreview-review
Comment on attachment 8945823 [details]
Bug 1433411 - Import `fetch` API in nsBrowserGlue.

https://reviewboard.mozilla.org/r/215922/#review221710

We should understand what's going on here better, and we should check the perf implications, as well as ensuring we have tests that verify that this actually does something. If this is seriously 100% broken and we didn't notice, that doesn't speak well of our existing test coverage.
Attachment #8945823 - Flags: review?(gijskruitbosch+bugs) → review-
> What depends on this L10nRegistry thing?

The whole new localization infra depends on those lines working. At the moment there's just one string in 58 in Preferences, and 4 more in Fx 59 that use Fluent.

Those strings will not show up in the UI at all if this code doesn't work. 

> What's the fallback if it "doesn't work" ?

I'd expect the l10n-id's to be displayed in place of text in this section: https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/privacy.xul#402
Flags: needinfo?(gandalf)
Reporter

Comment 11

a year ago
I'm still confused as to what exactly is going on here. Here's what I have so far:

1. this happens on regular release, beta, and m-c marionette runs on treeherder
2. happens on at least linux and macos
3. doesn't reproduce when starting an m-c build "normally" on my local machine. Doublechecked with some extra logging that the code runs and executes successfully.
4. because AFAICT the marionette issues are related to restarts using quit() with a restart flag, I tried doing that from a browser console, but that also doesn't seem to trigger this issue for me locally with m-c.

I also verified that the tracking section of the prefs looks normal to me on both 58 and nightly.


I also tried running marionette locally, but it doesn't provide almost any test logging output to stdout/err, so I have no idea how to determine if the error happened or not. I also don't see log files in 'hg st' that are obviously from marionette, so no idea where the output went...

:whimboo, can you help diagnose what's going on here? Maybe a pref marionette sets is triggering this?

My other hypothesis is that some other thing imports this property and because some chrome globals are shared, we currently get 'lucky' for normal starts and things "work" - and somehow, under some (race?) conditions, we get unlucky and things break. Andrew, is that plausible? How would I check what else is importing 'fetch' onto this scope, and why it sometimes works and sometimes doesn't?
Flags: needinfo?(hskupin)
Flags: needinfo?(continuation)
Summary: browser/omni.ja!/components/nsBrowserGlue.js, line 675: ReferenceError: fetch is not defined → Marionette triggers: browser/omni.ja!/components/nsBrowserGlue.js, line 675: ReferenceError: fetch is not defined
(In reply to :Gijs from comment #11)
> Andrew, is that plausible?

That's certainly plausible. Nothing guarantees that the loading of JSMs is deterministic.

> How would I check what else is importing 'fetch' onto this scope, and why it sometimes works
> and sometimes doesn't?

You could add xpc_PrintJSStack(cx, false, false, false) to xpc::GlobalProperties::Parse() when "fetch" is matched. It'll even slow down such calls a little bit, so maybe it will make the problem easier to reproduce. You might also need to dump the global, but as a start you could assume it is the shared JS global.
Flags: needinfo?(continuation)
Gijs, I won't have time right now but if you want to see more logging output specify the -vv option for marionette itself or mach. By default the log ends-up in gecko.log in CWD, but by using `--gecko-log -` it will be printed to stdout. I hope this helps for now.

Can we tell me again, when this exactly happens in Marionette tests?
Flags: needinfo?(hskupin)
For the record, passing the -vv flag to the "./mach marionette test" harness is equivalent to setting the marionette.log.level preference to "trace".
Priority: -- → P1
Reporter

Comment 15

a year ago
mozreview-review
Comment on attachment 8945823 [details]
Bug 1433411 - Import `fetch` API in nsBrowserGlue.

https://reviewboard.mozilla.org/r/215922/#review222660

OK, so the reason for this not working in a test is because the test in question isn't the failing Firefox refresh test, but it's failing in:

```
[task 2018-01-25T02:18:06.768Z] 02:18:06     INFO - TEST-START | js/xpconnect/tests/marionette/test_loader_global_sharing.py TestLoaderGlobalSharing.test_global_sharing_settings
```

which as the name suggests, tests what happens if disabling global sharing. It doesn't test much else so presumably it's relatively understandable that this doesn't trip anything else even though stuff breaks in core browser code.

Just requiring fetch as this patch does will fix the issue, so we should just land this.
Attachment #8945823 - Flags: review- → review+

Comment 16

a year ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e9b3af583a0e
Import `fetch` API in nsBrowserGlue. r=Gijs
Reporter

Comment 17

a year ago
Do we (in another bug) need to update this or another test to make it fail more fail-y when changing the 'share globals' flag causes JS errors? Or do we no longer really care about this test and/or the no-shared-globals case?
Reporter

Comment 18

a year ago
(In reply to :Gijs from comment #17)
> Do we (in another bug) need to update this or another test to make it fail
> more fail-y when changing the 'share globals' flag causes JS errors? Or do
> we no longer really care about this test and/or the no-shared-globals case?

Gah mid-airs, +ni this time...
Flags: needinfo?(continuation)
https://hg.mozilla.org/mozilla-central/rev/e9b3af583a0e
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(In reply to :Gijs from comment #17)
> Do we (in another bug) need to update this or another test to make it fail
> more fail-y when changing the 'share globals' flag causes JS errors? Or do
> we no longer really care about this test and/or the no-shared-globals case?

Yeah, it has shipped to release without any problems reported, so maybe we should remove the pref. It was a big perf boost, so I can't imagine us disabling it except in some very dire circumstances.

Kris, what do you think?
Flags: needinfo?(continuation) → needinfo?(kmaglione+bmo)
(In reply to Andrew McCreight [:mccr8] from comment #20)
> (In reply to :Gijs from comment #17)
> > Do we (in another bug) need to update this or another test to make it fail
> > more fail-y when changing the 'share globals' flag causes JS errors? Or do
> > we no longer really care about this test and/or the no-shared-globals case?
> 
> Yeah, it has shipped to release without any problems reported, so maybe we
> should remove the pref. It was a big perf boost, so I can't imagine us
> disabling it except in some very dire circumstances.
> 
> Kris, what do you think?

It's probably fine to remove it at this point. I mainly added that test because we were shipping this late in the cycle and needed to be sure we could continue to disable it if issues turned up.

On the other hand, the ability to turn off shared JSM globals may still be useful for debugging some issues, like leaks and poor memory usage. It's not worth the memory overhead most of the time, but having separate about:memory entries for each JSM can be pretty useful for getting a picture of which components are using the most memory.

Either way, in this case, the test found an actual bug.
Flags: needinfo?(kmaglione+bmo)
Please request beta uplift when you get a chance.
Assignee: nobody → gandalf
Comment on attachment 8945823 [details]
Bug 1433411 - Import `fetch` API in nsBrowserGlue.

Approval Request Comment
[Feature/Bug causing the regression]: bug 
1410731 
[User impact if declined]: there's an error in a marionette test and potentially in some rare scenarios at runtime (?)
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: because it adds one line of well understood code to explicitly include the API we use in that file (rather than relying on that API being available implicitly)
[String changes made/needed]: none
Attachment #8945823 - Flags: approval-mozilla-beta?
Comment on attachment 8945823 [details]
Bug 1433411 - Import `fetch` API in nsBrowserGlue.

Taking for 59b8.
Attachment #8945823 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.