The code that avoid addon users needs to be in tree.

A few clarification: Do we want to disable e10s (and maybe restart) for users: - with only disabled add-ons? - when they're installing their first add-on? - when they're enabling their first add-on? - having only default add-ons (on some OS / with some language settings there are add-ons enabled by default) Do we want to prompt users OR auto enable e10s (and maybe restart) for users: - when removing/disabling their last active add-on?

Dave, I'm trying to get the number of installed and active add-ons (extensions not plugins), and I want to do this early in the startup, so I can prevent the browser to start with e10s on. I don't think this is possible with the AddonManager so I think we should maintain a pref for the number of active extensions for this. Before I try to figure it out how to do it, I was wondering if we already have something like that or do you think this is the right approach or do you see some better way to do this?

You need a combination of the pref extensions.bootstrappedAddons and the pref extensions.enabledAddons to get the list of active add-ons without incurring startup costs. It's a bit of a hack and not really meant to be used outside the add-ons manager so if there is a way that you could make the code that does this live in the add-ons manager (ideally in https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#3140) then we'd be less likely to break it, otherwise add tests or run the risk!

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1) > A few clarification: > > Do we want to disable e10s (and maybe restart) for users: > - with only disabled add-ons? I think we should avoid users with 3rd party addons installed in general, regardless of whether those addons are enabled or disabled. I think it's a safe bet that users with disabled addons will at some point enable one of those addons. So lets avoid this issue all together and target users with no addons installed. > - when they're installing their first add-on? yes. > - when they're enabling their first add-on? yes, although isn't this superseded by the install condition above? > - having only default add-ons (on some OS / with some language settings > there are add-ons enabled by default) Ignoring the "default theme addon", I think we want to avoid all addons in general, including addons like pocket or webrtc that come pre-installed. > Do we want to prompt users OR auto enable e10s (and maybe restart) for users: > - when removing/disabling their last active add-on? Well the selection criteria code should automatically kick in here at startup (or whenever it runs) for users who have no addons installed. So I think these users are already covered by default behavior.

Just a suggestion; Code should do the "opposite" of the title. ie Enable e10s for users without addons. My understanding this is just temporary, (one or two releases.) Using existing preferences seems likely to add problems. Add pref eg "browser.tabs.remote.autostart.0" Probably bug 1232274 is best done as part of this one.

(In reply to Jonathan Howard from comment #5) > Just a suggestion; > Code should do the "opposite" of the title. ie Enable e10s for users without > addons. In my understanding we will turn on e10s for beta population, once that happens we want to be sure that users with add-ons installed will not be affected. This bug is about the later part, while the former should be done in a separate bug (depending on this one and on many others). That other bug is kind of a meta bug in fact, see bug 1218484. > My understanding this is just temporary, (one or two releases.) > Using existing preferences seems likely to add problems. > Add pref eg "browser.tabs.remote.autostart.0" > This might be a very good point, could you elaborate on this a bit? I was about to ask around about this anyway, can you explain to me how are these prefs used exactly in practice by any chance? (browser.tabs.remote.autostart, browser.tabs.remote.autostart.1, browser.tabs.remote.autostart.2) > Probably bug 1232274 is best done as part of this one. I'm not convinced. Why do you think these two bugs should be merged? I think this bug should be blocked on bug 1232274 instead. But maybe I'm overlooking something?

Definitely ask around. I'm not part of inside meetings so many know more. I have taken view this (patch) is not just for beta. If the go-ahead is given for release, the code change will be a simple toggle of this. Bug 1225496 50/50 split and any subsequent splits while still in testing add complication, so why I suggest using another preference. browser.tabs.remote.autostart.1 - unused browser.tabs.remote.autostart.2 - has been the enable by default for nightly/aurora/(experiments beta44) browser.tabs.remote - used by automated tests. Linked to nightly/aurora UI being able to toggle e10s. I can see a preference having 4 states; uninitialised - e10s off until determined that no add-ons are installed. inactive - e10s off enable no addons disable due to addons Other prefs being true override this without-addons pref.

Felipe also has some ideas for this, so CCing him in case he hasn't seen this bug.

Since others might have different plans for this bug, for transparency, here is my current plan: - After Jim's clarification I decided that using extensions.xpiStat pref would be the best here (it contains the list of all extensions, not just the active or the bootstrapped ones) - The logic for deciding at start up if we want e10s seems to be living here: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4671 in C++ and since I don't want to parse JSON and more importantly I don't want to separate xpiStat computing from XPIProvider that much: - I plan to do the work of loading and parsing extensions.xpiStat in a JS implemented XPCOM class that I use here: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2138 and in nsAppRunner. So I can do all the work in JS but still use it from C++ and I can share the code between these two places so it will be less likely that someone breaks it accidentally. Anyone has a better plan or disagrees let me know.

One thing to keep in mind for both this bug and bug 1232274 is that the hotfix add-on, and experiment add-ons, need to be exceptions for these rules and not restart/disable e10s.

That sounds a great plan if it works. I'm not sure that by the time nsAppRunner.cpp::BrowserTabsRemoteAutostart() gets called for the first time, that you can already run XPCOM components (since it's called early by graphics code). If that works, perfect! Just keep in mind that, in one release, the code will change from "all add-ons" to an add-ons whitelist, so keep the code flexible to do this change easily.

(In reply to :Felipe Gomes (needinfo me!) from comment #10) > One thing to keep in mind for both this bug and bug 1232274 is that the > hotfix add-on, and experiment add-ons, need to be exceptions for these rules > and not restart/disable e10s. Thanks, I'll add that to the other exception, the "default theme addon" Jim mentioned. (In reply to :Felipe Gomes (needinfo me!) from comment #11) > That sounds a great plan if it works. I'm not sure that by the time > nsAppRunner.cpp::BrowserTabsRemoteAutostart() gets called for the first > time, that you can already run XPCOM components (since it's called early by > graphics code). If that works, perfect! I've just tested it and it seems to work. > Just keep in mind that, in one release, the code will change from "all > add-ons" to an add-ons whitelist, so keep the code flexible to do this > change easily. I keep all the XPI JSON data parsing + decision making logic in JS so it should be simple to modify it later.

Ideally this patch should be triggered by the same pref as yours under Bug 1232274, so it might makes sense to rename the pref. I'm not sure how to detect if an add-on is an experiment or does any of the locations I filter out cover that already? (based on your patch it seems like it just double checking it) Another thing I'm in trouble with is that your patch excludes a bunch of things (aAddon.type != "extension") which I cannot if I want to rely only on the pref alone. Should we start saving |type| into extensions.xpiState to solve this issue? Note: later on this will be extended into whitelisting add-ons that are e10s ready (or blacklisting the ones that are not).

[Tracking Requested - why for this release]: This is part of the e10s-selection code that we want to validate on the beta45 experiment to make sure it's ready for a 46 release.

Comment on attachment 8709063 [details] [diff] [review] disable e10s for users with 3rd party add-ons WIP Review of attachment 8709063 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13) > Created attachment 8709063 [details] [diff] [review] > disable e10s for users with 3rd party add-ons WIP > > Ideally this patch should be triggered by the same pref as yours under Bug > 1232274, so it might makes sense to rename the pref. I'm not sure how to > detect if an add-on is an experiment or does any of the locations I filter > out cover that already? (based on your patch it seems like it just double > checking it) Another thing I'm in trouble with is that your patch excludes a > bunch of things (aAddon.type != "extension") which I cannot if I want to > rely only on the pref alone. Should we start saving |type| into > extensions.xpiState to solve this issue? Note: later on this will be > extended into whitelisting add-ons that are e10s ready (or blacklisting the > ones that are not). Ugh, yeah this is getting complicated and adding more and more information into XPIState everytime we want to change what affects whether e10s can enable or not isn't scalable (XPIState is already larger than we should be storing in prefs). I think I have a better approach. Instead of checking all the installed add-ons on startup and using that to decide whether e10s can be used we instead maintain a single boolean flag saying whether e10s can be used and update that whenever the set of enabled add-ons changes. Keeping that in sync is the trick. With bug 1232274 no add-on can install without a restart happening. And the first restart after an install we always load the full add-ons database and by the time we get here https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProviderUtils.js#2003 we have the full data on all installed add-ons and so it's trivial to set the flag correctly at that point. We also go there on any Firefox version upgrade so users should hit that on the first update to a new beta. Then we just need to worry about add-ons enabling or disabling. Again with bug 1232274 that won't happen during runtime, instead it happens at shutdown and when it does we again have all the add-on data and run through it here: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProviderUtils.js#1392 So in both cases you just need to figure out whether e10s can be used and set the flag. You could just use a boolean pref in which case nsAppRunner.cpp could just read it directly. If you are worried about users tinkering with that in nsAppRunner (though whatever they do would get blown away anytime an add-on becomes enabled, disabled, installed or uninstalled) then we could add it into the XPIStates object (we might want to tinker with the schema there a bit) and add an addonsSupportE10S attribute to amIAddonManager.idl which can call through directly.

(In reply to Dave Townsend [:mossop] from comment #15) > Ugh, yeah this is getting complicated and adding more and more information > into XPIState everytime we want to change what affects whether e10s can > enable or not isn't scalable (XPIState is already larger than we should be > storing in prefs). If XPIState is too big we can always store it somewhere else no? Besides I don't think it's a huge scaling issue to add type and I don't see what else should we add to it later... The white listing mechanism would live in the XPIStateStorage.js file as static data, or could be fetched from somewhere else and stored the latest list somewhere. If XPIState is already too big for a pref we should store it somewhere else anyway. > > I think I have a better approach. Instead of checking all the installed > add-ons on startup and using that to decide whether e10s can be used we > instead maintain a single boolean flag saying whether e10s can be used and > update that whenever the set of enabled add-ons changes. > > Keeping that in sync is the trick. With bug 1232274 no add-on can install > without a restart happening. And the first restart after an install we > always load the full add-ons database and by the time we get here > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > internal/XPIProviderUtils.js#2003 we have the full data on all installed > add-ons and so it's trivial to set the flag correctly at that point. We also > go there on any Firefox version upgrade so users should hit that on the > first update to a new beta. Yes but that's too late already isn't it? By the time we reach the big add-ons database update and all, BrowserTabsRemoteAutostart has been already called and the decisions is made with the wrong flag. I don't want to delay the startup in that early stage too much so I'm not sure changing the order here is feasible. But if I'm wrong here, I'd love your version. If I'm right though, I don't see how this issue could be fixed simply. Do you? > > Then we just need to worry about add-ons enabling or disabling. Again with > bug 1232274 that won't happen during runtime, instead it happens at shutdown modulo crash :( but I'm fine disabling e10s when an add-on is installed vs when it's activated. > and when it does we again have all the add-on data and run through it here: > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > internal/XPIProviderUtils.js#1392 > > So in both cases you just need to figure out whether e10s can be used and > set the flag. You could just use a boolean pref in which case > nsAppRunner.cpp could just read it directly. If you are worried about users > tinkering with that in nsAppRunner (though whatever they do would get blown > away anytime an add-on becomes enabled, disabled, installed or uninstalled) > then we could add it into the XPIStates object (we might want to tinker with > the schema there a bit) and add an addonsSupportE10S attribute to > amIAddonManager.idl which can call through directly. I'm really not worried about users tinkering with the flag. That should not be a huge issue IMO if they cannot do it unintentionally.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16) > (In reply to Dave Townsend [:mossop] from comment #15) > > Ugh, yeah this is getting complicated and adding more and more information > > into XPIState everytime we want to change what affects whether e10s can > > enable or not isn't scalable (XPIState is already larger than we should be > > storing in prefs). > > If XPIState is too big we can always store it somewhere else no? Besides > I don't think it's a huge scaling issue to add type and I don't see what else > should we add to it later... The white listing mechanism would live in the > XPIStateStorage.js file as static data, or could be fetched from somewhere > else and stored the latest list somewhere. If XPIState is already too big > for a pref we should store it somewhere else anyway. Do you want to use the multiprocessCompatible flag for instance? Is it just a whitelist of IDs? > > I think I have a better approach. Instead of checking all the installed > > add-ons on startup and using that to decide whether e10s can be used we > > instead maintain a single boolean flag saying whether e10s can be used and > > update that whenever the set of enabled add-ons changes. > > > > Keeping that in sync is the trick. With bug 1232274 no add-on can install > > without a restart happening. And the first restart after an install we > > always load the full add-ons database and by the time we get here > > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > > internal/XPIProviderUtils.js#2003 we have the full data on all installed > > add-ons and so it's trivial to set the flag correctly at that point. We also > > go there on any Firefox version upgrade so users should hit that on the > > first update to a new beta. > > Yes but that's too late already isn't it? By the time we reach the big > add-ons > database update and all, BrowserTabsRemoteAutostart has been already called > and > the decisions is made with the wrong flag. I don't want to delay the startup > in that early stage too much so I'm not sure changing the order here is > feasible. > But if I'm wrong here, I'd love your version. If I'm right though, I don't > see how > this issue could be fixed simply. Do you? When is BrowserTabsRemoteAutostart called? Jimm gave me a stack earlier that suggests it is first called after the add-ons manager has already initialised. If it is called earlier than that then using XPIStates isn't going to work anyway because it won't be up to date with the current set of add-ons. > > Then we just need to worry about add-ons enabling or disabling. Again with > > bug 1232274 that won't happen during runtime, instead it happens at shutdown > > modulo crash :( but I'm fine disabling e10s when an add-on is installed vs > when it's activated. Crashes are handled by causing a rescan of add-ons and so we'll cat that too I think. > > and when it does we again have all the add-on data and run through it here: > > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > > internal/XPIProviderUtils.js#1392 > > > > So in both cases you just need to figure out whether e10s can be used and > > set the flag. You could just use a boolean pref in which case > > nsAppRunner.cpp could just read it directly. If you are worried about users > > tinkering with that in nsAppRunner (though whatever they do would get blown > > away anytime an add-on becomes enabled, disabled, installed or uninstalled) > > then we could add it into the XPIStates object (we might want to tinker with > > the schema there a bit) and add an addonsSupportE10S attribute to > > amIAddonManager.idl which can call through directly. > > I'm really not worried about users tinkering with the flag. That should not > be > a huge issue IMO if they cannot do it unintentionally.

(In reply to Dave Townsend [:mossop] from comment #17) > Do you want to use the multiprocessCompatible flag for instance? Is it just > a whitelist of IDs? I would guess version number is sufficient. But good point. > When is BrowserTabsRemoteAutostart called? Jimm gave me a stack earlier that > suggests it is first called after the add-ons manager has already > initialised. If it is called earlier than that then using XPIStates isn't > going to work anyway because it won't be up to date with the current set of > add-ons. Let me try to write a new patch tomorrow with your approach and I'll see how far I can get with it. It surely would simplify things. And thanks for the help!

Now I see this flag in the code I understand it won't make possible future test changes difficult. :) (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16) > Yes but that's too late already isn't it? By the time we reach the big > add-ons > database update and all, BrowserTabsRemoteAutostart has been already called > and > the decisions is made with the wrong flag. I don't think it matters that the first session would have e10s disabled.

One request that came out in a meeting today is that, even when the pref introduced is off, we should still run the logic and add some annotation to the crash reporter meaning "this profile would have been blocked from e10s if add-ons blocking was active".

You were totally right about the orders, so this version is a lot simpler. It is based on your patch so I could share the logic that decides if an add-on can block e10s. There are only a few things: (In reply to Dave Townsend [:mossop] from comment #15) > Then we just need to worry about add-ons enabling or disabling. Again with > bug 1232274 that won't happen during runtime, instead it happens at shutdown > and when it does we again have all the add-on data and run through it here: > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > internal/XPIProviderUtils.js#1392 Is this part really needed? The patch seems to be working fine without it. Since when it matters there is always a restart, and we always hit the logic that scans add-ons and makes the decision. I left this part out but I might have missed something that you have in mind and why this is important... (In reply to Dave Townsend [:mossop] from comment #17) > > Crashes are handled by causing a rescan of add-ons and so we'll cat that too > I think. This part seemed to be buggy to me. When it's crashing after an add-on was disabled, and I restart ff, then it starts up the disabled add-on while it shows that it's disabled in the add-on manager... But I guess this is a separate bug. And last: is there a way to write test for this? It would require a browser restart...

Comment on attachment 8709970 [details] [diff] [review] disable e10s for users with 3rd party add-ons v2 Review of attachment 8709970 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, I'll reserve final r+ till I've seen the tests though. (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #21) > Created attachment 8709970 [details] [diff] [review] > disable e10s for users with 3rd party add-ons v2 > > You were totally right about the orders, so this version is a lot simpler. > It is based on your patch so I could share the logic that decides if an > add-on can block e10s. There are only a few things: > > (In reply to Dave Townsend [:mossop] from comment #15) > > Then we just need to worry about add-ons enabling or disabling. Again with > > bug 1232274 that won't happen during runtime, instead it happens at shutdown > > and when it does we again have all the add-on data and run through it here: > > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > > internal/XPIProviderUtils.js#1392 > > Is this part really needed? The patch seems to be working fine without it. > Since when it matters there is always a restart, and we always hit the logic > that scans add-ons and makes the decision. I left this part out but I might > have missed something that you have in mind and why this is important... In theory we're not meant to go through that path in startup unless a new add-on has been installed or a previous add-on has been uninstalled or the application has changed. In practice it is possible that we might hit it when an add-on changes state because of a bug in how XPIStates compares the set of add-ons and I may fix that at some point elsewhere. For now though as long as we have a test verifying the pref is correct in the case where the only change is an add-on enabling/disabling that will be enough for an r+ > (In reply to Dave Townsend [:mossop] from comment #17) > > > > Crashes are handled by causing a rescan of add-ons and so we'll cat that too > > I think. > > This part seemed to be buggy to me. When it's crashing after an add-on was > disabled, and I restart ff, then it starts up the disabled add-on while it > shows that it's disabled in the add-on manager... But I guess this is a > separate > bug. Hmm, that's not great, we used to be resilient to that. I guess file a bug and we can figure out how difficult that will be to fix separately. > And last: is there a way to write test for this? It would require a browser > restart... The extension manager xpcshell tests simulate restarts well enough that we'll be able to verify the pref is getting set correctly. See an example in the other bug. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm @@ +4238,5 @@ > } > }, > > /** > + * Determins if an add-on should be blocking e10s if enabled. s/Determins/Determine/ ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js @@ +2017,5 @@ > let isActive = !currentAddon.disabled; > let wasActive = previousAddon ? previousAddon.active : currentAddon.active > > + if (e10sBlockEnabled && isActive && XPIProvider.isBlockingE10s(currentAddon)) > + blockE10s = true; I think from felipe's comment you want to set this regardless of e10sBlockEnabled, then you'll get the annotations in nsAppRunner working.

(In reply to Dave Townsend [:mossop] from comment #22) > Looks good, I'll reserve final r+ till I've seen the tests though. Whoops, I meant to set the feedback flag anyway :)

As discussed on IRC the last test which is uninstalling the addon then after a restart checking the flag seems to be broken. It should work, but I guess the testing env is a bit different than the browser. Do you know what might be going on?

Comment on attachment 8710153 [details] [diff] [review] WIP Review of attachment 8710153 [details] [diff] [review]: ----------------------------------------------------------------- Looks like I was wrong and we do clear XPIStates when uninstalling a restartless add-on so at that point you'll have to check the list of active add-ons again and build the flag. It happens after this point https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4922 You'll need to call the code in two places, both as an else clause from this: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4900 and after this block https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4878

Comment on attachment 8710394 [details] [diff] [review] disable e10s for users with 3rd party add-ons v4 Review of attachment 8710394 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/test/xpcshell/test_e10s_restartless.js @@ +191,5 @@ > Services.prefs.setCharPref("extensions.hotfix.id", ID); > > yield check_normal(); > }); > + Ehh... seems like I've added an extra new line, I've fixed this locally.

Comment on attachment 8710394 [details] [diff] [review] disable e10s for users with 3rd party add-ons v4 Review of attachment 8710394 [details] [diff] [review]: ----------------------------------------------------------------- I've realised that we can be even safer here. Instead of calling updateAddonsBlockingE10s every time we think the state might have changed on uninstall or enable/disable we can just call it every time we successfully flush the database to disk. So just add a then to this promise https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProviderUtils.js#463 which calls updateAddonsBlockingE10s. I'd like to see a couple more tests for the case where there are two add-ons installed. Basically the copy the set you added but have a second add-on installed and do the checks once when it is enabled (the flag should always be blocking) and once when it is disabled (the flag should do the same as your existing test). You can just use writeInstallRDFForExtension to add an extension to the profile without needing to go through the install process. ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js @@ +2184,5 @@ > XPIDatabase.migrateData = null; > XPIDatabase.saveChanges(); > > + XPIDatabase.updateAddonsBlockingE10s(); > + You should still call updateAddonsBlockingE10s here though since saveChanges is asynchronous and we'll need the pref to be set correctly immediately after any changes were made by startup.

I've added the test and as we discussed on IRC made the call independent from the success of the save promise.

Comment on attachment 8710592 [details] [diff] [review] disable e10s for users with 3rd party add-ons v5 Review of attachment 8710592 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js @@ +1397,5 @@ > + // force the DB to load > + AddonManagerPrivate.recordSimpleMeasure("XPIDB_lateOpen_updateActive", > + XPIProvider.runPhase); > + this.syncLoadDB(true); > + } This shouldn't be necessary now, saveChanges already protects against calls when the DB isn't already loaded. ::: toolkit/mozapps/extensions/test/xpcshell/test_e10s_restartless.js @@ +272,5 @@ > + > + // After uninstall the only enabled addon and restart we should not block. > + blocked = Services.prefs.getBoolPref("extensions.e10sBlockedByAddons"); > + do_check_false(blocked); > + Please also uninstall addon2 here just to leave a clean slate if we add future tests to this file.

This is going to need a rebased patch for Aurora too.

There was a failure: TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_startup.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_startup.js | check_startup_changes - [check_startup_changes : 750] "[\\"addon2@tests.mozilla.org\\"]" == "[]" TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_startup.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_startup.js | check_startup_changes - [check_startup_changes : 750] "[\\"addon2@tests.mozilla.org\\"]" == "[]" Return code: 1 aAddon._installLocation was not undefined. I've added a guard against this and re-trying: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b731e73d64fb

Approval Request Comment [Feature/regressing bug #]: none [User impact if declined]: we need this for e10s beta experiment/roll-out [Describe test coverage new/current, TreeHerder]: includes tests [Risks and why]: very low risk, the patch is preffed of, another one will turn it on [String/UUID change made/needed]: none

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #33) > aAddon._installLocation was not undefined. I've added a guard against this > and re-trying: > remote: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=b731e73d64fb This is probably happening because we call XPIDatabase.saveChanges() before fully fixing up the database from local file changes during startup. Arguably the right thing is to actually make add-ons with no location not block e10s since they will get cleaned up later on in startup but it doesn't really matter since we'll save changes after that anyway.

Comment on attachment 8710950 [details] [diff] [review] rebased to aurora Looks good on Try.

Comment on attachment 8710950 [details] [diff] [review] rebased to aurora Taking it for 45, thanks.

[bugday-20160323] Status: RESOLVED,FIXED -> UNVERIFIED Comments: STR: Not clear. Developer specific testing Component: Name Firefox Version 46.0b9 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: Developer specific testing Actual Results: As expected