1354602 - Expose enabling containers via web extensions

Status: VERIFIED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Jonathan Kingston [:jkt] he/him]

Currently users have to enable containers manually to have the containers APIs, it would be convenient if an extension could turn on the relevant prefs via browser.privacy api.

This is actually a compat issue with keeping the experiment open after 57.

Comment 1

[Andy McKay]

We'd need an a+ from someone like baku to doing that. 

It would be pretty easy to add to the privcy API but we'd much rather turn on containers for all users. Having features that aren't completely baked in release flipped by WebExtensions isn't something we want to encourage, we'd rather have the feature be fully formed and supported. It feels like we've done this in the past and we've ended up with things that people rely on, but aren't really supported.

Comment 2

[Jonathan Kingston [:jkt] he/him]

Will speak to Baku about this, he mentioned in the previous meeting that potentially we could expose the pref only for Chrome level extensions. This would potentially be a migration plan for the Test pilot to keep it going post 57 deprecation etc.

Comment 3

[Andy McKay]

Test Pilot extensions will be able to survive post 57, ping clouserw for more information on that.

Comment 4

[John Gruen [:jgruen]]

Andy, can you provide a specific use case that you'd like to achieve with this work.

Comment 5

[Jonathan Kingston [:jkt] he/him]

As Andy explained at the work week his intentions are clear. Provide a way for extensions to enable containers. The UI should be just the default tab highlights etc that are enabled with the test pilot. I'm not sure if it even makes sense to enable about:preferences containers work.

The way suggested was any extension with "contextualIdentities" permission would enable containers much like how sidebars enable the icon.

Comment 6

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1354602 - Enabling containers for container addons on startup.

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

Comment 7

[Jonathan Kingston [:jkt] he/him]

The attached patch is a WIP as I need to account for removing of all container addons and tests etc.

:aswan is this the correct direction for enabling the container pref/s though?

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

https://reviewboard.mozilla.org/r/154078/#review159968

This is on the right track for actually setting the preference, but what happens when the extension is uninstalled (or disabled)?  What if I have two different extensions installed that both use containers and I uninstall one of them?  Given that this preference can also be manually changed in about:preferences there is no great answer, but I think that using `ExtensionSettingsStore.jsm` so we at least have the same behavior as other extensions that override preferences would be good.

Comment 9

[Andrew Swan [:aswan]]

Oh whoops, I just saw comment 7 after writing the review comments...

Comment 10

[Jonathan Kingston [:jkt] he/him]

When loading a temporary extension in web-ext and using the following code in my ExtensionAPI:

  async onShutdown(shutdownReason) {
    let {extension} = this;

    switch (shutdownReason) {
      case "ADDON_DISABLE":
      case "ADDON_DOWNGRADE":
      case "ADDON_UPGRADE":
      case "ADDON_UNINSTALL":
        ExtensionPreferencesManager.removeSetting(extension, CONTAINERS_ENABLED_SETTING_NAME);
        break;
    }
    let control = await ExtensionPreferencesManager.getLevelOfControl(extension, CONTAINERS_ENABLED_SETTING_NAME);
    if (control !== "controlled_by_other_extensions") {
      ContextualIdentityService.closeContainerTabs();
    }
  }


I am seeing the following in my browser toolbox:

TypeError: parentDocShell.getDocShellEnumerator is not a function[Learn More]  tab.js:62:23
TypeError: addon is null[Learn More]  ExtensionSettingsStore.jsm:404:5

Where 403 is:
http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionSettingsStore.jsm#403

I am calling this on uninstall of the extension however I needed to calculate the current owner of the property to see if I need to do cleanup, I don't think there is another way to do this.


What I would really like is:

ExtensionPreferencesManager.addSetting(CONTAINERS_ENABLED_SETTING_NAME, {
  prefNames: Object.keys(CONTAINER_PREF_INSTALL_DEFAULTS),

  removeCallback() {
    // called when no other extensions are controlling this setting
    ContextualIdentityService.closeContainerTabs()
  }
   
  ...
});

This would reduce race conditions and also prevent issues with disabling etc (I'm not sure if I need to handle that in the above code etc).

My question is can I add this callback, it doesn't seem like too much work and useful for others perhaps?

The other issue I am having is it seems that when I close the browser with temorary addons installed they are never removed from the list and so I never get this setting disabled in future.

Comment 11

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154078/diff/1-2/

Comment 12

[Jonathan Kingston [:jkt] he/him]

I added an example patch again with a new callback on the PreferenceManager for setting to initial as spoken about in: https://bugzilla.mozilla.org/show_bug.cgi?id=1354602#c10

Is there another way to do this instead perhaps?

If this is ok I can go ahead and add tests as this appears to be working as expected.

Comment 13

[Bob Silverberg [:bsilverberg]]

(In reply to Jonathan Kingston [:jkt] from comment #10)
> When loading a temporary extension in web-ext and using the following code
> in my ExtensionAPI:
> 
>   async onShutdown(shutdownReason) {
>     let {extension} = this;
> 
>     switch (shutdownReason) {
>       case "ADDON_DISABLE":
>       case "ADDON_DOWNGRADE":
>       case "ADDON_UPGRADE":
>       case "ADDON_UNINSTALL":
>         ExtensionPreferencesManager.removeSetting(extension,
> CONTAINERS_ENABLED_SETTING_NAME);
>         break;
>     }

I'm not sure the above makes sense. Generally we disable a setting when an extension is disabled, we don't remove it. Depending on your code that adds the setting on startUp, you might also want to disable (rather than remove) on UPGRADE and DOWNGRADE as well.

>     let control = await
> ExtensionPreferencesManager.getLevelOfControl(extension,
> CONTAINERS_ENABLED_SETTING_NAME);
>     if (control !== "controlled_by_other_extensions") {
>       ContextualIdentityService.closeContainerTabs();
>     }
>   }
> 
> 
> I am seeing the following in my browser toolbox:
> 
> TypeError: parentDocShell.getDocShellEnumerator is not a function[Learn
> More]  tab.js:62:23
> TypeError: addon is null[Learn More]  ExtensionSettingsStore.jsm:404:5
>

It sounds like the add-on is already removed by the time this code runs, which would explain the error. 
 
> Where 403 is:
> http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> ExtensionSettingsStore.jsm#403
> 
> I am calling this on uninstall of the extension however I needed to
> calculate the current owner of the property to see if I need to do cleanup,
> I don't think there is another way to do this.
> 
> 
> What I would really like is:
> 
> ExtensionPreferencesManager.addSetting(CONTAINERS_ENABLED_SETTING_NAME, {
>   prefNames: Object.keys(CONTAINER_PREF_INSTALL_DEFAULTS),
> 
>   removeCallback() {
>     // called when no other extensions are controlling this setting
>     ContextualIdentityService.closeContainerTabs()
>   }
>    
>   ...
> });
> 
> This would reduce race conditions and also prevent issues with disabling etc
> (I'm not sure if I need to handle that in the above code etc).
> 
> My question is can I add this callback, it doesn't seem like too much work
> and useful for others perhaps?
> 

It sounds like you just need to know if _any_ extensions are currently controlling the setting, so perhaps we can add a method to the SettingsStore to return that information. I think that would be a simpler API than introducing a callback.

> The other issue I am having is it seems that when I close the browser with
> temorary addons installed they are never removed from the list and so I
> never get this setting disabled in future.

This is a known bug which is being tracked via bug 1359558.

Comment 14

[Bob Silverberg [:bsilverberg]]

I wrote my comment before seeing your latest patch, but I'm still not crazy about the `resetToInitialCallback` thing.

Comment 15

[Jonathan Kingston [:jkt] he/him]

I could instead use:

async onShutdown(shutdownReason) {
  // bikesheding of name needed
  if (ExtensionPreferencesManager.isAnyOfSettingUsed(CONTAINERS_ENABLED_SETTING_NAME)) {
    ContextualIdentityService.closeContainerTabs()
  }
}

My worry with this however is that onShutdown might fire before the removeAll does as part of the WebExtension code.

Where as:

  resetToInitialCallback() {
    ...
  }

This is only called when the queue doesn't have any enabled items after removeAll happens.

My other ideas are to change resetToInitialCallback to just something that always fire on state change and also provide a method to get the top of the queue in ExtensionPreferencesManager so something like:

  onStateChange(value) {
    if (!ExtensionPreferencesManager.getTopItem()) {
      // remove
    }
  }

Or can we guarantee that onShutdown in the ExtensionAPI will call after all the settings have been removed?

Comment 16

[Bob Silverberg [:bsilverberg]]

(In reply to Jonathan Kingston [:jkt] from comment #15)
> I could instead use:
> 
> async onShutdown(shutdownReason) {
>   // bikesheding of name needed
>   if
> (ExtensionPreferencesManager.
> isAnyOfSettingUsed(CONTAINERS_ENABLED_SETTING_NAME)) {
>     ContextualIdentityService.closeContainerTabs()
>   }
> }
> 
> My worry with this however is that onShutdown might fire before the
> removeAll does as part of the WebExtension code.
> 
> Where as:
> 
>   resetToInitialCallback() {
>     ...
>   }
> 
> This is only called when the queue doesn't have any enabled items after
> removeAll happens.

Yes, I can see your point here.

> 
> My other ideas are to change resetToInitialCallback to just something that
> always fire on state change and also provide a method to get the top of the
> queue in ExtensionPreferencesManager so something like:
> 
>   onStateChange(value) {
>     if (!ExtensionPreferencesManager.getTopItem()) {
>       // remove
>     }
>   }
> 

One thing I'm not clear on is why you need to base this on values in the settings store at all. Can you not base it on the current values of the prefs?

> Or can we guarantee that onShutdown in the ExtensionAPI will call after all
> the settings have been removed?

I think that's a question of whether stuff in Management.on("shutdown") is guaranteed to run before (or after) stuff in onShutdown(). I'm not sure of the answer to that, but you could do a quick test to see in what order they seem to fire. If Management.on("shutdown") seems to fire before onShutdown() we can then verify whether that is guaranteed to happen.

Comment 17

[Jonathan Kingston [:jkt] he/him]

Hmm in that case I might have been over complicating it, I thought we were trying to be careful about listening to settings over preferences for this.

I could possibly just use a preference observer in the contextual identity service even.

:baku are you ok with us moving the pref changing code in about:preferences to actually be in the service?
So changing about:preferences or extension state would delete all custom containers, clear tabs and cookies and reinstate the default ones like our extension did?

Comment 18

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154078/diff/2-3/

Comment 19

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aecf7f7c0aea1d536e135b201a554b7b02630468

Comment 20

[Jonathan Kingston [:jkt] he/him]

So I pushed another patch which doesn't add a callback and instead just observes a pref for all containers code. I'm still in need of clearing up containers that the addon has added when removing the containers and reinstating the initial ones.

However I'm getting a few failures in my test that doesn't really make sense as manually testing it works.

The first error I can't seem to remove is:

> [test_contextualIdentity_with_permissions : 265] A promise chain failed to handle a rejection: AddonManager is not initialized - stack: _do_main@/home/jonathan/mozilla/firefox/testing/xpcshell/head.js:221:3

The other errors might be race conditions on the fact the pref hasn't been set perhaps I will look into that next, I just wanted to upload to try and review if anyone else know about the AddonManager bug.

:baku as mentioned in Bug 1368815 I have removed the tab clearing from the preference panels as there will likely be a few places where containers get disabled so using a pref observer.

Comment 21

[Jonathan Kingston [:jkt] he/him]

Also it might be worth removing the return false; that was added when containers are disabled (this for me is what the API is getting stuck on in my tests). Instead replace it with throw as it shouldn't ever happen that an extension has these methods when containers is disabled.

Comment 22

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdc37a3bb02dca82bf3f3607d8edd6027484736f

Comment 23

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154078/diff/3-4/

Comment 24

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154078/diff/4-5/

Comment 25

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154078/diff/5-6/

Comment 26

[Jonathan Kingston [:jkt] he/him]

:mconley I know you are super busy but I added you to the review because of Bug 1368815 where we discussed the pref observer.

I also wanted to highlight the private containers we have currently one which is managed in the identity migrations etc. If we added an internal API for chrome to add private containers some of this code may break due to resetting the containers. Should we cope for this case or is it clear enough with the current code?

:aswan I think I have addressed all comments here by you and bob, so ready for a web-ext peer to review :).

Thanks!

Comment 27

[Andrew Swan [:aswan]]

Adding :zombie to get some slightly quicker review on the webextensions bits

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

https://reviewboard.mozilla.org/r/154078/#review169018

Thanks jkt - r=me for the toolkit bits, though I have some questions / suggestions below.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:119
(Diff revision 6)
>    _saver: null,
>  
>    init(path) {
>      this._path = path;
> +
> +    Services.prefs.addObserver(CONTEXTUAL_IDENTITY_ENABLED_PREF, this);

I guess there's no need to unregister this observer in some kind of shutdown?

I'm not sure if our shutdown leak detector clears out services like this and the pref service or not... maybe do a debug try push to see if any shutdown leaks are detected.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:124
(Diff revision 6)
> +    const contextualIdentitiesEnabled = Services.prefs.getBoolPref(CONTEXTUAL_IDENTITY_ENABLED_PREF);
> +    if (!contextualIdentitiesEnabled) {
> +      await this.closeContainerTabs();
> +      this.notifyAllContainersCleared();
> +      this.resetDefault();
> +    }

I'd be a little worried about re-entrancy here, since someone could (in theory) flip that pref back and forth a lot while you're awaiting... but I don't think that'd produce any issues here, at least as far as I understand the contextual identity service.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:165
(Diff revision 6)
> +    this._defaultIdentities.forEach((identity) => {
> +      this._identities.push(Object.assign({}, identity));
> +    });

I've heard recently that the forEach loop is the slowest of the for loops... with the ol' C-style for being the fastest, and for of / for in being somewhere in the middle. Is this a hot code path, or something we need to be blazing fast? If so, maybe consider switching this to a for of or a C-style for loop, but I won't harp on it. Feel free to drop.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:169
(Diff revision 6)
> -    this._lastUserContextId = this._defaultIdentities.length;
> +    // Clone the array
> +    this._defaultIdentities.forEach((identity) => {
> +      this._identities.push(Object.assign({}, identity));
> +    });
> +    this._lastUserContextId = this._identities.length;
> +    this._openedIdentities = new Set();

This doesn't appear to be used.

Comment 29

[Tomislav Jovanovic :zombie]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

https://reviewboard.mozilla.org/r/154078/#review168868

I have a few questions, some improvements to the tests, and a general question (perhaps for Andrew):

 - Isn't using the ExtensionPreferencesManager for this a bit of an overkill?  There are no different values per extension.  All you need to keep track of is the count of active extensions with the permission.  Increase it on startup, and set the pref if was 0; decrease it on shutdown, and remove/reset the pref if it reaches 0.  I think we do we do stuff like this regularly, for example for menus [1].

1) http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-menus.js#619-631

::: toolkit/components/extensions/ext-contextualIdentities.js:51
(Diff revision 6)
> +
>  this.contextualIdentities = class extends ExtensionAPI {
> +  onStartup() {
> +    let {extension} = this;
> +
> +    if (extension.hasPermission("contextualIdentities")) {

Can this be an optional permission?  A general principle is that if something can, it should be optional.

::: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js:24
(Diff revision 6)
> -  await extension.unload();
> -
> -  Services.prefs.clearUserPref("privacy.userContext.enabled");
> -});
> -
>  add_task(async function test_contextualIdentity_with_permissions() {

The name of the test is not clear, either improve it or comment above what it's meant to test.

::: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js:86
(Diff revision 6)
>      browser.test.assertEq(4, cis.length, "we are back to 4 identities");
>  
>      browser.test.notifyPass("contextualIdentities");
>    }
>  
> +  Services.prefs.setBoolPref("privacy.userContext.enabled", false);

I'd rather you just assert the pref is disabled by default.  That way, if we decide to set the default pref later, the test will break right at the start, giving you clear indication what's wrong, rather than in the middle once you remove the user pref.

::: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js:99
(Diff revision 6)
>    await extension.startup();
> +  browser.test.assertTrue(Services.prefs.getBoolPref("privacy.userContext.enabled"), "Pref should now be enabled");
>    await extension.awaitFinish("contextualIdentities");
>    await extension.unload();
>  
> +  Services.prefs.setBoolPref("privacy.userContext.enabled", true);

You should test here that the preference was removed/reset to default after the extension is unloaded.  Also after the second extension.

::: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js:110
(Diff revision 6)
> +    },
> +  });
> +  await extension2.startup();
> +  await extension2.awaitFinish("contextualIdentities");
> +  await extension2.unload();
> +

Might be a good idea to test with another extension3, installed and unloaded while extension2 is running, and test expected values of the prefs at appropriate steps.

Comment 30

[Jonathan Kingston [:jkt] he/him]

> Can this be an optional permission?  A general principle is that if something can, it should be optional.

Never used optionals, will look into how that works. Currently not clear how the optionals get activated for example.


> I'd rather you just assert the pref is disabled by default.  That way, if we decide to set the default pref later, the test will break right at the start, giving you clear indication what's wrong, rather than in the middle once you remove the user pref.


Not sure we can sanely do this we have a different default for this in Nightly than other releases.

Comment 31

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

https://reviewboard.mozilla.org/r/154078/#review169018

> I'd be a little worried about re-entrancy here, since someone could (in theory) flip that pref back and forth a lot while you're awaiting... but I don't think that'd produce any issues here, at least as far as I understand the contextual identity service.

Currently this pref is only going to be changed when users flip the pref in about:config / about:preferences or remove their last container addon.

Besides debouncing or using some kind of queue here I'm not sure there is an easy solution that would cater for the await. The problem you are talking about would also have to cope for the positive case to reset to stop the resetting.

Would adding a timeout to closeContainerTabs be worthwhile here? It was a hard problem that I wasn't aware of a decent solution for essentially an edge case. If there is something obvious I am missing let me know.

> I've heard recently that the forEach loop is the slowest of the for loops... with the ol' C-style for being the fastest, and for of / for in being somewhere in the middle. Is this a hot code path, or something we need to be blazing fast? If so, maybe consider switching this to a for of or a C-style for loop, but I won't harp on it. Feel free to drop.

Changed it anyways, it's not a hot code path though. Again it should hopefully be super rare that users disable containers expecting to renable and disable again.

> This doesn't appear to be used.

This is used by our telemetry http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5271 to count the containers that they used. We delete containers in this set when the user has removed a container, this action should behave the same.

Comment 32

[Tomislav Jovanovic :zombie]

(In reply to Jonathan Kingston [:jkt] from comment #30)
> Never used optionals, will look into how that works. Currently not clear how
> the optionals get activated for example.

Sorry I didn't make this clear, I didn't intend to feature-creep this bug.. ;)

I was just asking if this permission _could_ be made optional (in another bug if we decide to), and to check if what you are doing here in this bug might prevent that / make it more complicated.


> Not sure we can sanely do this we have a different default for this in
> Nightly than other releases.

Ok, I consulted with Bob what ExtensionPreferencesManager actually does, and since it restores the previously set pref, I think what you did is fine.

Comment 33

[Jonathan Kingston [:jkt] he/him]

> Sorry I didn't make this clear, I didn't intend to feature-creep this bug.. ;)

Cool filed a follow up for this: Bug 1386673

Thanks for the follow up :zombie.

Comment 34

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0a58d93d840679aa025470394d57526207c1e9a

Comment 35

[Jonathan Kingston [:jkt] he/him]

Hey,

I'm not sure if I have found a bug in the ExtensionSettingsStore or one in my code.

The setting of the pref here:
https://hg.mozilla.org/try/rev/e2ba76ae0e035302558ebd52089e5205c999f5c0#l6.128

Then checking it is back into that state doesn't seem to work in test code:
https://hg.mozilla.org/try/rev/e2ba76ae0e035302558ebd52089e5205c999f5c0#l6.140

Didn't work, however I couldn't replicate this manually.


Going to double check my own code but just wanted to ask if you think this is bad.

Thanks
Jonathan

Comment 36

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154078/diff/6-7/

Comment 37

[Tomislav Jovanovic :zombie]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

https://reviewboard.mozilla.org/r/154078/#review170888

LGTM.  Please add Andrew for final review.

Comment 38

[Andrew Swan [:aswan]]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

https://reviewboard.mozilla.org/r/154078/#review172764

::: toolkit/components/extensions/ext-contextualIdentities.js:12
(Diff revision 7)
> +  "privacy.userContext.enabled": true,
> +  "privacy.userContext.longPressBehavior": 2,
> +  "privacy.userContext.ui.enabled": true,
> +  "privacy.usercontext.about_newtab_segregation.enabled": true,

Those mixed cases are kind of annoying...

::: toolkit/components/extensions/ext-toolkit.js:111
(Diff revision 7)
>    },
>    contextualIdentities: {
>      url: "chrome://extensions/content/ext-contextualIdentities.js",
>      schema: "chrome://extensions/content/schemas/contextual_identities.json",
>      scopes: ["addon_parent"],
> +    events: ["startup"],

Hm, this forces this module to be loaded even for extensions that don't use containers.  But it looks like we don't actually have a way to avoid this right now, ugh.

::: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js:117
(Diff revision 7)
> +  let extension2 = makeExtension("[e-mail]");
> +  let extension3 = makeExtension("[e-mail]");

It looks like this re-runs all the tests in the background script.  That's not harmful but does it actually give us an extra test coverage?  If not, perhaps we could use a simpler extension that has the "contextualIdentities" permission for this part of the test?

Comment 39

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

https://reviewboard.mozilla.org/r/154078/#review169018

> I guess there's no need to unregister this observer in some kind of shutdown?
> 
> I'm not sure if our shutdown leak detector clears out services like this and the pref service or not... maybe do a debug try push to see if any shutdown leaks are detected.

This looks like it isn't an issue from the try pushes I was using, we can always add in a shutdown observer later perhaps but it appears to be handled correctly.

Comment 40

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154078/diff/7-8/

Comment 41

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154078/diff/8-9/

Comment 42

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d4962958db6f -d 3323bbf5cea8: rebasing 413209:d4962958db6f "Bug 1354602 - Enabling containers for container addons on startup. r=aswan,mconley,zombie" (tip)
merging toolkit/components/contextualidentity/ContextualIdentityService.jsm
merging toolkit/components/extensions/ext-contextualIdentities.js
merging toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js
warning: conflicts while merging toolkit/components/extensions/ext-contextualIdentities.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 43

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154078/diff/9-10/

Comment 44

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ca488be2d1cbf0d26d4a4c80b73da9355d65d05

Comment 45

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bbf113b2cdbc27b1b2af21209ca8e691978b3e9

Comment 46

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55ce774574511f30791403d8833c3480cb973460

Comment 47

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdb64c5bb5df23959bdc94f00660c26df4d52a08

Comment 48

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8883139 [details]
Bug 1354602 - Enabling containers for container addons on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154078/diff/10-11/

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33a59196dc70
Enabling containers for container addons on startup. r=aswan,mconley,zombie

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/33a59196dc70

Comment 51

[Will Bamberg [:wbamberg]]

I've added a note about this here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextualIdentities

"Before Firefox 57, the contextualIdentities API is only available if the contextual identities feature is itself enabled. From Firefox 57 onwards, if an extension that uses the contextualIdentities API is installed, then the contextual identities feature will be enabled automatically."

Please let me now if that covers it.

As far as I can see, there's no actual WebExtension API change here, right?

Comment 52

[Will Bamberg [:wbamberg]]

Marking dev-doc-complete, see bug 1395659, comment 20.

Comment 53

[Jonathan Kingston [:jkt] he/him]

Sorry for the delay, looks good :wbamberg. Thanks for this!

Comment 54

[CosminB]

Attachment: Desktop.zip

I can reproduce this issue on Firefox 56.0.1 (20171002220106) under Wind 7 64-bit.
After installing the extensions that have containers, they will not be displayed when trying to see them.

This issue is verified as fixed on Firefox 57.0b11 (20171023180840) and Firefox 58.0a1 (20171023220222) under Wind 7 64-bit and Mac OS X 10.13.
After install, the containers from the extensions are visible in the extension’s panel.

Please see the attached videos.