Closed Bug 1326225 Opened 7 years ago Closed 7 years ago

Error in NormandyApi.jsm:37: ReferenceError: fetch is not defined

Categories

(Firefox :: Normandy Client, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ehsan.akhgari, Assigned: mythmon)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

While running a debug build, I get the following error:

*************************
A coding exception was thrown and uncaught in a Task.

Full message: ReferenceError: fetch is not defined
Full stack: apiCall@resource://shield-recipe-client/lib/NormandyApi.jsm:37:5
get@resource://shield-recipe-client/lib/NormandyApi.jsm:44:12
this.NormandyApi.fetchRecipes<@resource://shield-recipe-client/lib/NormandyApi.jsm:52:34
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
this.RecipeRunner.start<@resource://shield-recipe-client/lib/RecipeRunner.jsm:64:23
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
setTimeout_timer@resource://gre/modules/Timer.jsm:30:5

*************************

The fetch API isn't available outside of the window or worker contexts, so the code as written is guaranteed to never work.

Mike, can you please take this?
Flags: needinfo?(mcooper)
I put a PR up for this yesterday! https://github.com/mozilla/normandy-addon/pull/42

Mythmon will be back from PTO next week, at which point we'll review/merge them and consider when we want to do our next merge to moz-central for the system add-on.
Assignee: nobody → mkelly
Flags: needinfo?(mcooper)
Attached file Github PR (obsolete) —
Attached file Github PR
Attachment #8822462 - Attachment is obsolete: true
r=me to land this on mozilla-central, assuming it's green on try (I assume CI on github doesn't run the browser chrome tests...). Sorry for missing this in review. :-(
I can confirm I see this bug too on a non-debug build, it gets triggered by drawing diagrams on https://www.draw.io/ and I have to kill the firefox process.
Attachment #8823349 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8823349 [details]
Bug 1326225 - Fix un-imported fetch use in NormandyApi.jsm

https://reviewboard.mozilla.org/r/101904/#review102290

::: browser/extensions/shield-recipe-client/test/browser_NormandyApi.js:8
(Diff revision 1)
> +  SpecialPowers.setCharPref(
> +    "extensions.shield-recipe-client.api_url",
> +    "http://mochi.test:8888/browser/browser/extensions/shield-recipe-client/test",
> +  );

Now that I just saw this, please use pushPrefEnv, so that the pref gets reset when the test has finished (though the difference likely won't matter right now, it might if we're going to be using this for other tests).

::: browser/extensions/shield-recipe-client/test/test_server.sjs:7
(Diff revision 1)
> +  // Allow cross-origin, so you can XHR to it!
> +  response.setHeader("Access-Control-Allow-Origin", "*", false);
> +  // Avoid confusing cache behaviors
> +  response.setHeader("Cache-Control", "no-cache", false);
> +  response.setHeader("Content-Type", "application/json", false);
> +  response.write("{\"test\":\"data\"}");

This or JSON.stringify({test: "data"}), assuming that works?
Attachment #8823349 - Flags: review?(gijskruitbosch+bugs) → review+
Gijs: I've made the changes you suggested in the most recent version of the patch.

The try run [0] has a lot of oranges in it, but they all seem unrelated to this change to me. Gijs, do you think those represent problems with the add-on? Or are they unrelated and we should merge this?

[0]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d10f4ce7925ba26ca8155024b06119e34a4926a0&selectedJob=65920496
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mike Cooper [:mythmon] from comment #10)
> Gijs: I've made the changes you suggested in the most recent version of the
> patch.
> 
> The try run [0] has a lot of oranges in it, but they all seem unrelated to
> this change to me. Gijs, do you think those represent problems with the
> add-on? Or are they unrelated and we should merge this?
> 
> [0]:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d10f4ce7925ba26ca8155024b06119e34a4926a0&selectedJob=6
> 5920496

Certainly the valgrind stuff isn't your fault.

The other stuff... it's a known intermittent, but the thing that's odd is that it's failing in almost every test suite, and the messages in the leak bugs indicate they're frequent but not quite *that* frequent.

It looks like your trypush is on top of recent m-c (earlier today), which looked like this:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=cad2ea346d06ec5a3a70eda912513201dff0c21e

note a lot less orange mentioning that leak...

What I suspect is happening is that:

a) we didn't import fetch before, so we weren't fetching
b) with this patch, we're making the normandy code actually fetch stuff (even if it's doing dead fetches against a local test-only endpoint set in the general prefs file, except for this single new test we're adding, it's still making http connections to said endpoint)
c) those fetches are hitting http cache/xhr code which triggers leaks like in bug 1326136.

A trivial way of seeing if my hypothesis is right: do a trypush with the fetching delay (we don't fetch immediately on startup, right?) modified to something very long, like say 5 hours. None of the Firefoxen used for these test runs will stay up that long, so effectively the actual fetching won't happen unless done explicitly as part of the test you're adding. That would mean most of the oranges would go away... but you might still see the leak appear on the chunk of mochitest-browser where your new test is running.

If my hypothesis is correct, this might help the folks working on bug 1317290, bug 1326136 and bug 1325438 because they're all XHR/fetch-related, but currently pretty intermittent. If this patch makes them trivial to reproduce, that could help with fixing the leak in the core code that's leaking - which of course will eventually let you land... I've CC'd a few folks in case they want to poke at this.
Flags: needinfo?(gijskruitbosch+bugs)
I've pushed to try a version with a startup delay of 24 hours, to test your hypothesis.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f72ce97c00011a249a7f6c25a9a641505666b03e
Assignee: mkelly → mcooper
Pushed by mcooper@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/10565a2aa201
Fix un-imported fetch use in NormandyApi.jsm r=Gijs
sorry had to back out this for mochitest browser chrome failures, i.e., https://treeherder.mozilla.org/logviewer.html#?job_id=68241787&repo=autoland&lineNumber=6973
Flags: needinfo?(mcooper)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/306c4bd4973f
Backed out changeset 10565a2aa201 for mochitest browser chrome failures
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by mcooper@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fec53b169d0
Fix un-imported fetch use in NormandyApi.jsm r=Gijs
Comment on attachment 8823349 [details]
Bug 1326225 - Fix un-imported fetch use in NormandyApi.jsm

https://reviewboard.mozilla.org/r/101904/#review105016

::: browser/extensions/shield-recipe-client/test/browser.ini:6
(Diff revisions 3 - 4)
>  [browser_driver_uuids.js]
>  [browser_env_expressions.js]
>  [browser_EventEmitter.js]
>  [browser_Storage.js]
>  [browser_Heartbeat.js]
> +skip-if = true # bug 1325409

Is skipping this test really OK? The one-time intermittent in that bug doesn't seem relevant to the memory leak triggered by this change. And the memory leak is worrying on its own.
https://hg.mozilla.org/mozilla-central/rev/1fec53b169d0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Kris Maglione [:kmag] from comment #19)
> Comment on attachment 8823349 [details]
> Bug 1326225 - Fix un-imported fetch use in NormandyApi.jsm
> 
> https://reviewboard.mozilla.org/r/101904/#review105016
> 
> ::: browser/extensions/shield-recipe-client/test/browser.ini:6
> (Diff revisions 3 - 4)
> >  [browser_driver_uuids.js]
> >  [browser_env_expressions.js]
> >  [browser_EventEmitter.js]
> >  [browser_Storage.js]
> >  [browser_Heartbeat.js]
> > +skip-if = true # bug 1325409
> 
> Is skipping this test really OK? The one-time intermittent in that bug
> doesn't seem relevant to the memory leak triggered by this change. And the
> memory leak is worrying on its own.

I would concur... We need to figure this out ASAP, because these tests indicate we're leaking like a sieve here (what with them leaking an entire browser window). These leaks are different from the XHR leaks, which AIUI got fixed in a different bug.
Blocks: 1330954
(In reply to :Gijs from comment #21)
> These leaks are different from the XHR leaks, which AIUI got fixed in a
> different bug.

The XHR leaks were fixed by changing the update URL to point to an HTTPS
port so they don't stall. The fact that stalled requests cause shutdown
leaks is being tracked in bug 1326136.
Flags: needinfo?(gijskruitbosch+bugs)
The addition of the skip test was originally done in bug 1325148. I had actually removed it because the change hasn't been backported to the Github repo I made my changes from. I also under estimated the severity of the problem.

I'm not sure why the comment on the skip-if referenced bug 1325409. The bug it should have referenced was bug 1328590.
Flags: needinfo?(mcooper)
Depends on: 1344670
Depends on: 1345650
Product: Shield → Firefox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: