No contextualIdentity events

VERIFIED FIXED in Firefox 57

Status

enhancement
P3
normal
VERIFIED FIXED
2 years ago
11 months ago

People

(Reporter: andy+bugzilla, Assigned: jkt)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [contextualIdentities], triaged)

Attachments

(2 attachments)

Reporter

Description

2 years ago
Currently I can query to find out details about a contextualIdentity, but I can't tell when they change in a webExtension. In my extension I list all the tabs and show the colour of a container. However if a user goes to "Manage containers" and changes the colour from yellow to orange... the WebExtension won't know without querying again.

The standard way to do this is some events like:

contextualIdentities.onCreated
contextualIdentities.onChanged
contextualIdentities.onRemoved

The extension can then addListeners if they'd like to find out when they change.

Updated

2 years ago
Priority: -- → P3
Whiteboard: [contextualIdentities] → [contextualIdentities], triaged
Assignee

Updated

2 years ago
Assignee

Comment 1

2 years ago
There isn't any implications in adding this is there?
This would be useful in other extensions outside the Test Pilot.
Flags: needinfo?(amarchesini)
No particular issue. BTW, are they needed for Test Pilot?
Flags: needinfo?(amarchesini) → needinfo?(jkt)
Assignee

Comment 4

2 years ago
No as we are in charge of all the creation points, so it's not needed. (if users have managed to add through the about:preferences panel then they have a bug as we disable it).
Flags: needinfo?(jkt)
Assignee

Comment 5

2 years ago
Assuming I add a test or 5 does the current attached patch look ok?
Flags: needinfo?(amarchesini)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8867511 [details]
Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved

https://reviewboard.mozilla.org/r/139050/#review142600

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:175
(Diff revision 1)
>    },
>  
>    create(name, icon, color) {
> +    let userContextId = ++this._lastUserContextId;
>      let identity = {
> -      userContextId: ++this._lastUserContextId,
> +      userContextId: userContextId,

just: userContextId,
Attachment #8867511 - Flags: review?(amarchesini)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8867511 [details]
Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved

https://reviewboard.mozilla.org/r/139050/#review142602
Attachment #8867511 - Flags: review?(amarchesini) → review+
Yes, it looks correct. A test is needed, and also a proper review from a webextension peer.
Flags: needinfo?(amarchesini)
Assignee

Comment 9

2 years ago
As we are likely to launch this on AMO this will cause issues with our addon not being the only crud end points for containers. We will need these events to ensure menus are updated correctly and others will need it to reduce calling the API repeatedly.
Assignee

Comment 10

2 years ago
So my current thinking is:
- Use onChanged like cookies or use onUpdated like tabs?
- Should we flatten the contextual identity to be the return value or should we nest it under a "contextualIdentity" key like cookies do
- We don't have the full object for removed, should I flatten this differently?

Curently:

browser.contextualIdentities.onCreated.addListener((e) => {console.log(e);});

// {contextualIdentity: {cookieStoreId: 12, icon: "fingerprint" color: "red", name: "bloop"}}


browser.contextualIdentities.onChanged.addListener((e) => {console.log(e);});

// {contextualIdentity: {cookieStoreId: 12, icon: "fingerprint" color: "red", name: "bloop"}}


browser.contextualIdentities.onRemoved.addListener((e) => {console.log(e);});

// {cookieStoreId: 12}


My rationale to nest in the "contextualIdentity" key is if we needed reason etc. However this feels a little excessive for remove. I could possibly make the event pass the deleted data for the container?
Flags: needinfo?(amckay)
Assignee

Comment 11

2 years ago
I'm going to make the following changes and hope they are ok:

- onUpdated as we have contextualIdentities.update rather than change.
- Return the full object to be consistent on onRemoved
- Serialise the JSON structure of the current state of the container in ContextualIdentitiesService and unserialize in the extension code rather than query again.

Should we care about the user manually flipping the container prefs outside adding our own containers changes? We could file a follow up bug to trigger onUpdated and onRemoved listeners on pref change to the current list of containers.
Flags: needinfo?(amckay)
Comment hidden (mozreview-request)
Assignee

Comment 13

2 years ago
I made a demo extension as part of testing this:
https://github.com/jonathanKingston/extension-debugging/tree/master/container-events

Feel free to use this code in the documentation process or update the one Andy had.

Comment 14

2 years ago
mozreview-review
Comment on attachment 8867511 [details]
Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved

https://reviewboard.mozilla.org/r/139050/#review164622

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:195
(Diff revision 2)
>        name
>      };
>  
>      this._identities.push(identity);
>      this.saveSoon();
> +    Services.obs.notifyObservers(null, "contextual-identity-created", this.getIdentityObserverOutput(identity));

indentation.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:213
(Diff revision 2)
>        delete identity.l10nID;
>        delete identity.accessKey;
>  
> -      Services.obs.notifyObservers(null, "contextual-identity-updated", userContextId);
>        this.saveSoon();
> +      Services.obs.notifyObservers(null, "contextual-identity-updated", this.getIdentityObserverOutput(identity));

indentation.

::: toolkit/components/extensions/ext-contextualIdentities.js:23
(Diff revision 2)
>  
>    return result;
>  };
>  
> +const convertIdentityFromObserver = identityJSON => {
> +  let identity = JSON.parse(identityJSON);

This seems fragile. Would be nice if we create the object passing 1 by 1 the parameter we want from the JSON object. Sometimes I wish JS had MOZ_ASSERT()
Attachment #8867511 - Flags: review+
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Assignee: nobody → jkt
Keywords: dev-doc-needed

Comment 16

2 years ago
mozreview-review
Comment on attachment 8867511 [details]
Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved

https://reviewboard.mozilla.org/r/139050/#review165860

a few little nits but overall looks great, thanks!

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:195
(Diff revision 3)
>        name
>      };
>  
>      this._identities.push(identity);
>      this.saveSoon();
> +    Services.obs.notifyObservers(null, "contextual-identity-created",

nit: can you pass all the details as the subject of the notification instead of as a json encoded string?

::: toolkit/components/extensions/ext-contextualIdentities.js:144
(Diff revision 3)
>            return Promise.resolve(convertedIdentity);
>          },
> +
> +        onCreated: new EventManager(context, "contextualIdentities.onCreated", fire => {
> +          let observer = (subject, topic, identityJSON) => {
> +            if (containersEnabled) {

I'm confused by this check.  If it is possible to create/change/remove containers even when using them is disabled by the pref, won't extensions want to be able to track those changes?

::: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js:116
(Diff revision 3)
> +
> +    browser.test.notifyPass("contextualIdentities_events");
> +  }
> +
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background: `(${backgroundScript})()`,

nit, this can just be `background: backgroundScript,` (or rename the function to be `background` and the entire line can be `background,`)
Attachment #8867511 - Flags: review?(aswan) → review+
Assignee

Comment 17

2 years ago
Hey thanks for the review!

I have some questions.


> I'm confused by this check.  If it is possible to create/change/remove containers even when using them is disabled by the pref, won't extensions want to be able to track those changes?

The check is there such that containers could be enabled by the pref or some other means and the containers addon could be notified of the additions?
Maybe this isn't important at all if my other patch to land that container addons can always enable containers?

> nit: can you pass all the details as the subject of the notification instead of as a json encoded string?

Do you mean userContextId or something? The issue here is the delete happens before the containers code gets called so I can't query for the container there. Or are you speaking of just passing a raw object as the subject, is that possible even?

> nit, this can just be `background: backgroundScript,` (or rename the function to be `background` and the entire line can be `background,`)

Ah yeah cool :) thanks.
Flags: needinfo?(aswan)
(In reply to Jonathan Kingston [:jkt] from comment #17)
> > I'm confused by this check.  If it is possible to create/change/remove containers even when using them is disabled by the pref, won't extensions want to be able to track those changes?
> 
> The check is there such that containers could be enabled by the pref or some
> other means and the containers addon could be notified of the additions?
> Maybe this isn't important at all if my other patch to land that container
> addons can always enable containers?

Sorry, I didn't follow the first sentence.  I agree that if we force containers to be on then this is irrelevant though (I've been waiting on that other patch for others to weigh in on the high level concept before going over the code in detail)

> > nit: can you pass all the details as the subject of the notification instead of as a json encoded string?
> 
> Do you mean userContextId or something? The issue here is the delete happens
> before the containers code gets called so I can't query for the container
> there. Or are you speaking of just passing a raw object as the subject, is
> that possible even?

Yeah, I meant passing the raw object as the subject.  Its possible, the observer service is all xpcom stuff but its doable with wrappedJSObject.  Here's an example:
http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/mozapps/extensions/AddonManager.jsm#2060-2061
http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/modules/ExtensionsUI.jsm#221
Flags: needinfo?(aswan)
Assignee

Comment 19

2 years ago
> Sorry, I didn't follow the first sentence.

My hunch was that we could have containers extensions listen for when containers get added rather than having a containersEnabled event they would listen for. I think we can safely assume that if containers get enabled with extensions that want to use them we can ignore this check.

> (I've been waiting on that other patch for others to weigh in on the high level concept before going over the code in detail)

No worries I have been put on something else this week, I'm currently thinking of making the ContainersService observe the pref and remove/enable containers there and clear all tabs. It's a refactor I haven't had time to do at the moment.


> Its possible, the observer service is all xpcom stuff but its doable with wrappedJSObject.

Ah ok, I think I have done this before in cpp methods actually, thanks.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 22

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/59765ae0aee3
Add web extension events for containers onUpdated, onCreated and onRemoved r=aswan,baku
Keywords: checkin-needed
Comment hidden (mozreview-request)
Assignee

Comment 27

2 years ago
mozreview-review
Comment on attachment 8867511 [details]
Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved

https://reviewboard.mozilla.org/r/139050/#review168740

::: toolkit/components/contextualidentity/tests/unit/test_basic.js:10
(Diff revision 5)
>  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
>  Cu.import("resource://gre/modules/ContextualIdentityService.jsm");
> +Cu.import("resource://gre/modules/osfile.jsm");
>  
> -const TEST_STORE_FILE_NAME = "test-containers.json";
> +const TEST_STORE_FILE_PATH = OS.Path.join(OS.Constants.Path.profileDir, "test-containers.json");

I'm not really sure why the following error wasn't an issue before:

https://treeherder.mozilla.org/logviewer.html#?job_id=119795653&repo=autoland&lineNumber=2692

Essentially this relates to using a relative path for the test file:

http://searchfox.org/mozilla-central/source/toolkit/components/contextualidentity/ContextualIdentityService.jsm#269

:KWierso has there been any changes in how xpcshell read files perhaps? Maybe the file isolation sandbox work. I'm also not really sure why this wouldn't fail the test.

The other error is somewhat understandable in that the lazy reading of the strings didn't initialise before and now we explicitly read these before sending the events.

Either way is this ready for merging?
Assignee

Comment 28

2 years ago
See: https://bugzilla.mozilla.org/show_bug.cgi?id=1344519#c27

Thanks
Flags: needinfo?(jkt) → needinfo?(wkocher)
It's possible the sandbox level 3 stuff broke this. I'd say it's worth trying again.
Flags: needinfo?(wkocher)
Assignee

Updated

2 years ago
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(jkt)
Keywords: checkin-needed
Assignee

Comment 31

2 years ago
Sorry again!
Flags: needinfo?(jkt)
Keywords: checkin-needed

Comment 32

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/31db4b302143
Add web extension events for containers onUpdated, onCreated and onRemoved r=aswan,baku
Keywords: checkin-needed
I had to back this out for android xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=120837712&repo=autoland
Flags: needinfo?(jkt)

Comment 34

2 years ago
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/18e395b49530
Backed out changeset 31db4b302143 for android xpcshell test_basic.js failures a=backout CLOSED TREE
Comment hidden (mozreview-request)
Assignee

Comment 41

2 years ago
TL;DR I think the risk is super low to check this in again.

I added the strings which made the tests fail :KWierso, am I in need of review again. Both of my reviewers are away it seems.

Essentially we should have more tests covering Android and we expanded the test such that it broke Android, but t would have already been broken somewhat without these strings.

Either way web extensions are unlikely to be running on Android at the moment as they don't have any UX, I also haven't had time to work on testing physically an extension on Android.
Flags: needinfo?(jkt) → needinfo?(wkocher)
Assignee

Comment 42

2 years ago
Requesting check-in here as no changes were made besides the modification of strings.
Flags: needinfo?(wkocher)
Keywords: checkin-needed

Comment 43

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3323bbf5cea8
Add web extension events for containers onUpdated, onCreated and onRemoved r=aswan,baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3323bbf5cea8
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee

Comment 46

2 years ago
This looks great, sorry for the delay in reviewing. Thanks!
Flags: needinfo?(jkt)

Comment 47

2 years ago
I tryed reproducing the issue using the example extension and the https://addons.mozilla.org/en-us/firefox/addon/multi-account-containers/ webextension from amo but was unable to reproduce it. What am I suppose to be looking out for?
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(amckay)
Reporter

Comment 48

2 years ago
I installed every event:

https://addons.mozilla.org/en-US/firefox/addon/every-event/

Clicked on the browser action button to get to configuration and turned logging off for all events EXCEPT contextualIdentity events, then enabled notifications. Then with a browser console open, I went to about:preferences and created a new container, updated it and then deleted it.

I got these messages:

Event fired: browser.contextualIdentities.onCreated
Arguments:  Object { contextualIdentity: Object }

Event fired: browser.contextualIdentities.onUpdated
Arguments:  Object { contextualIdentity: Object }

Event fired: browser.contextualIdentities.onRemoved
Arguments:  Object { contextualIdentity: Object }

I would call this verified fixed, but that flow might work for you.
Flags: needinfo?(amckay)

Comment 49

2 years ago
I can reproduce this issue on Firefox 57.0a1 (20170813183258) under Wind 10 64-bit. 

This issue is verified as fixed on Firefox 57.0b11 (20171023180840) under Wind 10 64-bit and Mac OS X 10.13.

Updated

2 years ago
Status: RESOLVED → VERIFIED

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.