Closed Bug 864895 Opened 7 years ago Closed 6 years ago

PopupNotifications should catch exceptions from callbacks

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: MattN, Assigned: sankha)

References

()

Details

(Whiteboard: [doorhanger][mentor=MattN][lang=js][good first bug])

Attachments

(1 file, 7 obsolete files)

Spun-off from bug 841967 comment 9 and 10:

Exceptions thrown by PopupNotification consumers during the mainAction and secondary actions callbacks should be caught and reported to the error console.  Currently an exception during the callback on one notification can prevent callbacks on other notifications from being called for the same event.

It seems like a try/catch can happen in PopupNotifications_fireCallback but there are other callbacks which don't use this mechanism that should also be fixed.
I think this was probably the main cause of bug 841967, fwiw, so I think we should fix this in whatever version we're actually shipping the delay in (Aurora now?).
That's what I wanted your opinion on in bug 841967.  I would have fixed this there if I knew you also thought this was the main cause.
Attached patch WIP Patch v1 (obsolete) — Splinter Review
Matt, can you please go through this?
Assignee: nobody → sankha93
Attachment #745734 - Flags: feedback?(mnoorenberghe+bmo)
Comment on attachment 745734 [details] [diff] [review]
WIP Patch v1

Review of attachment 745734 [details] [diff] [review]:
-----------------------------------------------------------------

Good start.  This only covers the one set of callbacks though.  I also think Cu.reportError makes sense here.  Please add tests for each case to ensure execution continues despite an exception.
Attachment #745734 - Flags: feedback?(mnoorenberghe+bmo) → feedback+
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #745734 - Attachment is obsolete: true
Attachment #748671 - Flags: review?(mnoorenberghe+bmo)
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #748671 - Attachment is obsolete: true
Attachment #748671 - Flags: review?(mnoorenberghe+bmo)
Attachment #748673 - Flags: review?(mnoorenberghe+bmo)
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #748673 - Attachment is obsolete: true
Attachment #748673 - Flags: review?(mnoorenberghe+bmo)
Attachment #748674 - Flags: review?(mnoorenberghe+bmo)
Attachment #748674 - Attachment is patch: true
Attachment #748674 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 748674 [details] [diff] [review]
patch v2

>From: Sankha Narayan Guria <sankha93@gmail.com>
>
>Bug 864895 - PopupNotifications catches exceptions from callbacks; r=MattN
>
>diff --git a/browser/base/content/test/browser_popupNotification.js b/browser/base/content/test/browser_popupNotification.js

>+    run: function () {
>+      loadURI("http://example.com/", function() {
>+        let notifyObj = new basicNotification();
>+        notifyObj.options.eventCallback = function (eventName) {
>+          if (eventName == "removed") {
>+            throw new Error("Oops!");
>+            ok(true, "Test continues after error is raised.");

This is a non-sequitur - this code will never be executed, because of the exception. What you want to test is that that exception doesn't negatively effect the PopupNotifications code itself (i.e. that an exception that is thrown from _fireCallback doesn't cause trouble).

One way to test this would be to have two shown notifications, and have the first notifications "shown" event callback throw an exception. Then verify that the second notification's "shown" handler is still called. Before your patch this should fail, after your patch it should succeed. Another way to add that test coverage would be to show a notification whose "dismissed" event callback throws an exception, then dismiss the notification. Before your change, the notification panel won't have been cleared out correctly (because the code after http://hg.mozilla.org/mozilla-central/annotate/b30552dbb013/toolkit/content/PopupNotifications.jsm#l704 won't run), and after your change it should be properly cleared.

Both of those tests provide test coverage of the _fireCallback change. To cover the other changes, you should test adding notifications whose mainActions and secondaryActions throw, and then test that the panel is still properly dismissed after those actions are triggered.

Of course, for every test that you add you should ensure that it fails when your fix isn't applied.

The code changes look good otherwise, but the test needs some work.
Attachment #748674 - Flags: review?(mnoorenberghe+bmo) → feedback-
Attached patch WIP Patch (obsolete) — Splinter Review
I have written the test according to the comments above. But when I run it I am getting TEST-UNEXPECTED-FAIL| chrome://mochitests/content/browser/browser/base/content/test/browser_popupNotification.js | Test timed out

On the screen I see tests completing successfully till test #29. So I guess there is some problem in my test, that prevents it from going to test #30. What am I doing wrong?
Attachment #748674 - Attachment is obsolete: true
Attachment #756908 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #756908 - Flags: feedback?(gavin.sharp)
I made a test based on #7 that "works" in that it fails without the jsm change and passes after. It could still be improved and doesn't cover the action callbacks. Hope this helps.
Attachment #756908 - Attachment is obsolete: true
Attachment #756908 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #756908 - Flags: feedback?(gavin.sharp)
Attached patch patch with all test cases (obsolete) — Splinter Review
Attachment #756927 - Attachment is obsolete: true
Attachment #758493 - Flags: review?(mnoorenberghe+bmo)
Note: this may need to be adjusted if bug 882858 lands first.
Comment on attachment 758493 [details] [diff] [review]
patch with all test cases

Review of attachment 758493 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling the review request, as bug 864895 has landed. Will attach a new patch.
Attachment #758493 - Flags: review?(mnoorenberghe+bmo)
Sorry, I mean bug 882858.
Sankha, are you still working on this? I'd like to get this landed as soon as possible.
Attached patch updated patch v3Splinter Review
Attachment #758493 - Attachment is obsolete: true
Attachment #817656 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 817656 [details] [diff] [review]
updated patch v3

Review of attachment 817656 [details] [diff] [review]:
-----------------------------------------------------------------

I had mentioned to Sankha over IRC some months ago that I wanted errorNotification to inherit from basicNotification but we can handle that in a follow-up so the fix can land.
Attachment #817656 - Flags: review?(mnoorenberghe+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/cdf9d97ed9cd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.