1418311 - Using browser.notifications API invalidates the map of notifications in another context

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[Rob Wu [:robwu]]

Attachment: notifications-forgetful.xpi

Upon calling getAPI in ext-notifications.js, the global map of notification IDs is overwritten. Consequently, an extension cannot update or dismiss previously created notifications any more.

The map should not be replaced after initialization, maybe by using a DefaultWeakMap.


STR:

1. Load attached add-on at about:debugging.
2. Look at the console of the background page (or the global JS console).

Expected (this happens in Chrome):
Closed notification test_notif_id   background.js:2
Removed notification as expected.   background.js:22

Actual:
notifications.getAll should have returned some notification, got {}.  touch-notifications-api.js:4:9
notifications.clear did not remove a notification!  background.js:24

The extension first creates a notification from the background page,
then opens a new tab to call a browser.notifications API and closes the tab,
then calls notifications.clear from the background page.
The failing test shows that the WebExtensions API has forgotten about the previously created notification.


[1] https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/toolkit/components/extensions/ext-notifications.js#99-104

Comment 1

[Bob Silverberg [:bsilverberg]]

I'll take a look.

Comment 2

[Andrew Swan [:aswan]]

There's no need to use notificationsMap any more, the notifications for a particular extension can just be stored in a member field on the ExtensionAPI class.  The same is true for several other APIs (eg alarms)

Comment 3

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1418311 - Avoid overwriting the notifications map in different contexts,

Prior to this patch, the map that holds the list of notifications for an extension was recreated
for each context. This fixes that issue and maintains a list for the extension across
contexts.

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

Comment 4

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8929530 [details]
Bug 1418311 - Avoid overwriting the notifications map in different contexts,

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

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8929530 [details]
Bug 1418311 - Avoid overwriting the notifications map in different contexts,

https://reviewboard.mozilla.org/r/200808/#review206090

::: toolkit/components/extensions/ext-notifications.js:17
(Diff revision 2)
> -// WeakMap[Extension -> Map[id -> Notification]]
> -let notificationsMap = new WeakMap();
> -
>  // Manages a notification popup (notifications API) created by the extension.
> -function Notification(extension, id, options) {
> -  this.extension = extension;
> +function Notification(extension, container, id, options) {
> +  this.container = container;

Can this be called notificationsMap?  I wasn't sure what container was until later.

::: toolkit/components/extensions/ext-notifications.js:101
(Diff revision 2)
> -          let notifications = notificationsMap.get(extension);
> -          if (notifications.has(notificationId)) {
> +          if (notificationsMap.has(notificationId)) {
> +            notificationsMap.get(notificationId).clear();
> -            notifications.get(notificationId).clear();
>            }
>  
>            // FIXME: Lots of options still aren't supported, especially

Lets just remove this comment, it's kind of pointless.  The two options we have bugs for are listed in later comments.

::: toolkit/components/extensions/ext-notifications.js:127
(Diff revision 2)
>            return Promise.resolve(result);
>          },
>  
>          onClosed: new EventManager(context, "notifications.onClosed", fire => {
>            let listener = (event, notificationId) => {
>              // FIXME: Support the byUser argument (bug 1413188).

reformat to TODO Bug 1413188, support byUser argument

::: toolkit/components/extensions/ext-notifications.js:159
(Diff revision 2)
>            return () => {
> -            notificationsMap.get(extension).off("shown", listener);
> +            notificationsMap.off("shown", listener);
>            };
>          }).api(),
>  
>          // Intend to implement this later: https://bugzilla.mozilla.org/show_bug.cgi?id=1190681

reformat to "TODO Bug 1190681, implement button support.

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8929530 [details]
Bug 1418311 - Avoid overwriting the notifications map in different contexts,

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5da988653a15
Avoid overwriting the notifications map in different contexts, r=mixedpuppy

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5da988653a15

Comment 9

[Madalin Cotetiu]

Verified as fixed in FF59 Win7(64bit), Ubuntu 16.24

The following is displayed in browser console when loading the extension:
notifications.getAll returned {
    "test_notif_id": {
        "appIconMaskUrl": null,
        "contextMessage": null,
        "eventTime": null,
        "iconUrl": "data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7",
        "imageUrl": null,
        "isClickable": null,
        "items": null,
        "message": "Look at the console of the background page",
        "priority": null,
        "progress": null,
        "title": "test closing via notifications.close",
        "type": "basic"
    }
}  touch-notifications-api.js:6
Closed notification test_notif_id  background.js:2
Removed notification as expected.

Marking bug as verified fixed.