1265797 - WebExtensions Notification observer should not remove the notificationId on alertshow

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

the observer at line: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-notifications.js#54
can receive an "alertshow" topic (e.g. https://dxr.mozilla.org/mozilla-central/search?q=alertshow&redirect=false&case=true) 
and it wrongly assumes that it needs to remove the notification, and after that, if we try to close the alert using notifications.clear, it will fail (and return false) because the notificationId cannot be found anymore in the notification map.

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1265797 - WebExtensions Notification observer should not remove the notificationId on alertshow, r?aswan

Note that I looked at trying to create a test to test this bug and which would prove it was fixed by the patch,
but there is already a test in place [1] that would catch this if the timing was right.

It looks like most of the time the call to clear() happens before the observer reacts to the `alertshow` topic
and only if the opposite is true would the bug manifest itself.

All of this is to say that I believe that the fix is a good one, but not one for which we can accurately test.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_ext_notifications.html#91

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

Comment 2

[Andrew Swan [:aswan]]

Comment on attachment 8750850 [details]
MozReview Request: Bug 1265797 - WebExtensions Notification observer should not remove the notificationId on alertshow, r?aswan

https://reviewboard.mozilla.org/r/51675/#review48539

::: toolkit/components/extensions/ext-notifications.js:71
(Diff revision 1)
> -    }
> +    } else if (topic === "alertfinished") {
> -    if (topic === "alertfinished") {
>        notifications.emit("closed", data);
>      }
>  
>      notifications.delete(this.id);

I think the safe thing here would be to only delete the notification if you know you're getting an event that means the alert has been closed.  OSX for instance seems to generate a events that don't look like they correspond to dismissing the alert:
https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/OSXNotificationCenter.mm#499

Comment 3

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8750850 [details]
MozReview Request: Bug 1265797 - WebExtensions Notification observer should not remove the notificationId on alertshow, r?aswan

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

Comment 4

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/51675/#review48539

> I think the safe thing here would be to only delete the notification if you know you're getting an event that means the alert has been closed.  OSX for instance seems to generate a events that don't look like they correspond to dismissing the alert:
> https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/OSXNotificationCenter.mm#499

Good point. Fixed!

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8750850 [details]
MozReview Request: Bug 1265797 - WebExtensions Notification observer should not remove the notificationId on alertshow, r?aswan

https://reviewboard.mozilla.org/r/51675/#review48947

Comment 6

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a9850205f60

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5bdcb5693f01

Comment 8

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

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

Comment 10

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8750850 [details]
MozReview Request: Bug 1265797 - WebExtensions Notification observer should not remove the notificationId on alertshow, r?aswan

Approval Request Comment
[Feature/regressing bug #]: Bug 1276706 - notifications.clear not working
[User impact if declined]: This is a bug in the WebExtensions notifications API which is meant to be feature-complete in 48. Users of the API could easily hit this bug.
[Describe test coverage new/current, TreeHerder]: Unfortunately it was not possible to write an automated test for this change, but it was tested manually. The fact that this fixes bug 1276706 was verified by QA.
[Risks and why]: This is a very simple change, isolated to toolkit/components/extensions/ext-notifications.js so can be considered to be low risk. 
[String/UUID change made/needed]:none

Note also that there is a follow-up bug to this one (bug 1275363) that addresses a problem introduced by this bug, so that will need to be uplifted as well.

Comment 11

[Bob Silverberg [:bsilverberg]]

Note: try run against aurora with all three patches for uplift: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b92fec992103

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8750850 [details]
MozReview Request: Bug 1265797 - WebExtensions Notification observer should not remove the notificationId on alertshow, r?aswan

Improve WebExtension, taking it

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8e93a77ffdd4

Comment 14

[Vasilica Tamas, QA [:vtamas]]

Reproduced the initial issue on Firefox 48.0a2 (2016-05-31) under Windows 10 64-bit.

Verified fixed using the attached xpi from Bug 1276706 on Firefox 49.0a2 (2016-06-07) and Firefox 48 beta 1 build 2 (20160606200529) under Windows 10 64-bit.

But, I noticed that 2 errors are thrown in Browser Console every time the notification closes:
TypeError: this is undefined               ext-notifications.js:59:7
NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "this is undefined" {file: "chrome://extensions/content/ext-notifications.js" line: 59}]'[JavaScript Error: "this is undefined" {file: "chrome://extensions/content/ext-notifications.js" line: 59}]' when calling method: [nsIObserver::observe]                       alert.js:269
 
Should we be concerned about these errors?

Comment 15

[Bob Silverberg [:bsilverberg]]

Thanks Vasilica. Those errors are fixed by a subsequent bug (bug 1275363).