Closed Bug 1418311 Opened 2 years ago Closed 2 years ago

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

Categories

(WebExtensions :: General, defect, P3)

56 Branch
defect

Tracking

(firefox59 verified)

VERIFIED FIXED
mozilla59
Tracking Status
firefox59 --- verified

People

(Reporter: robwu, Assigned: bsilverberg)

Details

(Whiteboard: [notifications])

Attachments

(2 files)

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
I'll take a look.
Assignee: nobody → bob.silverberg
Priority: -- → P3
Whiteboard: [notifications]
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 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.
Attachment #8929530 - Flags: review?(mixedpuppy) → review+
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5da988653a15
Avoid overwriting the notifications map in different contexts, r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/5da988653a15
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.