Closed Bug 1265797 Opened 4 years ago Closed 4 years ago

WebExtensions Notification observer should not remove the notificationId on alertshow

Categories

(WebExtensions :: Untriaged, defect, P3)

defect

Tracking

(firefox47 wontfix, firefox48 verified, firefox49 verified)

VERIFIED FIXED
mozilla49
Iteration:
49.2 - May 23
Tracking Status
firefox47 --- wontfix
firefox48 --- verified
firefox49 --- verified

People

(Reporter: rpl, Assigned: bsilverberg)

References

Details

(Whiteboard: [notifications] triaged)

Attachments

(1 file)

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.
Assignee: nobody → bob.silverberg
Priority: -- → P3
Whiteboard: [notifications] triaged
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/
Attachment #8750850 - Flags: review?(aswan)
Iteration: --- → 49.2 - May 23
Status: NEW → ASSIGNED
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
Attachment #8750850 - Flags: review?(aswan)
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/
Attachment #8750850 - Flags: review?(aswan)
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 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
Attachment #8750850 - Flags: review?(aswan) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5bdcb5693f01
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Duplicate of this bug: 1276706
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.
Attachment #8750850 - Flags: approval-mozilla-aurora?
Flags: qe-verify+
QA Contact: vasilica.mihasca
Note: try run against aurora with all three patches for uplift: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b92fec992103
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
Attachment #8750850 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Status: RESOLVED → VERIFIED
Flags: qe-verify+ → needinfo?(bob.silverberg)
Thanks Vasilica. Those errors are fixed by a subsequent bug (bug 1275363).
Flags: needinfo?(bob.silverberg)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.