1389265 - contextualIdentities API resolves the promise when an error occurs

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[Rob Wu [:robwu]]

When container tabs are disabled, the contextualIdentities API methods resolve values with "false" [1], while I expected it to be rejecting a promise.

Similarly, the .get and .update methods resolve the promise with `null` instead of rejecting a promise when an error occurs (i.e. when the passed argument is invalid).

The documentation does not mention this. E.g. the documentation of browser.contextualIdentities.query [2] is written as if the only valid return value is an array:

A Promise that will be fulfilled with an array of ContextualIdentity objects, each describing a single identity.

A way to solve this bug is to update the documentation for all contextualIdentity methods and state that the return value can be different (null/false).
But I think that the API looks better if error conditions are propagated through a rejected promise instead of a null/false value.


[1] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/ext-contextualIdentities.js#27-29
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextualIdentities/query

Comment 1

[Jonathan Kingston [:jkt] he/him]

I think if we are going to solve this we should do this before 57.

We are actually removing the resolve false's in: https://bugzilla.mozilla.org/show_bug.cgi?id=1354602

Maybe I should add the rejects there to prevent so much diff noise.

The original idea of null, false was that you could tell the difference between the two responses. Perhaps we don't need to return anything for those null responses.

Comment 2

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1389265 - Change contextual identity web extension APIs to reject instead of returning null.

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

Comment 3

[Rob Wu [:robwu]]

Comment on attachment 8896766 [details]
Bug 1389265 - Change contextual identity web extension APIs to reject instead of returning null.

https://reviewboard.mozilla.org/r/168056/#review173156

::: toolkit/components/extensions/ext-contextualIdentities.js:62
(Diff revision 1)
>      let self = {
>        contextualIdentities: {
>          get(cookieStoreId) {
>            let containerId = getContainerForCookieStoreId(cookieStoreId);
>            if (!containerId) {
> -            return Promise.resolve(null);
> +            return Promise.reject();

Please add a useful error message, e.g.

```
throw new ExtensionError("description here");
```

(import ExtensionError from ExtensionUtils at the top of the file).

Comment 4

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8896766 [details]
Bug 1389265 - Change contextual identity web extension APIs to reject instead of returning null.

https://reviewboard.mozilla.org/r/168056/#review173156

> Please add a useful error message, e.g.
> 
> ```
> throw new ExtensionError("description here");
> ```
> 
> (import ExtensionError from ExtensionUtils at the top of the file).

I am just going to reject with an error message instead of throwing as that seems way more common in extension code.

Comment 5

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8896766 [details]
Bug 1389265 - Change contextual identity web extension APIs to reject instead of returning null.

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

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8896766 [details]
Bug 1389265 - Change contextual identity web extension APIs to reject instead of returning null.

https://reviewboard.mozilla.org/r/168056/#review173320

Comment 7

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8896766 [details]
Bug 1389265 - Change contextual identity web extension APIs to reject instead of returning null.

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

Comment 8

[Jonathan Kingston [:jkt] he/him]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4e43c6fdf21
Change contextual identity web extension APIs to reject instead of returning null. r=aswan

Comment 10

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

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

Comment 11

[Will Bamberg [:wbamberg]]

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

Comment 12

[Victor Carciu]

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.

Comment 13

[Jonathan Kingston [:jkt] he/him]

QE not required here either. Thanks.