864895 - PopupNotifications should catch exceptions from callbacks

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Matthew N. [:MattN]]

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.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

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?).

Comment 2

[Matthew N. [:MattN]]

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.

Comment 3

[Sankha Narayan Guria [:sankha]]

Attachment: WIP Patch v1

Matt, can you please go through this?

Comment 4

[Matthew N. [:MattN]]

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.

Comment 5

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

Comment 6

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

Comment 7

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 748674 [details] [diff] [review]
patch v2

>From: Sankha Narayan Guria <[e-mail]>
>
>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.

Comment 9

[Sankha Narayan Guria [:sankha]]

Attachment: WIP Patch

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?

Comment 10

[Matthew N. [:MattN]]

Attachment: WIP v.2 working test thrown together

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.

Comment 11

[Sankha Narayan Guria [:sankha]]

Attachment: patch with all test cases

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Note: this may need to be adjusted if bug 882858 lands first.

Comment 13

[Sankha Narayan Guria [:sankha]]

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.

Comment 14

[Sankha Narayan Guria [:sankha]]

Sorry, I mean bug 882858.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Sankha, are you still working on this? I'd like to get this landed as soon as possible.

Comment 16

[Sankha Narayan Guria [:sankha]]

Attachment: updated patch v3

Comment 17

[Matthew N. [:MattN]]

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.

Comment 18

[Matthew N. [:MattN]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=1ad5d95de297

Comment 19

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf9d97ed9cd

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cdf9d97ed9cd