The main culprit seems to be https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/addon/window.js , which seems to be used for XHRs (?) and creating content-privileged subframes, and maybe other things. I think this just about survived bug 1145470 because it never actually links to any problematic URIs, it just creates an about:blank with system privileges directly in the docshell and then loads a data: URI into it. But bug 1320124 ends that, so this will need some updating.

I have half a mind to suggest we just kill this right now, but I guess it would break every add-on that uses a panel or page worker... We can switch to a windowless browser easily enough.

Comment on attachment 8854162 [details] Bug 1353104 - use HiddenFrame.jsm to create frames for add-on sdk add-ons, https://reviewboard.mozilla.org/r/126134/#review128734 r=me if tests pass. I wouldn't be surprised if this breaks add-ons in some subtle ways, though... ::: addon-sdk/source/lib/sdk/addon/window.js:20 (Diff revision 1) > const cfxArgs = require("../test/options"); > > var addonPrincipal = Cc["@mozilla.org/systemprincipal;1"]. > createInstance(Ci.nsIPrincipal); > > -var hiddenWindow = getHiddenWindow(); > +let HiddenFrame = Cu.import("resource:///modules/HiddenFrame.jsm", {}).HiddenFrame; Nit: `const {HiddenFrame} = require("resource:///modules/HiddenFrame.jsm");` ::: addon-sdk/source/lib/sdk/addon/window.js:21 (Diff revision 1) > > var addonPrincipal = Cc["@mozilla.org/systemprincipal;1"]. > createInstance(Ci.nsIPrincipal); > > -var hiddenWindow = getHiddenWindow(); > +let HiddenFrame = Cu.import("resource:///modules/HiddenFrame.jsm", {}).HiddenFrame; > +let myHiddenFrame = new HiddenFrame(); Nit: s/myHiddenFrame/hiddenFrame/g please ::: addon-sdk/source/lib/sdk/addon/window.js:22 (Diff revision 1) > +exports.ready = myHiddenFrame.get(); > +exports.window = myHiddenFrame.getWindow(); :( I hate that we have to do this so eagerly... but I guess this is the API we're stuck with... ::: browser/modules/HiddenFrame.jsm:51 (Diff revision 1) > + */ > + getWindow() { > + this.get(); > + this._browser.QueryInterface(Ci.nsIInterfaceRequestor); > + let docShell = this._browser.getInterface(Ci.nsIDocShell); > + return docShell.contentViewer.DOMDocument.defaultView; Nit: `_browser.getInterface(Ci.nsIWebNavigation).document.defaultView)` Alternatively, skip the content viewer and `.getInterface(Ci.nsIDOMWindow)` on the webnav/docshell.

(In reply to Kris Maglione [:kmag] from comment #6) > Comment on attachment 8854162 [details] > Bug 1353104 - use HiddenFrame.jsm to create frames for add-on sdk add-ons, > > https://reviewboard.mozilla.org/r/126134/#review128734 > > r=me if tests pass. I wouldn't be surprised if this breaks add-ons in some > subtle ways, though... Can you elaborate on how you expect this to break add-ons and/or if we should do something to make this optional / pref-off-able in case it does? It looks like this crashes on Linux64. Because of course it does. Also, leaks on Windows and OS X. Not entirely sure why, tbh. :-\ > ::: addon-sdk/source/lib/sdk/addon/window.js:20 > (Diff revision 1) > > const cfxArgs = require("../test/options"); > > > > var addonPrincipal = Cc["@mozilla.org/systemprincipal;1"]. > > createInstance(Ci.nsIPrincipal); > > > > -var hiddenWindow = getHiddenWindow(); > > +let HiddenFrame = Cu.import("resource:///modules/HiddenFrame.jsm", {}).HiddenFrame; > > Nit: `const {HiddenFrame} = require("resource:///modules/HiddenFrame.jsm");` FWIW, when I tried this with Cu.import, the constructor then threw claiming it wasn't a constructor.

(In reply to :Gijs from comment #7) > (In reply to Kris Maglione [:kmag] from comment #6) > > Comment on attachment 8854162 [details] > > Bug 1353104 - use HiddenFrame.jsm to create frames for add-on sdk add-ons, > > > > https://reviewboard.mozilla.org/r/126134/#review128734 > > > > r=me if tests pass. I wouldn't be surprised if this breaks add-ons in some > > subtle ways, though... > > Can you elaborate on how you expect this to break add-ons No, I wish I knew, because then I'd know how to fix it. But people always wind up using these APIs in weird ways we don't expect. > and/or if we should do something to make this optional / pref-off-able in > case it does? It might be worth making it preffable so we can turn it off with a hotfix, if we need to. > > Nit: `const {HiddenFrame} = require("resource:///modules/HiddenFrame.jsm");` > > FWIW, when I tried this with Cu.import, the constructor then threw claiming > it wasn't a constructor. Weird.

Comment on attachment 8854162 [details] Bug 1353104 - use HiddenFrame.jsm to create frames for add-on sdk add-ons, Previous trypushes had leaks. It seems that the system/unload method isn't called reliably and thus the windowless browsers leak. To work around this, I forced HiddenFrame.jsm to kill off any remaining browsers on xpcom-shutdown. As noted on IRC, it would be better to make the windowless browsers cycle-collected instead. I don't have time to dive into that right now, also because I know approximately nothing about our cycle collector and don't have time to deep-dive into that as well. In an ideal world we should also figure out what's going on with unload not being called in the SDK. In the world we live in, I can't really bring myself to care given all this code will be dead by 57. I added a pref as discussed. While investigating the leaks (which happened, among others, in test-xhr.js) I also saw that the SDK XMLHttpRequest code uses a hidden frame for the requests. That seemed odd, so I just replaced it with Cu.importGlobalProperties... which seems to pass all the tests and seemed to simplify code *a lot*, so I left it. Let me know if this makes sense. Requesting review again because the changes are substantial.

(In reply to :Gijs from comment #12) > As noted on IRC, it would be better to make the windowless browsers > cycle-collected instead. I don't have time to dive into that right now, also > because I know approximately nothing about our cycle collector and don't > have time to deep-dive into that as well. This is a particularly tricky case, anyway, so it's probably not the best place to start learning about the cycle collector. But the more we start to use windowless browsers, the more I worry that this is going to bite us. So if you file a bug, I'll see what I can do about it.

Comment on attachment 8854162 [details] Bug 1353104 - use HiddenFrame.jsm to create frames for add-on sdk add-ons, https://reviewboard.mozilla.org/r/126134/#review129570 ::: addon-sdk/source/lib/sdk/addon/window.js:12 (Diff revision 5) > +const { get } = require("../preferences/service"); > + > +if (!get("extensions.usehiddenwindow", false)) { Ick. Can you use something like `const prefs = require(...); ... if(!prefs.get)`? I know this is the usual sort of style in SDK code, but I find it really hard to follow, sometimes. ::: addon-sdk/source/lib/sdk/net/xhr.js:11 (Diff revision 5) > module.metadata = { > "stability": "stable" > }; > > const { deprecateFunction } = require("../util/deprecate"); > -const { Cc, Ci } = require("chrome"); > +const { Ci , Cu } = require("chrome"); Nit: Extra space after `Ci` ::: browser/modules/HiddenFrame.jsm:17 (Diff revision 5) > Cu.import("resource://gre/modules/Services.jsm"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > const XUL_PAGE = "data:application/vnd.mozilla.xul+xml;charset=utf-8,<window%20id='win'/>"; > > +const gAllBrowsers = new Set(); Nit: Can you make this a `WeakSet` and use `ChromeUtils.nondeterministicGetWeakSetKeys` to iterate over it, just so things get cleaned up a bit earlier if someone forgets to call `destroy()`? You'd have to store the `HiddenFrame` instance, though, rather than the browser, since `WeakSet`s only support JSAPI and WebIDL objects. ::: browser/modules/HiddenFrame.jsm:23 (Diff revision 5) > + > +let cleanupRegistered = false; > +function ensureCleanupRegistered() { > + if (!cleanupRegistered) { > + cleanupRegistered = true; > + Services.obs.addObserver( function() { Nit: Extra space after `(`

(In reply to Kris Maglione [:kmag] from comment #15) > Nit: Can you make this a `WeakSet` and use > `ChromeUtils.nondeterministicGetWeakSetKeys` to iterate over it, just so > things get cleaned up a bit earlier if someone forgets to call `destroy()`? > > You'd have to store the `HiddenFrame` instance, though, rather than the > browser, since `WeakSet`s only support JSAPI and WebIDL objects. AIUI that would mean that if the hiddenframe ref went dead, it'd disappear from the weakset, but the browser might not be destroyed yet (assuming someone has a window ref somewhere or whatever). Right? Or am I missing something?

(In reply to :Gijs from comment #16) > (In reply to Kris Maglione [:kmag] from comment #15) > > Nit: Can you make this a `WeakSet` and use > > `ChromeUtils.nondeterministicGetWeakSetKeys` to iterate over it, just so > > things get cleaned up a bit earlier if someone forgets to call `destroy()`? > > > > You'd have to store the `HiddenFrame` instance, though, rather than the > > browser, since `WeakSet`s only support JSAPI and WebIDL objects. > > AIUI that would mean that if the hiddenframe ref went dead, it'd disappear > from the weakset, but the browser might not be destroyed yet (assuming > someone has a window ref somewhere or whatever). Right? Or am I missing > something? I thought about that, but the browser is stored in a private property, and none of our existing code touches it, so nothing should be keeping the browser ref alive if the HiddenFrame ref is dead. Keeping the window ref alive on its own shouldn't do it. It only becomes an issue if the window holds a reference that eventually gets back to the browser, and that should only be possible via the HiddenFrame. If I'm wrong about that, switching to a WeakSet should cause the SDK tests to fail again, so we'll soon find out. And if I'm wrong, we'll have learned something interesting...

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3d1ad05d7b99 use HiddenFrame.jsm to create frames for add-on sdk add-ons, r=kmag

Backed out in https://hg.mozilla.org/integration/autoland/rev/e850b9c21333 for https://treeherder.mozilla.org/logviewer.html#?job_id=89676690&repo=autoland on Android

I really wish we had failure screenshots on android... Anyway, there's a good possibility this is related the remote XUL failure in bug 1352043, but if not, I'd be willing to just disable that test on Android.

(In reply to Kris Maglione [:kmag] from comment #21) > I really wish we had failure screenshots on android... > > Anyway, there's a good possibility this is related the remote XUL failure in > bug 1352043, but if not, I'd be willing to just disable that test on Android. Well, speaking of which... don't we kind of not really ship XUL on Android (except, IIRC, we still do kind-of, for various sundry reasons)? Is loading a XUL data: URI in that hidden browser going to work? Might be fine for browser/ code - it doesn't need to run on android. Less sure about sdk add-ons. Aren't those supposed to work on Android?

No, we do ship XUL on android, we just avoid using it as much as possible. But we still use XUL browsers for things like tabs, and some random bits of XUL in places like the add-on manager. So this should definitely work. Also, we do something similar for WebExtension background pages, which work fine on android.

1352043 landed, but a trypush with this shows android is still orange: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97c0cb453ad4e68bb2504bbfe924801133c458ea&selectedJob=90449117 Going to land with the test disabled per comment #21.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/810436dcf99e use HiddenFrame.jsm to create frames for add-on sdk add-ons, r=kmag

I'm seeing errors in the logs indicating that HiddenFrame.jsm is missing on Android.

(In reply to Jan Henning [:JanH] from comment #28) > I'm seeing errors in the logs indicating that HiddenFrame.jsm is missing on > Android. Please file a new bug with details, specifically which "the logs", and needinfo me there. Thanks!