597136 - Remove the HUDService onerror event handler and use the window ID from nsIScriptError2 messages

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

The window.onerror event handler is "leaked", in the sense that it's never cleared when the Web Console is closed, and it throws whenever an exception in the web page occurs, because the handler tries to send the message to the inexistent Web Console. We need to clear the event handler.

(see HUDService.setOnErrorHandler() and HUDService.windowInitializer())

The fix should be simple/trivial.

Comment 1

[Patrick Walton (:pcwalton)]

Nominating this for blocking status for Firefox 4, because this creates a very poor developer experience.

Comment 2

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Proposed patch attached.

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 476470 [details] [diff] [review]
Proposed patch.

I'm really glad you've taken to work on this bug. While this patch solves the issue, I really do not like the approach. For one, it adds the public window.onerror.orgOnErrorFunc method - this is an implementation detail that we should not expose to the public.

From the patch:

+  contentWindow.wrappedJSObject.onerror = function() {};
+  let origOnError = contentWindow.wrappedJSObject.onerror;

I'd prefer:

  let origOnError = function() {};
  contentWindow.wrappedJSObject.onerror = origOnError;

Also:

+  HUDService.activateHUDForContext(gBrowser.selectedTab);
+  HUDService.deactivateHUDForContext(gBrowser.selectedTab);

While this works fine, I kind of doubt it always works. The initialization includes some async aspects - like adding DOM nodes and the likes. I would suggest waiting for DOMNodeInserted - and then disable the console. See an example in attachment 476339 [details] [diff] [review] from bug 597103. May not be the best way - if you have better ideas, please let me know. ;)

Other than that, here's how I'd suggest the fix should be done:

1. there's bug 584775 which is trivial to fix in this bug. fixing that would also make our code more resistant to overwrites from the public web. and we no longer need to store the original window.onerror, since we won't need to overwrite it.

2. currently we use window.wrappedJSObject.console in the window.onerror event handler, which is bogus, because when the author overwrites window.console, we call that instead of our own window.console object. Not nice!

3. I'd move the window.addEventListener("error", ..., false) to the HUDConsole object (or in the HeadsUpDisplay object - doesn't really matter which), where we can reliably always output messages to our HUDConsole object.

When it's time to remove the WebConsole from the tab, we can do something like HUD.removeOnErrorHandler()  ... where HUD is the HUD object from the weakref.

This approach would give us a more reliable implementation of the onerror event handler.

Comment 4

[Patrick Walton (:pcwalton)]

(In reply to comment #3)
> While this works fine, I kind of doubt it always works. The initialization
> includes some async aspects - like adding DOM nodes and the likes.

Adding DOM nodes isn't asynchronous.

Comment 5

[Mihai Sucan [:msucan]]

(In reply to comment #4)
> (In reply to comment #3)
> > While this works fine, I kind of doubt it always works. The initialization
> > includes some async aspects - like adding DOM nodes and the likes.
> 
> Adding DOM nodes isn't asynchronous.

Uh, oh, my bad on this. Still, I had problems when I tried activate/deactivate very quickly one after the other. Given it works in the test code you provided,  you can leave it like that.

Comment 6

[Patrick Walton (:pcwalton)]

(In reply to comment #3)
> Comment on attachment 476470 [details] [diff] [review]
> Proposed patch.
> 
> I'm really glad you've taken to work on this bug. While this patch solves the
> issue, I really do not like the approach. For one, it adds the public
> window.onerror.orgOnErrorFunc method - this is an implementation detail that we
> should not expose to the public.

Totally agreed. I had considered trying to hide it better, but all the solutions I could come up with added a lot of complexity. Looks like I completely forgot about the obvious addEventListener() solution - thanks a lot for the idea :)

Comment 7

[Kevin Dangoor]

ping. This bug is a beta8 blocker. Let's get it into review and off our plate.

Comment 8

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.

New patch rebases to trunk, uses addEventListener(), and provides a new test.

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 484541 [details] [diff] [review]
Proposed patch, version 2.

I have tested the patch and there are some problems.

+  onErrorHandler: function HS_onErrorHandler(aErrorMsg, aURL, aLineNumber)
+  {

Based on a bit of testing, (Cu.reportError(aErrorMsg + " " + aURL + " " + aLineNumber)) it looks like the new event handler only receives the first argument, not three as expected. Additionally, aErrorMsg is not an error message, it's an Event object. A quick for..in loop reveals no relevant properties you can use. Weird.

Given this problem, it's obvious that any exceptions thrown by external scripts are now lost. Shawn in bug 584775 correctly suggested we shall switch to addEventListener, but does it work as we need it to? We should ask someone about this issue. A quick MDN search doesn't give more info.

+    let window = XPCNativeWrapper.unwrap(this);
+    window.console.error(aErrorMsg + " @ " + aURL + " " + lineNum);

This is really dangerous. I tested with overwriting the window.console.error() function. The script in the page was executed by the HUDService.

(In order to get these two lines of code to run, I obviously first deleted the other lines that fail to execute due to aforementioned issue.)


+  let error = null;
+  try {
+    EventUtils.sendMouseEvent({ type: "click" }, button);
+  } catch (e) {
+    error = e;
+  }

This test doesn't look right. Will sendMouseEvent() ever throw? The button click event handler always throws. If sendMouseEvent() executes the event handlers synchronously, then this call should always throw, irrespective of how we attach/detach our window.onerror event handler. I think this test needs further testing itself.

Additionally, there's bug 574941 to take into consideration as well. The event handler exception is lost when you use sendMouseEvent().

Also, I find it weird there is no specific test on how window.onerror works. All HUDService tests pass with this patch applied. We should file a bug about writing a test for catching errors from external JS, because we have no such test, at the moment.

I am sorry, but I have to give this patch f-. I hope you do not mind.

Comment 10

[Patrick Walton (:pcwalton)]

Bug 605492 essentially makes this bug obsolete. There will be no reason to listen for the onerror event once that bug lands.

If we wanted to use addEventListener(), we would need an extra IDL interface to extract the filename and line number from the nsIDOMEvent object. This seems like a waste of time to implement, given that the patch in bug 605492 obviates the need for the event listener anyhow.

Comment 11

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 3.

New patch simply removes window.onerror entirely.

Comment 12

[Mihai Sucan [:msucan]]

Can't give it f- nor f+. We need the replacement code which uses the stuff from bug 605492. We can't simply push this patch ... because it would cause a regression in terms of showing errors.

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 485106 [details] [diff] [review]
Proposed patch, version 3.

As discussed on IRC, this is only a part of the work. Giving it f+. Looking forward for the rest of the patch.

Comment 14

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2.

Patch part 2 makes the Console use the new nsIScriptError2 interface. A couple of opportunistic cleanups were made re. the error reporting, and a new unit test was added to check that external JavaScript errors make it to the Console. Feedback requested.

Comment 15

[Mihai Sucan [:msucan]]

Comment on attachment 485920 [details] [diff] [review]
Proposed patch, part 2.

This patch looks really good. Glad to see these pieces falling into place.


Comments:

When I applied the patch, Make failed. The patch is missing browser_webconsole_bug_597136_external_script_errors.js.


-    displayNode = this.getHeadsUpDisplay(hudId);
+    displayNode = this.getHeadsUpDisplay(aHUDId);
     outputNode = displayNode.querySelectorAll(".hud-output-node")[0];

I'm "troubled" by this piece of code in logConsoleActivity. For one, we rely on getHeadsUpDisplay() which is unreliable (see bug 603176), and two ... we already know the hudId, and we already have a HUD weakref. We don't have to search the HUD - we can access it directly. If you look into getHeadsUpDisplay() you'll see it searches the hudBox ... like we don't really know "where it is". Try:

outputNode = this.hudWeakReferences[aHUDId].get().outputNode;

(lastly, querySelectorAll()[0] is equivalent to querySelector(), but faster)


Regarding getHUDIdsForScriptError():

Overall it looks good, but let's think of the whole picture. We get an nsIScriptError2 message which comes from a unique window object (ID is given). We can only have a single HUD open for that window (which is always within a tab of a chrome window). This method can only return one hudId for such cases. It can return multiple IDs only if we don't have a window ID, and we use the old URI approach.

You do:

+        // And now for some pointer chasing to find the appropriate console.
+        let chrome = HUDService.getChromeWindowFromContentWindow(content);
+        let browser = chrome.gBrowser.getBrowserForDocument(content.document);
+        let notificationBox = chrome.gBrowser.getNotificationBox(browser);
+
+        let hudBox = notificationBox.querySelector(".hud-box");

We do somewhat similar pointer chasing in logConsoleActivity() when getHeadsUpDisplay() is called as well. I believe a much cleaner approach would be:

return [ HUDService.getHudIdByWindow(content) ];

This is minimal pointer hunting. Coupled with the HUD weak ref usage in logConsoleActivity() ... I think we'll actually boost Web Console performance when handling *lots* of messages.

That's all. Feedback+ with the above changes.

Comment 16

[Patrick Walton (:pcwalton)]

(In reply to comment #15)
> Overall it looks good, but let's think of the whole picture. We get an
> nsIScriptError2 message which comes from a unique window object (ID is given).
> We can only have a single HUD open for that window (which is always within a
> tab of a chrome window). This method can only return one hudId for such cases.
> It can return multiple IDs only if we don't have a window ID, and we use the
> old URI approach.

Yes, that's intentional. Do you want me to change it?

Comment 17

[Mihai Sucan [:msucan]]

(In reply to comment #16)
> (In reply to comment #15)
> > Overall it looks good, but let's think of the whole picture. We get an
> > nsIScriptError2 message which comes from a unique window object (ID is given).
> > We can only have a single HUD open for that window (which is always within a
> > tab of a chrome window). This method can only return one hudId for such cases.
> > It can return multiple IDs only if we don't have a window ID, and we use the
> > old URI approach.
> 
> Yes, that's intentional. Do you want me to change it?

It's fine to fallback - no problem. But the change I'd like to see ... is that the code uses getHudIdByWindow(content), instead of the lines that do pointer chasing.

Comment 18

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 2.

New patch addresses feedback. Review requested.

Comment 19

[Mihai Sucan [:msucan]]

Comment on attachment 486116 [details] [diff] [review]
Proposed patch, part 2, version 2.

A comment about:

+ let content = windowUtils.getOuterWindowWithId(windowID);

This can throw. If the windowID happens to be an ID for which getOuterWindowWithId() fails ... then it throws. I would suggest the use of try { ... }.

Comment 20

[Patrick Walton (:pcwalton)]

(In reply to comment #19)
> Comment on attachment 486116 [details] [diff] [review]
> Proposed patch, part 2, version 2.
> 
> A comment about:
> 
> + let content = windowUtils.getOuterWindowWithId(windowID);
> 
> This can throw. If the windowID happens to be an ID for which
> getOuterWindowWithId() fails ... then it throws. I would suggest the use of try
> { ... }.

In what circumstances does getOuterWindowWithId() fail?

Comment 21

[Mihai Sucan [:msucan]]

(In reply to comment #20)
> (In reply to comment #19)
> > Comment on attachment 486116 [details] [diff] [review] [details]
> > Proposed patch, part 2, version 2.
> > 
> > A comment about:
> > 
> > + let content = windowUtils.getOuterWindowWithId(windowID);
> > 
> > This can throw. If the windowID happens to be an ID for which
> > getOuterWindowWithId() fails ... then it throws. I would suggest the use of try
> > { ... }.
> 
> In what circumstances does getOuterWindowWithId() fail?

While working on the patch for bug 606498, I managed to get (during testing) window IDs that caused exceptions (window IDs for inner window objects). It's obvious such code was only temporary, but I would've liked to see the Web Console not "crash" when the window ID was wrong - since it has a fallback mechanism.

One case when getOuterWindowWithId() will probably fail ... is when the outer window is deleted by the time the Web Console code executes. So, having a try {...} wouldn't be bad for those cases.

Perhaps ask robcee or ddahl about this as well?

Comment 22

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 3.

Proposed patch, part 2, version 3 wraps the getOuterWindowWithId() in a defensive try block.

Comment 23

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

Comment on attachment 486116 [details] [diff] [review]
Proposed patch, part 2, version 2.

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>-   * get the nsIDOMNode outputNode via a nsIURI.spec
>+   * Gets the nsIDOMNode outputNode via a nsIURI.spec. Used by unit tests.

>   getDisplayByURISpec: function HS_getDisplayByURISpec(aURISpec)

Can we move it to the tests?

>+  logConsoleActivity: function HS_logConsoleActivity(aHUDId, aActivityObject)

>-    displayNode = this.getHeadsUpDisplay(hudId);
>-    outputNode = displayNode.querySelectorAll(".hud-output-node")[0];
>+    outputNode = this.hudWeakReferences[aHUDId].get().outputNode;

This is a very confusing change to review... It seems like we use "hud" to refer to different types of objects. this.hudWeakReferences[aHUDId] is apparently a HeadsUpDisplay object, while it looks like this.getHeadsUpDisplay(hudId) returns a HeadsUpDisplay's HUDBox (aka "displayNode"). It'd be nice if we could kill getHeadsUpDisplay entirely and just use this.hudWeakReferences[aHUDId].HUDBox (or outputNode or other property, where appropriate) consistently. Followup bug fodder.

>-  logActivity: function HS_logActivity(aType, aURI, aActivityObject)
>+  logActivity: function HS_logActivity(aType, aHUDId, aActivityObject)

You seem to have forgotten to update a caller:
http://hg.mozilla.org/mozilla-central/annotate/4d0f2c178e66/toolkit/components/console/hudservice/HUDService.jsm#l2394

>   {
>     var displayNode, outputNode, hudId;

Can you remove these while you're at it? They're unused.

>+  getHUDIdsForScriptError:
>+  function ConsoleUtils_getHUDIdsForScriptError(aScriptError) {
>+    let scriptError2 = aScriptError.QueryInterface(Ci.nsIScriptError2);
>+    if (scriptError2) {

Use an instanceof check for this: if (aScriptError instanceof Ci.nsIScriptError2)
QueryInterface in JS throws on failure, it never returns null.

>+      let windowID = scriptError2.outerWindowID;
>+      if (windowID) {
>+        // We just need some arbitrary window here so that we can GI to
>+        // nsIDOMWindowUtils...
>+        let someWindow = HUDService.currentContext();

Just use Services.ww.activeWindow? I'd like to kill currentContext() eventually.

>+        // Now find the appropriate console.
>+        let hudId = HUDService.getHudIdByWindow(content);
>+        if (hudId) {
>+          return [ hudId ];

Hmm, I didn't realize we were already keeping a ID->window mapping. This seems like it will fail if you open the console on a page with subframes, since those subframes won't be immediately tracked (unless you reload the page, triggering calls to windowInitializer for the new window creation). I guess at the very least we should fall back to finding the HUD ID using the same technique as windowInitializer (can factor that code out into a helper).

>+    // The error had no window ID. As a less precise fallback, see whether we
>+    // can find some consoles to send to via the URI.
>+    let hudIds = HUDService.uriRegistry[aScriptError.sourceName];
>+    return hudIds ? hudIds : [];

return HUDService.uriRegistry[aScriptError.sourceName] || [];

>   observe: function HCO_observe(aSubject, aTopic, aData)

>         default:
>-          HUDService.reportConsoleServiceMessage(aSubject);
>+          for (let i = 0; i < hudIds.length; i++) {
>+            HUDService.reportConsoleServiceMessage(hudIds[i], aSubject);
>+          }
>           return;

Is there any point in enumerating all of the other specific cases we want to report, given that they're now handled by the default case?

r- because of these issues, but this is otherwise a nice cleanup.

Comment 24

[Patrick Walton (:pcwalton)]

(In reply to comment #23)
> >+  logConsoleActivity: function HS_logConsoleActivity(aHUDId, aActivityObject)
> 
> >-    displayNode = this.getHeadsUpDisplay(hudId);
> >-    outputNode = displayNode.querySelectorAll(".hud-output-node")[0];
> >+    outputNode = this.hudWeakReferences[aHUDId].get().outputNode;
> 
> This is a very confusing change to review... It seems like we use "hud" to
> refer to different types of objects. this.hudWeakReferences[aHUDId] is
> apparently a HeadsUpDisplay object, while it looks like
> this.getHeadsUpDisplay(hudId) returns a HeadsUpDisplay's HUDBox (aka
> "displayNode"). It'd be nice if we could kill getHeadsUpDisplay entirely and
> just use this.hudWeakReferences[aHUDId].HUDBox (or outputNode or other
> property, where appropriate) consistently. Followup bug fodder.

You aren't the first to notice :) It's bug 592469.

> >+        // Now find the appropriate console.
> >+        let hudId = HUDService.getHudIdByWindow(content);
> >+        if (hudId) {
> >+          return [ hudId ];
> 
> Hmm, I didn't realize we were already keeping a ID->window mapping. This seems
> like it will fail if you open the console on a page with subframes, since those
> subframes won't be immediately tracked (unless you reload the page, triggering
> calls to windowInitializer for the new window creation).

Indeed, it's a very imprecise fallback.

> I guess at the very
> least we should fall back to finding the HUD ID using the same technique as
> windowInitializer (can factor that code out into a helper).

Aha, good idea.

> >   observe: function HCO_observe(aSubject, aTopic, aData)
> 
> >         default:
> >-          HUDService.reportConsoleServiceMessage(aSubject);
> >+          for (let i = 0; i < hudIds.length; i++) {
> >+            HUDService.reportConsoleServiceMessage(hudIds[i], aSubject);
> >+          }
> >           return;
> 
> Is there any point in enumerating all of the other specific cases we want to
> report, given that they're now handled by the default case?

I kept it in as a sort of documentation, but perhaps it'd just be best to move
it all to a comment block above "default:" and remove the extra cases.

Comment 25

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 4.

New patch addresses comments.

Comment 26

[Patrick Walton (:pcwalton)]

(In reply to comment #23)
> >   getDisplayByURISpec: function HS_getDisplayByURISpec(aURISpec)
> 
> Can we move it to the tests?

I went ahead and removed it entirely. It's a very imprecise way to get the console in the first place, since there could be multiple tabs and/or windows open with the same URI.

> Just use Services.ww.activeWindow? I'd like to kill currentContext()
> eventually.

I killed currentContext().

> Hmm, I didn't realize we were already keeping a ID->window mapping. This seems
> like it will fail if you open the console on a page with subframes, since those
> subframes won't be immediately tracked (unless you reload the page, triggering
> calls to windowInitializer for the new window creation). I guess at the very
> least we should fall back to finding the HUD ID using the same technique as
> windowInitializer (can factor that code out into a helper).

We have a helper: HUDService.getChromeWindowFromContentWindow(). I modified getHudIdByWindow() to use it.

> Is there any point in enumerating all of the other specific cases we want to
> report, given that they're now handled by the default case?

Moved all of the cases into comments.

Comment 27

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 5.

Forgot to qrefresh. Here's the new version.

Comment 28

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

Comment on attachment 487006 [details] [diff] [review]
Proposed patch, version 5.

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>   getHudIdByWindow: function HS_getHudIdByWindow(aContentWindow)

>+    // As a fallback, do a little pointer chasing to try to find the Web
>+    // Console. This fallback approach occurs when opening the Console on a
>+    // page with subframes.
>+    let chrome = HUDService.getChromeWindowFromContentWindow(aContentWindow);
>+    let gBrowser = chrome.gBrowser;
>+    let browser = gBrowser.getBrowserForDocument(aContentWindow.document);

Hmm, I forgot about getChromeWindowFromContentWindow when reviewing the changes to windowInitializer. I'm wary of scope creep here (don't want to bundle in too many changes in the same bug), but we really should just have a getParents(aContentWindow) function which returns the chrome window, the xul:tabbrowser, and the xul:browser element that contain aContentWindow. It can just be:

function getParents(aContentWindow) {
  var chromeEventHandler = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                                         .getInterface(Ci.nsIWebNavigation)
                                         .QueryInterface(Ci.nsIDocShell)
                                         .chromeEventHandler;
  let chromeWindow = chromeEventHandler.ownerDocument.defaultView;
  let gBrowser = XPCNativeWrapper.unwrap(chromeWindow).gBrowser; // unwrap may not be needed anymore?
  let docElem = chromeWindow.document.documentElement;
  if (!docElem || !gBrowser ||
      docElem.getAttribute("windowtype") != "navigator:browser")
    return [chromeWindow, null, null]; // not a browser window...

  return [chromeWindow, gBrowser, chromeEventHandler]; // chromeEventHandler is a xul:browser
}

>+    this.windowRegistry[hudId].push(aContentWindow);  // Cache it!

Don't you need to also update uriRegistry? I'd suggest calling registerDisplay, but that does a bunch of stuff that seems undesirable here (touches displayRegistry, filterPrefs, calls registerActiveContext, etc.). I think we should probably get rid of the uriRegistry entirely, but I guess I'll file a followup on that.

>-                let gBrowser = HUDService.currentContext().gBrowser;
>+                let type = "navigator:browser";
>+                let browserWindow = Services.wm.getMostRecentWindow(type);
>+                let gBrowser = browserWindow;

I don't know exactly what this is trying to do, but it almost certainly shouldn't just be using the most recent browser window. Can we move the currentContext removal to bug 583107 and sort this out properly? It's really unrelated to the current bug, sorry if I implied you should do it now :)

>+  function ConsoleUtils_getHUDIdsForScriptError(aScriptError) {
>+    if (aScriptError instanceof Ci.nsIScriptError2) {
>+      let scriptError2 = aScriptError.QueryInterface(Ci.nsIScriptError2);

QI isn't necessary, instanceof does an implicit QI and XPConnect remembers that state and flattens interfaces on the object automatically.

> HeadsUpDisplayUICommands = {
>   toggleHUD: function UIC_toggleHUD() {
>-    var window = HUDService.currentContext();
>+    var window = Services.ww.activeWindow;

This should just take a chromeWindow argument rather than using activeWindow. Will require a rejigging of the closebutton listener, but the other callers can easily just pass in "window".

>   observe: function HCO_observe(aSubject, aTopic, aData)

>+        // Display the messages from the following categories:

I think this comment might be better off in a bug than in the code - it's likely to get out of date quickly, and already is somewhat misleading (suggests that the reason errors don't show up is that they're missing sourceName, when really the fix is to make them nsIScriptError2s and give them window IDs).

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_warn_user_about_replaced_api.js b/toolkit/components/console/hudservice/tests/browser/browser_warn_user_about_replaced_api.js

>-  var display = HUDService.getDisplayByURISpec(content.location.href);
>-  var outputNode = display.querySelectorAll(".hud-output-node")[0];
>-
>+  outputNode = HUDService.hudWeakReferences[hudId].get().outputNode;

ugh, head.js really shouldn't declare that all of those random variables, just makes things confusing. followup, I guess.

Comment 29

[Mihai Sucan [:msucan]]

(In reply to comment #28)
> Comment on attachment 487006 [details] [diff] [review]
> Proposed patch, version 5.
> 
> >-                let gBrowser = HUDService.currentContext().gBrowser;
> >+                let type = "navigator:browser";
> >+                let browserWindow = Services.wm.getMostRecentWindow(type);
> >+                let gBrowser = browserWindow;
> 
> I don't know exactly what this is trying to do, but it almost certainly
> shouldn't just be using the most recent browser window. Can we move the
> currentContext removal to bug 583107 and sort this out properly? It's really
> unrelated to the current bug, sorry if I implied you should do it now :)

The use of currentContext() in this piece of network logging code is fixed by bug 600183 - which is waiting for checkin, after b7. 


> >   observe: function HCO_observe(aSubject, aTopic, aData)
> 
> >+        // Display the messages from the following categories:
> 
> I think this comment might be better off in a bug than in the code - it's
> likely to get out of date quickly, and already is somewhat misleading (suggests
> that the reason errors don't show up is that they're missing sourceName, when
> really the fix is to make them nsIScriptError2s and give them window IDs).

It's not yet outdated. There are indeed, nsIScriptError messages that come without any sourceName. Once the patches from bug 606498 are reviewed and landed, they'll fix most of that stuff.

The categories list should go at least in a bug number, with a reference in the code, or into an MDN wiki page (again with a link in the code).


(sorry for "jumping in", I just wanted to comment on the two aspects that are related to code I wrote.)

Comment 30

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

(In reply to comment #29)
> The use of currentContext() in this piece of network logging code is fixed by
> bug 600183 - which is waiting for checkin, after b7. 

Cool - more reason to postpone the currentContext change, I think.

> It's not yet outdated. There are indeed, nsIScriptError messages that come
> without any sourceName. Once the patches from bug 606498 are reviewed and
> landed, they'll fix most of that stuff.

Also good! It was the "since no sourceName is given" part of the comment that seems misleading now, given bug 605492/bug 606498. The comment is useful, I just don't think it needs to live in the code, where it's likely to get out of date if/when the core code it references changes. I think it serves its purpose as long as its findable in a bug (via hg blame).

(Your comments here are both useful and relevant, no need to apologize! :)

Comment 31

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 6.

New patch addresses review comments.

Comment 32

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

Comment on attachment 487635 [details] [diff] [review]
Proposed patch, version 6.

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>   getHudIdByWindow: function HS_getHudIdByWindow(aContentWindow)

>+    let parents = ConsoleUtils.getParents(aContentWindow);

let [win, tabBrowser, browser] = ConsoleUtils.getParents(aContentWindow);

>+  function HS_reportConsoleServiceMessage(aHUDId, aConsoleMessage)
>+    this.logActivity("console-listener", aHUDId, aConsoleMessage);

Doesn't really seem like there's any reason to keep reportConsoleServiceMessage if all it does is call logActivity. Just call logActivity directly from HCO_observe?

>+  getHUDIdsForScriptError:

>+        let someWindow = Services.ww.activeWindow;
>+        someWindow = someWindow.QueryInterface(Ci.nsIInterfaceRequestor);
>+        let windowUtils = someWindow.getInterface(Ci.nsIDOMWindowUtils);

style nit: I'd just write this as:
let windowUtils = Services.ww.activeWindow
                          .QueryInterface(Ci.nsIInterfaceRequestor)
                          .getInterface(Ci.nwIDOMWindowUtils);

>+        // It's possible that this error could be an inner window

Do we know under what conditions this occurs? Seems like it'd be a bug... worth a followup perhaps?

>+   * Returns the chrome window, the tab browser, and the browser that
>+   * contain the given content window.
>+   *
>+   * @param nsIDOMWindow aContentWindow
>+   *        The content window to query.
>+   * @returns Array<nsISupports>

They aren't nsISupports, they're direct references to the elements, so I'd just remove this @returns. Also maybe mention that this depends on Firefox-specific internals (i.e. the presence of gBrowser as a global property on the window).

r=me with these addressed, woohoo!

Comment 33

[Patrick Walton (:pcwalton)]

(In reply to comment #32)
> >+        // It's possible that this error could be an inner window
> 
> Do we know under what conditions this occurs? Seems like it'd be a bug... worth
> a followup perhaps?

See comment 21.

> 
> >+   * Returns the chrome window, the tab browser, and the browser that
> >+   * contain the given content window.
> >+   *
> >+   * @param nsIDOMWindow aContentWindow
> >+   *        The content window to query.
> >+   * @returns Array<nsISupports>
> 
> They aren't nsISupports, they're direct references to the elements, so I'd just
> remove this @returns. Also maybe mention that this depends on Firefox-specific
> internals (i.e. the presence of gBrowser as a global property on the window).

Well, I thought the plan was to move HUDService.jsm to browser/ at some point anyway, in which case marking Firefox-specific features won't be useful. There's already been a fair amount of Firefox specificity added throughout; I'm not sure how feasible it is to dissociate it at this point.

Comment 34

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 7.

Version 7 of the patch addresses the review comments.

Comment 35

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

(In reply to comment #33)
> See comment 21.

That doesn't say much more than "this happens", and I'd like to understand why it happens. But I can sort that out later.

> Well, I thought the plan was to move HUDService.jsm to browser/ at some point
> anyway, in which case marking Firefox-specific features won't be useful.
> There's already been a fair amount of Firefox specificity added throughout; I'm
> not sure how feasible it is to dissociate it at this point.

Right, we are already dependent on Firefox in a bunch of places. But I think it is useful to indicate those places when possible so that someone doing the work of removing those ties has an easier time of it. I don't think it's an unfeasible task.

Comment 36

[Patrick Walton (:pcwalton)]

(In reply to comment #35)
> (In reply to comment #33)
> > See comment 21.
> 
> That doesn't say much more than "this happens", and I'd like to understand why
> it happens. But I can sort that out later.

Mihai, could you weigh in? I've never seen that error myself, and I'd like to remove the try block if we can. I'm not a fan of adding try blocks when we don't know exactly what can cause the errors... gratuitous use of "try" has masked a lot of issues in the Web Console code before.

Comment 37

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 8.

New version of the patch adds the note about the Firefox dependency to getParents().

Comment 38

[Mihai Sucan [:msucan]]

(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #33)
> > > See comment 21.
> > 
> > That doesn't say much more than "this happens", and I'd like to understand why
> > it happens. But I can sort that out later.
> 
> Mihai, could you weigh in? I've never seen that error myself, and I'd like to
> remove the try block if we can. I'm not a fan of adding try blocks when we
> don't know exactly what can cause the errors... gratuitous use of "try" has
> masked a lot of issues in the Web Console code before.

As explained, during development work on bug 606498 I had the chance of giving wrong window IDs to the HUDService. Again, such issues were only *temporary*. Still, I did not expect the Web Console *breaks* because of that, knowing it has a fallback mechanism.

The above begs the question: "well if it was only temporary, and if such issue never should really occur, then why add the try?" The answer to that is ... we can get window IDs to window objects that no longer exist by the time the HUDService code is executed. Please read bug 605492 comment 22. The getOuterWindowWithId() method can return null, and the code in this patch, without the try, doesn't take into consideration the possibility of the method returning null.

I agree with not making *gratuitous* use of "try". Ultimately, I leave the decision up to you and Gavin.

Comment 39

[Mihai Sucan [:msucan]]

Interesting discovery:

I just tried the test from bug 595889 and it fails worse with this patch applied.

Problem: Services.ww.activeWindow is null and the code throws. Why? The alert() causes activeWindow to become null.

This patch needs to be updated to not use activeWindow.

Comment 40

[Patrick Walton (:pcwalton)]

(In reply to comment #38)
> (In reply to comment #36)
> > (In reply to comment #35)
> > > (In reply to comment #33)
> > > > See comment 21.
> > > 
> > > That doesn't say much more than "this happens", and I'd like to understand why
> > > it happens. But I can sort that out later.
> > 
> > Mihai, could you weigh in? I've never seen that error myself, and I'd like to
> > remove the try block if we can. I'm not a fan of adding try blocks when we
> > don't know exactly what can cause the errors... gratuitous use of "try" has
> > masked a lot of issues in the Web Console code before.
> 
> As explained, during development work on bug 606498 I had the chance of giving
> wrong window IDs to the HUDService. Again, such issues were only *temporary*.
> Still, I did not expect the Web Console *breaks* because of that, knowing it
> has a fallback mechanism.
> 
> The above begs the question: "well if it was only temporary, and if such issue
> never should really occur, then why add the try?" The answer to that is ... we
> can get window IDs to window objects that no longer exist by the time the
> HUDService code is executed. Please read bug 605492 comment 22. The
> getOuterWindowWithId() method can return null, and the code in this patch,
> without the try, doesn't take into consideration the possibility of the method
> returning null.
> 
> I agree with not making *gratuitous* use of "try". Ultimately, I leave the
> decision up to you and Gavin.

But the window is guaranteed to be alive when we catch the error, because error reporting is synchronous? Bug 605492 comment 19 seems to support this. When e10s lands, then there could be a race between a window getting destroyed and our receiving the error. But as I understand it, in the pre-Electrolysis world, error reporting should be synchronous and so there's no way the window could be destroyed before we catch the error.

I'm totally in support of future-proofing against e10s, but in that case IMO we should put a note in the code stating that this is what the try block is for.

Comment 41

[Patrick Walton (:pcwalton)]

(In reply to comment #39)
> Interesting discovery:
> 
> I just tried the test from bug 595889 and it fails worse with this patch
> applied.
> 
> Problem: Services.ww.activeWindow is null and the code throws. Why? The alert()
> causes activeWindow to become null.
> 
> This patch needs to be updated to not use activeWindow.

Weird. The documentation here [1] states:

> The Watcher serves as a global storage facility for the current active (front
> most non-floating-palette-type) window, storing and returning it on demand.
> Users must keep this attribute current, including after the topmost window is
> closed. This attribute obviously can return null if no windows are open, but
> should otherwise always return a valid window.

So we have Services.ww.activeWindow returning null when there are windows open. This seems like a platform bug to me.

[1]: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIWindowWatcher

Comment 42

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

It doesn't hurt much to be defensive with regard to getOuterWindowWithId returning null, so let's just do that (with an explicit check rather than a try/catch).

The activeWindow issue does seem odd. It's rather annoying that we need to get to a window for this at all. Regardless of whether the focus code is buggy, we don't really care about focus/activation at all, so let's just use Services.wm.getMostRecentWindow(null), and to be extra safe we could even handle that being null as well.

windowInitializer can also use getParents (being added by this patch), we should get a followup filed on that (and perhaps on cleaning up the windowInitializer in general - I have some ideas).

Comment 43

[Mihai Sucan [:msucan]]

(In reply to comment #42)
> It doesn't hurt much to be defensive with regard to getOuterWindowWithId
> returning null, so let's just do that (with an explicit check rather than a
> try/catch).
> 
> The activeWindow issue does seem odd. It's rather annoying that we need to get
> to a window for this at all. Regardless of whether the focus code is buggy, we
> don't really care about focus/activation at all, so let's just use
> Services.wm.getMostRecentWindow(null), and to be extra safe we could even
> handle that being null as well.
> 
> windowInitializer can also use getParents (being added by this patch), we
> should get a followup filed on that (and perhaps on cleaning up the
> windowInitializer in general - I have some ideas).

I looked a bit through the code of the focus manager and that of nsGlobalWindow ... on how they deal with lowering and raising windows. As it looks to me, activeWindow can be null. I don't know why the documentation states it should not be null, because by the looks of the code, "yes, it can be null".

Anyhow, it would require more investigation into the Gecko code to see *why* it's null at that precise moment (when the alert() is shown).

What you suggest seems really reasonable.

Comment 44

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

We should probably update the documentation for ns(I)WindowWatcher to save people this struggle in the future.

Nice work!

Comment 45

[Patrick Walton (:pcwalton)]

(In reply to comment #44)
> We should probably update the documentation for ns(I)WindowWatcher to save
> people this struggle in the future.
> 
> Nice work!

Just did.

Comment 46

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 9.

New version of the patch adds null checks to getOuterWindowWithId() and uses getMostRecentWindow() instead of activeWindow.

Comment 47

[Mihai Sucan [:msucan]]

Comment on attachment 488957 [details] [diff] [review]
Proposed patch, version 9.

I've been playing with this patch. I found that:

+  getParents: function ConsoleUtils_getParents(aContentWindow) {
+    let chromeEventHandler =
+      aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
+                    .getInterface(Ci.nsIWebNavigation)
+                    .QueryInterface(Ci.nsIDocShell)
+                    .chromeEventHandler;

This throws. chromeEventHandler can be null. Try to execute only the mochitest file browser_webconsole_bug_595934_message_categories.js.

If I run all of the HUDService tests at once, no error occurs. However, when I only run the aforementioned test, then it fails.

This should be investigated, before it is checked-in.

Comment 48

[Patrick Walton (:pcwalton)]

Removing checkin-needed while we investigate. (gavin, anything come to mind?)

Comment 49

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 10.

Patch version 10 fixes the issue. The problem is that chrome windows could end up being passed to getParents() when the Web Console receives messages. Now getHudIdForWindow() explicitly checks to see whether the window is a chrome window and avoids passing it to getParents().

A new testcase has been added. Re-review requested.

Comment 50

[Patrick Walton (:pcwalton)]

(In reply to comment #49)
> Created attachment 489591 [details] [diff] [review]
> Proposed patch, version 10.
> 
> Patch version 10 fixes the issue. The problem is that chrome windows could end
> up being passed to getParents() when the Web Console receives messages.

Here by "messages" I mean "network events".

Comment 51

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

Comment on attachment 489591 [details] [diff] [review]
Proposed patch, version 10.

I'd rather a null check on chromeEventHandler in getParents that early-returns [null,null,null] (with associated comment tweaks).

Comment 52

[Patrick Walton (:pcwalton)]

Attachment: [backed-out] Proposed patch, version 11.

Patch version 11 moves the check to getParents().

Comment 53

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

Comment on attachment 489868 [details] [diff] [review]
[backed-out] Proposed patch, version 11.

http://hg.mozilla.org/mozilla-central/rev/563c20646a43

Comment 54

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

Comment on attachment 489868 [details] [diff] [review]
[backed-out] Proposed patch, version 11.

http://hg.mozilla.org/mozilla-central/rev/1023c324e6be

leaky. Logs:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289568701.1289571253.14933.gz&fulltext=1

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289568668.1289570968.13424.gz&fulltext=1

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289569945.1289572419.20599.gz

Comment 55

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

taking a look to see if I can spot the cause of the leak. If I can get something ready, I will push to try before 9am PT, assuming the tree has reopened and is available...

Comment 56

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

pushed http://hg.mozilla.org/try/rev/a5084e226dc7... awaiting results.

Comment 57

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

Attachment: version 11.1

my changes, sent to try

Comment 58

[Nochum Sossonko [:Natch]]

(In reply to comment #56)
> pushed http://hg.mozilla.org/try/rev/a5084e226dc7... awaiting results.

http://tbpl.mozilla.org/?tree=MozillaTry&rev=a5084e226dc7 for convenience :)

Comment 59

[Patrick Walton (:pcwalton)]

Attachment: [checked-in] Proposed patch, version 12.

New version of the patch changes the explicit check for an instance of nsIDOMChromeWindow to feature detection on chromeEventHandler. It also fixes a leak involving a global variable called "outputNode" mistakenly being created.

Comment 60

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

Comment on attachment 490176 [details] [diff] [review]
[checked-in] Proposed patch, version 12.

http://hg.mozilla.org/mozilla-central/rev/65a5f743eaab