Extensions need a way to identify themselves to the add-ons manager, to allow for things like delaying restart or any activity in which we do not want extensions to be able to interfere with each other. Bootstrap's startup() function should accept an optional argument Symbol(addon.id), which will be unique but still give enough info to be useful for logging.

(In reply to Robert Helmer [:rhelmer] from comment #0) > Extensions need a way to identify themselves to the add-ons manager, to > allow for things like delaying restart or any activity in which we do not > want extensions to be able to interfere with each other. > > Bootstrap's startup() function should accept an optional argument > Symbol(addon.id), which will be unique but still give enough info to be > useful for logging. Correction - this should be a property on the data object, not an optional argument.

I feel like there's a bit too much repetition in the symbol passing, but there are a bunch of places we call startup(), any ideas to make this more concise? Also note that AddonManager.verifySymbol() really just exists for the benefit of the test since we don't have a real user of this yet, since the reference to the Symbol() is held inside XPIProvider.jsm I can't think of a more reasonable way to get it out for the test. Review commit: https://reviewboard.mozilla.org/r/35659/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35659/

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop https://reviewboard.mozilla.org/r/35659/#review32325 I think we can simplify this a bunch by just always including the instanceID in the params generated in callBootstrapMethod regardless of the type of method we're calling. unloadBootstrapScope should also delete the instanceID for the add-on. ::: toolkit/mozapps/extensions/AddonManager.jsm:2291 (Diff revision 1) > + }, So when an add-on wants to use an API that needs to know the add-on caller it has to pass its instanceID but I don't think it should need to pass the ID too since we can discover that from the instanceID. So I think this should be getAddonByInstanceID(aInstanceID, aCallback).

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35659/diff/1-2/

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop Actually I see a problem with the test, it should be making sure the returned addon is valid in the "success" case - I'll fix that up.

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35659/diff/2-3/

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35659/diff/3-4/

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop https://reviewboard.mozilla.org/r/35659/#review32541 Rather than doing so much work in the test add-on's bootstrap code you can instead just use BootstrapMonitor. After started, BootstrapMonitor.started.get(id).data.instanceID should be the symbol passed which you can then do tests with. Please also test with attempting to create a fake symbol to get the add-on. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3904 (Diff revision 4) > + return new Promise(resolve => AddonManager.getAddonByID(addonID, resolve)); You an just call this.getAddonByID directly ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3908 (Diff revision 4) > + return new Promise(resolve => null); I think you want Promise.resolve(null) here ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4625 (Diff revision 4) > + } I think we should also pass it to shutdown calls. Rather than storing in gAddonSymbols would you mind doing a little extra here and replacing the bootstrapScopes property with something like this: XPIProvider.activeAddons[id] = { bootstrapScope: <scope>, instanceID: <symbol> } Maybe just make it a map while you're there. With that it's probably just easier to just create the instanceID in loadBootstrapScope.

(In reply to Dave Townsend [:mossop] from comment #8) > Comment on attachment 8721412 [details] > MozReview Request: Bug 1249689 - generate and provide a Symbol for each > add-on on startup r?mossop > > https://reviewboard.mozilla.org/r/35659/#review32541 > > Rather than doing so much work in the test add-on's bootstrap code you can > instead just use BootstrapMonitor. After started, > BootstrapMonitor.started.get(id).data.instanceID should be the symbol passed > which you can then do tests with. Please also test with attempting to create > a fake symbol to get the add-on. Hm I think there is a "bad symbol" test in the addon bootstrap.js now, doesn't really matter since I will move it out of here and into the xpcshell test instead - thanks for the tip about getting the instanceID out of BootstrapMonitor! Reset SGTM, followup coming soon. I'll do the "bootstrapScope {} -> activeAddons Map" refactor in its own commit, then put this change on top of that.

Review commit: https://reviewboard.mozilla.org/r/36007/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36007/

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35659/diff/4-5/

https://reviewboard.mozilla.org/r/35659/#review32541 I have a patch which does this, but the instanceID isn't making it through to the xpcshell test (however I see it in `BootstrapMonitor.jsm`). I think this is because `data` gets `JSON.stringify`'d along with `info` in https://dxr.mozilla.org/mozilla-central/search?q=bootstrapmonitor-event&redirect=true&case=true and it can't do that with a symbol (it appears that `stringify` just silently removes or modifies Symbols per the docs https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify). If I hardcode some dummy string value to `data` in BootstrapMonitor then that makes it through to the xpcshell test just fine. We could call `toString()` instead of providing a reference to the actual symbol for testing purposes and just check that the key is set correctly, but we wouldn't be able to pass this into `getAddonByInstanceID()` so probably not a really great test. So for the moment I left the test code in `bootstrap.js`, let me know if I missed anything and/or if you have ideas for changing BootstrapMonitor to accomodate this.

Comment on attachment 8722397 [details] MozReview Request: Bug 1249689 - replace bootstrapScope with an activeAddons Map() that contains it r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36007/diff/1-2/

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35659/diff/5-6/

Comment on attachment 8722397 [details] MozReview Request: Bug 1249689 - replace bootstrapScope with an activeAddons Map() that contains it r?mossop https://reviewboard.mozilla.org/r/36007/#review32701 ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2698 (Diff revision 2) > - if (!(id in XPIProvider.bootstrapScopes)) > + if (!(XPIProvider.activeAddons.has(id))) No need for the extra parenthesis here now ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4459 (Diff revision 2) > + }); You could clean up the later code a little by declaring this as activeAddon rather than needing to get it from the map everytime. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4463 (Diff revision 2) > - this.bootstrapScopes[aId] = null; > + this.activeAddons.get(aId).bootstrapScope = null; If you just pre-declare it as null above then this line becomes unnecessary. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4598 (Diff revision 2) > - if (!(aAddon.id in this.bootstrapScopes)) { > + if (!(this.activeAddons.has(aAddon.id))) { Rather than using has here and get later, just get the activeAddon object and check it is truthy

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop https://reviewboard.mozilla.org/r/35659/#review32703 Looks good, I'd like to see a couple of tweaks to the test though. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3903 (Diff revision 6) > + for (let [aID, val] of this.activeAddons) { s/aID/id/ ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3909 (Diff revision 6) > + return new Promise.resolve(null); I don't think you need new here. ::: toolkit/mozapps/extensions/test/addons/test_symbol/bootstrap.js:7 (Diff revision 6) > + const PREF = "symboltest.instanceid.set"; You're using the same pref to track two different results generated from two asynchronous functions racing each other. It probably works correctly right now but I'd rather be explicit and use one pref for each test in case something changes about the timing later. ::: toolkit/mozapps/extensions/test/addons/test_symbol/bootstrap.js:22 (Diff revision 6) > + AddonManager.getAddonByInstanceID(Symbol("bad symbol")) Please also try the case that is Symbol(id)

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35659/diff/6-7/

Comment on attachment 8722397 [details] MozReview Request: Bug 1249689 - replace bootstrapScope with an activeAddons Map() that contains it r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36007/diff/2-3/

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35659/diff/7-8/

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop https://reviewboard.mozilla.org/r/35659/#review32741 ::: toolkit/mozapps/extensions/test/addons/test_symbol/bootstrap.js:26 (Diff revision 8) > + Services.prefs.setBoolPref(PASS_PREF, false); These are already set to false above, no need to set them again.

Comment on attachment 8721412 [details] MozReview Request: Bug 1249689 - generate and provide a Symbol for each add-on on startup r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35659/diff/8-9/

Waiting for try run before landing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9ab24a1198a

(In reply to Robert Helmer [:rhelmer] from comment #22) > Waiting for try run before landing: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9ab24a1198a Hm try seems to be stuck :/ going to re-try try.

Correct try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78fbdd60fa88 Also did a Linux one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d18af3039515 I went through the failures and pretty sure they're not caused by this patch, it's a bit worrying how many were showing up on Windows :/ The ones making external connections to google are weird too, but again unlikely to be this change.

https://hg.mozilla.org/integration/mozilla-inbound/rev/93390f1d1faa https://hg.mozilla.org/integration/mozilla-inbound/rev/002d7897c93c

https://hg.mozilla.org/mozilla-central/rev/93390f1d1faa https://hg.mozilla.org/mozilla-central/rev/002d7897c93c