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)
Firefox
Normandy Client
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)
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
Comment 3•7 years ago
|
||
Attachment #8822462 -
Attachment is obsolete: true
Comment 4•7 years ago
|
||
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. :-(
Comment 6•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8823349 -
Flags: review?(gijskruitbosch+bugs)
Comment 8•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
(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)
Assignee | ||
Comment 12•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: mkelly → mcooper
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
Pushed by mcooper@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/10565a2aa201 Fix un-imported fetch use in NormandyApi.jsm r=Gijs
Comment 15•7 years ago
|
||
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)
Comment 16•7 years ago
|
||
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/306c4bd4973f Backed out changeset 10565a2aa201 for mochitest browser chrome failures
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 18•7 years ago
|
||
Pushed by mcooper@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1fec53b169d0 Fix un-imported fetch use in NormandyApi.jsm r=Gijs
Comment 19•7 years ago
|
||
mozreview-review |
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.
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1fec53b169d0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 21•7 years ago
|
||
(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.
Comment 22•7 years ago
|
||
(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.
Updated•7 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 23•7 years ago
|
||
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)
Updated•6 years ago
|
Product: Shield → Firefox
You need to log in
before you can comment on or make changes to this bug.
Description
•