1364895 - ContextualIdentities API isn't reloaded when containers are enabled

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Jonathan Kingston [:jkt] he/him]

- Should browser.contextualIdentities even be a key of browser?
- When contextualIdentities is enabled the API should be enabled.

STR:

1. Set privacy.userContext.enabled false
2. Install extension
3. Set privacy.userContext.enabled true
4. Use browser.contextualIdentities


STR requires an extension that checks this API again in 4. which mine does not due to this issue.

I'm not sure if we want some form of onEnabled/onDisabled event as hopefully containers will become a default eventually.

---

Currently my extension pretty much requires containers to function correctly:

https://addons.mozilla.org/en-GB/firefox/addon/sea-containers/

I have added a link to install containers and when the user installs the browser.contextualIdentities API is still undefined.

Comment 1

[Andy McKay]

That would be up to baku, so pinging for an opinion.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: extension.patch

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8870787 [details] [diff] [review]
extension.patch

Review of attachment 8870787 [details] [diff] [review]:
-----------------------------------------------------------------

Please use XPCOMUtils.defineLazyPreferenceGetter() instead of Preferences.get()

Comment 4

[Andrea Marchesini [:baku]]

This is exactly what I have done. I just expose an object called 'Preferences'.

Comment 5

[Andrew Swan [:aswan]]

Not defineLazyModuleGetter, defineLazyPreferenceGetter:
http://searchfox.org/mozilla-central/rev/35b37316149108d53a02fb8498e250ea8a22ab5d/js/xpconnect/loader/XPCOMUtils.jsm#286-306

Comment 6

[Andrea Marchesini [:baku]]

Attachment: extension.patch

Ah sorry! My fault. Here a new patch.

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8871327 [details] [diff] [review]
extension.patch

Review of attachment 8871327 [details] [diff] [review]:
-----------------------------------------------------------------

The implementation looks good but is this really the desired behavior?  Will users of the API be frustrated by for instance, the inability to tell if query() returned an empty array since the query didn't match versus because containers are disabled?  Sorry for not raising that in the first pass, if this behavior is desired (eg we don't want to expose to extensions whether they are enabled?) then r=me.

Comment 8

[Andrea Marchesini [:baku]]

> The implementation looks good but is this really the desired behavior?  Will

The reason why I wrote this patch is because often, containers are enabled/disabled. If a webextension uses the contextualIdentity API, when containers are enabled again, browser.contextualIdentities is not available because we don't refresh the permission/manifest. Better to have this approach how implemented here.

jkt, feedback?

Comment 9

[Jonathan Kingston [:jkt] he/him]

Can we change the return if the user has containers disabled? That way the user can at least check and cope with this?
Options:
- return false
- throw
- return a uniqueId we could provide a constant for: browser.contextualIdentities.IS_DISABLED

Alternatively can we somehow expose something which indicates if containers is enabled?

browser.contextualIdentities.isDisabled(); // True or false


Personally I would suggest returning false is the most friendly and also gives a clear signal containers has been disabled, this would also be the minimal amount of code to remove if/when containers is always on.

Comment 10

[Andrea Marchesini [:baku]]

Good to me with returning 'false'.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be95ec17a58f
Expose contextualIdentities to webExtensions also if privacy.userContext.enabled is set to false, r=aswan

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be95ec17a58f