580030 - the error handler fails silently after page reload

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

The error handler (HUDService.setOnErrorHandler()) fails silently after page reload.

How to repro:

1. make a web page that allows you to trigger a JavaScript exception.
2. load the page.
3. open the HUD console.
4. trigger the JS exception your web page. see that it works - it shows in the HUD console.
5. refresh the page, and trigger the JS exception again. now the error does not show in the HUD console.

Based on debugging I determined that the window.onerror event handler seems to work fine. It is the console.error() call that fails silently - which is used by the event handler.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: test case

This is a test case. How to repro:

1. load this test case.
2. open the HUD console.
3. click the "generate error" button. it will show up in the HUD console.
4. refresh page.
5. generate an error. the error is not displayed in the HUD console.

Comment 2

[Mihai Sucan [:msucan]]

Attachment: patch + test code

This is the patch for the code.

The window.onerror event handler worked fine (HUDService.setOnErrorHandler), and the HUDService.windowInitializer() also properly updated the window.onerror event handler.

The console.error() method invoked by the window.onerror event handler, also worked fine.

The problem goes deep into the HeadsUpDisplay.makeHTMLNode() invoked by console.error(). The the return this.HTMLFactory() fails because the HTMLFactory() method does not exist ... because when the page is reloaded, the HeadsUpDisplay object is constructed with config.consoleOnly = true, which only re-attaches the existing HUD, but it does not create the HTMLFactory() method.

The try {} fails to catch the error, and it also hides the error - so no error shows in the Error console, nor anywhere else. Thus, the window.onerror event handler fails to display errors ... silently.

It should also be noted that the failure to catch the error applies *only* when the error comes from an event handler. (freaky) Hence the test code I provide uses a DOM event handler to generate an error.

The fix is trivial - I just don't use the try statement. The test code required quite some work, but I got it working fine.

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 458676 [details] [diff] [review]
patch + test code

timeouts in test code cause intermittent failures, switch to something more deterministic.

I'd also prefer if you synthesized the click from the chrome side rather than in content.

Comment 4

[Mihai Sucan [:msucan]]

Attachment: updated patch + test code

Dave: thanks for your review!

I have now updated the test code. It no longer uses timeouts, and the click event is dispatched from the main test code, not from the test page.

The updated patch applies cleanly on mozilla-central.

Comment 5

[David Dahl :ddahl]

Comment on attachment 458978 [details] [diff] [review]
updated patch + test code

Looking good. a simple error on my part, hard to figure out. good catch.

Comment 6

[David Dahl :ddahl]

Comment on attachment 458978 [details] [diff] [review]
updated patch + test code

This still needs official toolkit review, but is pretty important to get landed as we are masking CSS and script errors that should be in the console.

Comment 7

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

Comment on attachment 458978 [details] [diff] [review]
updated patch + test code

>diff -r 1ac07fe5f6c9 toolkit/components/console/hudservice/HUDService.jsm

>   makeHTMLNode:

>+    if (this.HTMLFactory) {
>       return this.HTMLFactory(aTag);
>     }
>+    else {

Please avoid the else-after-return.

>diff -r 1ac07fe5f6c9 toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js

>+function testErrorOnPageReload() {

>+    // dispatch a click event to the button in the test page.
>+    var contentDocument = browser.contentDocument.wrappedJSObject;
>+    var button = contentDocument.getElementsByTagName("button")[0];
>+    var clickEvent = contentDocument.createEvent("MouseEvents");
>+    clickEvent.initMouseEvent("click", true, true, null, 0, 0, 0, 0, 0,
>+      false, false, false, false, 0, null);

This can be done more concisely using EventUtils.synthesizeMouse.

Comment 8

[Mihai Sucan [:msucan]]

Attachment: [checked-in] update 2 for patch + test code

Thanks Gavin for your review!

This is the updated patch. Now the code no longer has the else-after-return problem.

As discussed, I cannot switch to EventUtils.synthesizeMouse() - it fails for me.

Comment 9

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 460263 [details] [diff] [review]
[checked-in] update 2 for patch + test code

http://hg.mozilla.org/mozilla-central/rev/37f3f119d3a8

Comment 10

[David Dahl :ddahl]

This appears to still fail, but "silently" as we just throw a new error instead of calling ok(false, errorOutput);


function testLogEntry(aOutputNode, aMatchString, aSuccessErrObj)
{
  var msgs = aOutputNode.querySelector(".hud-group").childNodes;
  for (var i = 1; i < msgs.length; i++) {
    var message = msgs[i].textContent.indexOf(aMatchString);
    if (message > -1) {
      ok(true, aSuccessErrObj.success);
      return;
    }
  }
  throw new Error(aSuccessErrObj.err); <-- this should be ok(false, aSuccessErrObj.err)
}

and of course the errors should be fixed, which are probably more like timing issues

Comment 11

[Mihai Sucan [:msucan]]

I haven't seen this fail yet.

I think we need to first split the HUDService test into multiple files before trying to fix existing tests. It's too much trouble.

With regards to throw new Error() at the end in testLogEntry(): sure, we can use ok(false, aSuccessErrObj.err). However, this was the initial intent of the testLogEntry() function. I did not change it. I agree it would be better to do ok(false, ...) instead of throwing.

Comment 12

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 13

[Mihai Sucan [:msucan]]

Attachment: [checked-in] test fix

Test code fix.

This patch changes the testLogEntry() function to not throw. It now uses ok(false, errorMessage).

The issue reported in this bug is still fixed - I don't think it regressed. All tests pass on my system, and using the test case (attachment 458613 [details]) I cannot reproduce the error. If you can reproduce the error, please paste the output - I'd like to see how it fails. The testErrorOnPageReload() function uses precise events that are not, hopefully, tied to specific timers - they should execute fine irrespective of resource constrains.

Please let me know if other changes are needed. Thanks David!

Comment 14

[Kevin Dangoor]

this is test only at this point, moving to b7 so as to avoid confusion.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Moving to betaN+

Comment 16

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 472397 [details] [diff] [review]
[checked-in] test fix

can we get a quick review on this? It's a really simple test change for better error reporting and I'd like to close this bug.

Comment 17

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 472397 [details] [diff] [review]
[checked-in] test fix

http://hg.mozilla.org/mozilla-central/rev/9c2bdcc68406