582201 - exceptions show twice in WebConsole

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

Exceptions thrown by a web page are shown twice in the WebConsole. This happens because of the window.onerror event handler added by the HUDService.setOnErrorHandler() method, and because of the HUDService.logConsoleActivity() method.

To reproduce this issue, please see attachment 458613 [details].

I suggest we remove the window.onerror event handler, and that this bug fix becomes a blocker for Firefox 4.

Comment 1

[David Dahl :ddahl]

It's not quite as simple as that. The onerror handler will catch things that the logic behind logConsoleActivity will not catch, e.g: any error in an externally-referenced script.

Perhaps the onerror handler should check if the source uri is the same as the document.location.href and ignore the message?

Comment 2

[Mihai Sucan [:msucan]]

I will test with external scripts as well and I'll see what I can do. Will provide a patch soon. Thanks for your reply, David!

Comment 3

[Mihai Sucan [:msucan]]

Attachment: patch + test code

This patch fixes the issue and includes test code that makes sure error messages are not shown twice.

The test for bug 580030 needed a rewrite, given it started failing after the fix. That test and the new test use the same approach - a Service.console listener.

Looking forward to your feedback!

Comment 4

[David Dahl :ddahl]

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

looks good, I just don't like variable names like:

+        pos = text.indexOf("test-duplicate-error.js");
+        ok(pos > -1, "found test-duplicate-error.js");
+

use a more descriptive name instead of "pos". Not a big deal though.

Comment 5

[Mihai Sucan [:msucan]]

Attachment: updated patch + test code

Thank you David for the feedback+!

I have updated the patch as you suggested. Now asking for review from Gavin.

Comment 6

[Shawn Wilsher :sdwilsh]

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

For comments with more expandable code context, please see http://reviews.visophyte.org/r/461766/.

on file: toolkit/components/console/hudservice/HUDService.jsm line 199
>     var origOnerrorFunc = window.onerror;
>     window.onerror = function windowOnError(aErrorMsg, aURL, aLineNumber)

I talked to ddhal about this on irc, but making a comment to so it doesn't get
lost.  How this error handler swaping is done means your code will fail to
work properly in at least two cases:
1) The page replaces it's error handler some time later.
2) The pages error handler throws, causing your handler to not return false.

The way to fix this would be to use addEventListener.  You do not have to
worry about this for this bug, David said he'd file a new one on it.


on file: toolkit/components/console/hudservice/HUDService.jsm line 202
>       if (aURL && !(aURL in self.uriRegistry)) {

A comment explaining this would be greatly appreciated.  I don't know this
codebase well, and looking at it I have no idea why this would be needed.


on file: toolkit/components/console/hudservice/tests/browser/Makefile.in line 61
> 	test-duplicate-error.js \

Looks to me like you actually want this in _BROWSER_TEST_FILES even though
they go to the same place.


on file: toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js line 629
>     QueryInterface: XPCOMUtils.generateQI(
>       [Ci.nsIObserver]
>     ),

Your styling is not consistent here with code you add further down (I prefer
the latter style of the two you used as it matches the rest of the codebase
better).


on file: toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js line 643
>       var successMsg = "Found the error message after page reload";
>       var errMsg = "Could not get the error message after page reload";

this should probably be constants


on file: toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js line 659
>   browser.addEventListener("load", function() {
>     if (!pageReloaded) {
>       pageReloaded = true;
>       content.location.reload();
>       return;
>     }
> 
>     browser.removeEventListener("load", arguments.callee, true);

Can you explain (in the bug) why this change was necessary please.


on file: toolkit/components/console/hudservice/tests/browser/test-duplicate-error.html line 9
>     <script type="text/javascript"><!--
>       fooDuplicateError1.bar();
>     // --></script>
>     <script type="text/javascript" src="test-duplicate-error.js"></script>

Can you explain this test a bit better in the bug please?


on file: toolkit/components/console/hudservice/tests/browser/test-duplicate-error.js line 1
> 
> fooDuplicateError2.baz();
> 

This file, as well as the new test file you added need proper licensing.  You
can use the public domain license per
http://www.mozilla.org/MPL/license-policy.html


r- because I want to see the answers to my questions, and I'd like to take a look at the patch again.

Comment 7

[Mihai Sucan [:msucan]]

(In reply to comment #6)
> Comment on attachment 461766 [details] [diff] [review]
> updated patch + test code
> 
> For comments with more expandable code context, please see
> http://reviews.visophyte.org/r/461766/.
> 
> on file: toolkit/components/console/hudservice/HUDService.jsm line 199
> >     var origOnerrorFunc = window.onerror;
> >     window.onerror = function windowOnError(aErrorMsg, aURL, aLineNumber)
> 
> I talked to ddhal about this on irc, but making a comment to so it doesn't get
> lost.  How this error handler swaping is done means your code will fail to
> work properly in at least two cases:
> 1) The page replaces it's error handler some time later.
> 2) The pages error handler throws, causing your handler to not return false.
> 
> The way to fix this would be to use addEventListener.  You do not have to
> worry about this for this bug, David said he'd file a new one on it.

Thanks for mentioning. I already saw the report.


> on file: toolkit/components/console/hudservice/HUDService.jsm line 202
> >       if (aURL && !(aURL in self.uriRegistry)) {
> 
> A comment explaining this would be greatly appreciated.  I don't know this
> codebase well, and looking at it I have no idea why this would be needed.

Each web page for which there's a HUD registered (WebConsole), is put inside the HUDService.uriRegistry object. This allows one to keep track of which pages have a HUD open.

The bug at hand is about the situation when an error, an exception, is shown twice. That happens when the HUDConsoleObserver (registered with Services.console, to receive nsIConsoleMessages or nsIScriptErrors from the nsIConsoleService) receives an error, an exception, from a script inside the page (a local script, not an external script). This causes the message the to be displayed once by the window.onerror event handler, and by the HUDConsoleObserver. When errors are thrown from external scripts inside a web page, they cannot be associated to the web page / window they came from. Thus, they are skipped by the associated code from being displayed in the WebConsole - otherwise we'd end up with messages from different windows/tabs.

The fix is really simple: if the error comes from the web page (in the uriRegistry), we skip it in the window.onerror handler.

> on file: toolkit/components/console/hudservice/tests/browser/Makefile.in line
> 61
> > 	test-duplicate-error.js \
> 
> Looks to me like you actually want this in _BROWSER_TEST_FILES even though
> they go to the same place.

Ah, right. Will fix. I thought that if I put the script there, then it will execute automatically when people run the tests from that folder. This is, of course, not intended, because the JavaScript file is intended to be used only by the test-duplicate-error.html page.


> on file:
> toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
> line 629
> >     QueryInterface: XPCOMUtils.generateQI(
> >       [Ci.nsIObserver]
> >     ),
> 
> Your styling is not consistent here with code you add further down (I prefer
> the latter style of the two you used as it matches the rest of the codebase
> better).

Will fix.


> on file:
> toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
> line 643
> >       var successMsg = "Found the error message after page reload";
> >       var errMsg = "Could not get the error message after page reload";
> 
> this should probably be constants

Will fix.

> on file:
> toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
> line 659
> >   browser.addEventListener("load", function() {
> >     if (!pageReloaded) {
> >       pageReloaded = true;
> >       content.location.reload();
> >       return;
> >     }
> > 
> >     browser.removeEventListener("load", arguments.callee, true);
> 
> Can you explain (in the bug) why this change was necessary please.

Sure.
(this is "history", sorry!)

- There was bug 579975 which caused exceptions (and other messages) to never show up in the error console, even if they were coming from the HUDConsoleObserver and even if they could be associated to the current tab properly. The problem was due to a small error in filtering the messages.

It should be noted that at that point in time exceptions only showed up when they were caught by the window.onerror handler.

- In the same time we also worked on bug 580030 which consisted of exceptions failing to show up after page reload. The problem was trivial, as explained in

https://bugzilla.mozilla.org/show_bug.cgi?id=580030#c2

Basically, after the windowInitializer was invoked, on page reload, the HeadsUpDisplay.makeXULNode() method failed silently to display the errors coming from the window.onerror event handler. That was a special case with try {} failing during the event handler execution - otherwise it worked fine.

The test code for bug 580030 (see attachment 460263 [details] [diff] [review]) relied on the fact that the click event handler is fired after the click event handler of the button, which when was invoked, also triggered the window.onerror event handler, which in turn displayed the error message. (uh oh)

Now, you can see that after the two main fixes, we got to this bug, and to this change in the test code of bug 580030. Now it was necessary to asynchronously listen for the error to show up.


> on file:
> toolkit/components/console/hudservice/tests/browser/test-duplicate-error.html
> line 9
> >     <script type="text/javascript"><!--
> >       fooDuplicateError1.bar();
> >     // --></script>
> >     <script type="text/javascript" src="test-duplicate-error.js"></script>
> 
> Can you explain this test a bit better in the bug please?

Sure.

The test generates two errors, one from an internal script and one from an external script. The main test code checks to make sure each of the two errors show only once. As explained above, without the fix in the window.onerror event handler, then the fooDuplicateError1 coming from the internal script would display twice.

Now that I wrote so much on the history of the code, I noticed that I can remove the external script. Will fix this in the updated patch.


> on file:
> toolkit/components/console/hudservice/tests/browser/test-duplicate-error.js
> line 1
> > 
> > fooDuplicateError2.baz();
> > 
> 
> This file, as well as the new test file you added need proper licensing.  You
> can use the public domain license per
> http://www.mozilla.org/MPL/license-policy.html

Will fix and will post an updated patch.

Thank you for your review! Really appreciated. Sorry for writing too much in this comment. :)

Comment 8

[Mihai Sucan [:msucan]]

Attachment: updated patch + test code

This is the updated patch, with changes based on your review.

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Thank you for your review! Really appreciated. Sorry for writing too much in
> this comment. :)
I don't think you'll ever find a reviewer complaining about a detailed explanation of things. :)

I should be able to do the review tomorrow.

Comment 10

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh

Comment 11

[Shawn Wilsher :sdwilsh]

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

This is a blocker; it does not need approval.

Comment 12

[David Dahl :ddahl]

(In reply to comment #11)
> Comment on attachment 463261 [details] [diff] [review]
> updated patch + test code
> 
> This is a blocker; it does not need approval.

ah! yeah... So much for my workflow:)

Comment 13

[Mihai Sucan [:msucan]]

Attachment: [checked-in] rebased patch + test code

This is the rebased patch and test code for the default branch of mozilla-central.

Comment 14

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

Comment on attachment 464360 [details] [diff] [review]
[checked-in] rebased patch + test code

http://hg.mozilla.org/mozilla-central/rev/b7f787c93115