Closed
Bug 1265797
Opened 8 years ago
Closed 8 years ago
WebExtensions Notification observer should not remove the notificationId on alertshow
Categories
(WebExtensions :: Untriaged, defect, P3)
WebExtensions
Untriaged
Tracking
(firefox47 wontfix, firefox48 verified, firefox49 verified)
People
(Reporter: rpl, Assigned: bsilverberg)
References
Details
(Whiteboard: [notifications] triaged)
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
aswan
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details |
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.
Updated•8 years ago
|
Assignee: nobody → bob.silverberg
Priority: -- → P3
Whiteboard: [notifications] triaged
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Iteration: --- → 49.2 - May 23
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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•8 years ago
|
||
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+
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a9850205f60
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5bdcb5693f01
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 10•8 years ago
|
||
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?
Assignee | ||
Updated•8 years ago
|
status-firefox47:
--- → wontfix
status-firefox48:
--- → affected
Updated•8 years ago
|
Flags: qe-verify+
QA Contact: vasilica.mihasca
Assignee | ||
Comment 11•8 years ago
|
||
Note: try run against aurora with all three patches for uplift: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b92fec992103
Comment 12•8 years ago
|
||
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+
Comment 13•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/8e93a77ffdd4
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 15•8 years ago
|
||
Thanks Vasilica. Those errors are fixed by a subsequent bug (bug 1275363).
Flags: needinfo?(bob.silverberg)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•