Closed Bug 580030 Opened 12 years ago Closed 12 years ago

the error handler fails silently after page reload

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(blocking2.0 betaN+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: msucan, Assigned: msucan)

References

Details

(Whiteboard: [kd4b6])

Attachments

(3 files, 2 obsolete files)

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.
Attached file 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.
Attached patch patch + test code (obsolete) — Splinter Review
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.
Attachment #458676 - Flags: review?(dtownsend)
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.
Attachment #458676 - Flags: review?(dtownsend) → review-
Attached patch updated patch + test code (obsolete) — Splinter Review
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.
Attachment #458676 - Attachment is obsolete: true
Attachment #458978 - Flags: feedback?(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.
Attachment #458978 - Flags: feedback?(ddahl) → feedback+
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.
Attachment #458978 - Flags: review?(dtownsend)
Attachment #458978 - Flags: approval2.0?
Attachment #458978 - Flags: approval2.0?
blocking2.0: --- → beta3+
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.
Attachment #458978 - Flags: review?(dtownsend) → review+
Assignee: nobody → mihai.sucan
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.
Attachment #458978 - Attachment is obsolete: true
Comment on attachment 460263 [details] [diff] [review]
[checked-in] update 2 for patch + test code

http://hg.mozilla.org/mozilla-central/rev/37f3f119d3a8
Attachment #460263 - Attachment description: update 2 for patch + test code → [checked-in] update 2 for patch + test code
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Whiteboard: [kd4b6]
Severity: normal → blocker
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
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!
Attachment #472397 - Flags: feedback?(ddahl)
Blocks: devtools4b7
this is test only at this point, moving to b7 so as to avoid confusion.
Blocks: devtools4b8
No longer blocks: devtools4b7
Attachment #472397 - Flags: feedback?(ddahl) → feedback+
Moving to betaN+
blocking2.0: beta3+ → betaN+
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.
Attachment #472397 - Flags: review?(gavin.sharp)
Attachment #472397 - Flags: review?(gavin.sharp) → review+
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment on attachment 472397 [details] [diff] [review]
[checked-in] test fix

http://hg.mozilla.org/mozilla-central/rev/9c2bdcc68406
Attachment #472397 - Attachment description: test fix → [checked-in] test fix
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.