597151 - The HUDConsoleObserver is never removed

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

The HUDConsoleObserver remains active forever after the Web Console is open,
even after closing all Web Consoles. This means the code in the HUDService is always executed when nsIConsoleMessages are received. The code throws, at times. For example, on Google.com I get:

Error: [Exception... "'Error: Cannot get outputNode by id' when calling method: [nsIConsoleListener::observe]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "<unknown>"  data: no]

This happens because the code tries to display the CSS parser warnings triggered by the web page, but there is no Web Console to show them.

Comment 1

[David Dahl :ddahl]

(In reply to comment #0)
> This happens because the code tries to display the CSS parser warnings
> triggered by the web page, but there is no Web Console to show them.

I'm pretty sure I had this in a try catch before the initial review, and most of the try catches were removed during the review. we should add a new try catch to ignore these errors. We can also null the observers and HUDService once all consoles are closed, but that will not fix this problem in all cases.

Comment 2

[Mihai Sucan [:msucan]]

Opting for try can only get us that far. A try only hides problems. I think we can check if we have the Web Console and simply not throw.

Removing the observer once all consoles are closed doesn't entirely solve the issue, indeed. I think the error still shows when a CSS parser warning pops in a tab where there is no Web Console.

The fix is to do both things - don't fail if there's no Web Console in the tab, and remove the observer once it's no longer needed.

Comment 3

[Patrick Walton (:pcwalton)]

Attachment: WIP patch.

WIP patch attached. No unit tests yet, but informal testing suggests that the bug is fixed.

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 476472 [details] [diff] [review]
WIP patch.

I'm glad you've taken the bug. Thanks!

Patch looks good. I don't see any errors coming from the observer now. Yay :). It would be even nicer if the observer would also be removed when the last display is unregistered.

Looking forward for the unit test.

Comment 5

[Patrick Walton (:pcwalton)]

Nominating for blocking2.0 because this results in a lot of spam in the Error Console.

Comment 6

[Kevin Dangoor]

Assigning to Mihai to confirm that the problem still exists and that the patch is still good for it.

Comment 7

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

The patch is bitrotten by bug 597136. Also I think it doesn't actually fix this bug as summarized, since it doesn't remove the handler after all HUDs are closed, but rather just makes sure that it doesn't throw in that cases. Bug 597136's patch should have the same effect.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to comment #7)
> The patch is bitrotten by bug 597136. Also I think it doesn't actually fix this
> bug as summarized, since it doesn't remove the handler after all HUDs are
> closed, but rather just makes sure that it doesn't throw in that cases. Bug
> 597136's patch should have the same effect.

Agreed. The approach used here is to simply avoid causing exceptions.

I'll change the patch to actually remove the HUDConsoleObserver when all Web Consoles are closed.

Comment 9

[Mihai Sucan [:msucan]]

Attachment: proposed patch

New proposed patch. What it does:

- introduces a way to "suspend" and "wakeup" the Web Console (HUDService).
- it wakes up when the first Web Console is open.
- it suspends when the last Web Console is closed.

The HUDService is "suspended" when it has no HUDConsoleObserver listener registered to the nsIConsoleService, no HUDWindowObserver, and no httpObserver active. This means the HUDService no longer lowers the overall browser performance when it's closed from all windows/tabs, having no such observers active.

Changes were quite minimal, I would've expected it requires more work. I hope you like it. Feedback for improvements is welcome. Thanks!

Comment 10

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

Comment on attachment 487591 [details] [diff] [review]
proposed patch

I wish I had more to say about this patch, but after going through it, it looks pretty clean to me.

I feel like this *should* work without actually testing it, but I'd like a run through try to see if there any leaks reported. I'm worried that adding additional semantics to our activation code (e.g., wakeup, suspend, activate, deactivate) are over-complicating things however.

Also, feel free to use "let" instead of "var" inside functions. They're preferred.

Comment 11

[Mihai Sucan [:msucan]]

(In reply to comment #10)
> Comment on attachment 487591 [details] [diff] [review]
> proposed patch
> 
> I wish I had more to say about this patch, but after going through it, it looks
> pretty clean to me.
> 
> I feel like this *should* work without actually testing it, but I'd like a run
> through try to see if there any leaks reported. I'm worried that adding
> additional semantics to our activation code (e.g., wakeup, suspend, activate,
> deactivate) are over-complicating things however.

Thanks for the feedback+! I tried to keep the patch as "surgical" as possible.

With regards to adding more "semantics" (wakeup and suspend), I was given the option to add only HUDConsoleObserver.uninit() and "get the job done" for this bug report. But then, that wouldn't have been the ideal solution in my opinion. We need to stop the HUDWindowObserver and the httpObserver as well. Practically, we need to stop any observer/listener we have, when all consoles are closed - "suspend" the HUDService. The "activate" and "deactivate" semantics are strictly for tabs/contexts.

> Also, feel free to use "let" instead of "var" inside functions. They're
> preferred.

I used var because locally in the functions I worked with, they already had vars. I favored local consistency, instead of mixing the two uses.

I'll push the patch to the try server today.

Comment 12

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

yep, I understood the reasoning behind the additional control semantics and can see why it's necessary. I think the comments are helpful enough for distinguishing these cases.

I know we've got a bunch of var declarations, but figured we could replace them as we go. Not a huge deal and there's prior usage in the file as you mentioned, but they can be the source of some confusing behavior if we're not careful.

Looking forward to the probably excellent try results. :)

Comment 13

[Mihai Sucan [:msucan]]

The try server results seem to look fine:

http://hg.mozilla.org/try/pushloghtml?changeset=4c181878dba5
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4c181878dba5

Will ask for review now.

Comment 14

[Mihai Sucan [:msucan]]

Comment on attachment 487591 [details] [diff] [review]
proposed patch

Asking for review. This patch makes the HUDService "cleanup" after itself when all Web Consoles are closed.

Comment 15

[Mihai Sucan [:msucan]]

Uh oh, the irony:

- I cannot reproduce this bug with the mozilla-central default branch.
- but I can reproduce this bug with the patch from bug 597151.

I last pulled from m-c on the 1st of November (changeset 57134:8957830e22a8). Situation with the debug build on my system (at the moment):

- browser_webconsole_bug_594497_history_arrow_keys.js runs fine but it doesn't complete all tests in the time allocated, and it times out, even if it runs fine. I can work around the issue only by calling requestLongerTimeout() in the test file.

- browser_webconsole_bug_595350_multiple_windows_and_tabs.js and
- browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js
both crash:
###!!! ASSERTION: invalid array index: 'i < Length()', file ../../../dist/include/nsTArray.h, line 339

(no work around, just remove the tests from the list)

- and bug 607325: browser_webconsole_bug_594477_clickable_output.js also always fails for me, as explained there. I work around the issue by removing the type=content attribute from the network panel iframe.

The above problems apply to both debug builds from the mozilla-central default branch *and* to builds that hold the patch from bug 597151. No differences there.

If I work around the above problems: no memleaks in either build, when running the Web Console tests.

It is interesting to note that the Mozilla build and test bots have no problems with the default branch, nor with the patch from bug 597151.

Comment 16

[Mihai Sucan [:msucan]]

(argh, this last comment was meant for bug 606055. sorry!)

Comment 17

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

Comment on attachment 487591 [details] [diff] [review]
proposed patch

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

>   activateHUDForContext: function HS_activateHUDForContext(aContext)
>   {
>+    if (Object.keys(this._headsUpDisplays).length == 0) {
>+      this.wakeup();
>+    }

Seems like the check should just be in wakeup(), so that you can just call it unconditionally here (and so that things don't go wrong if it starts being called elsewhere).

>     this.windowRegistry[id].forEach(function(aContentWindow) {
>-      if (aContentWindow.wrappedJSObject.console instanceof HUDConsole) {
>-        delete aContentWindow.wrappedJSObject.console;
>+      var wrappedJSObject = aContentWindow.wrappedJSObject;
>+      if (wrappedJSObject.console &&
>+          wrappedJSObject.console.constructor === HUDConsole) {
>+        delete wrappedJSObject.console;
>       }
>     });

Bug 587734 removes this code entirely, fwiw.

>+    if (specHudArr) {
>+      for (var i = 0; i < specHudArr.length; i++) {
>+        if (specHudArr[i] == id) {
>+          specHudArr.splice(i, 1);
>+        }
>       }
>     }

This would be much simpler with filter: 
this.uriRegistry[uri] = this.uriRegistry[uri].filter(function (e) e != id);

>+    this.initialConsoleCreated = false;

This too is removed by bug 587734.

Comment 18

[Mihai Sucan [:msucan]]

(In reply to comment #17)
> Comment on attachment 487591 [details] [diff] [review]
> proposed patch
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >   activateHUDForContext: function HS_activateHUDForContext(aContext)
> >   {
> >+    if (Object.keys(this._headsUpDisplays).length == 0) {
> >+      this.wakeup();
> >+    }
> 
> Seems like the check should just be in wakeup(), so that you can just call it
> unconditionally here (and so that things don't go wrong if it starts being
> called elsewhere).

Good point. Thanks! Will update the patch.


> >     this.windowRegistry[id].forEach(function(aContentWindow) {
> >-      if (aContentWindow.wrappedJSObject.console instanceof HUDConsole) {
> >-        delete aContentWindow.wrappedJSObject.console;
> >+      var wrappedJSObject = aContentWindow.wrappedJSObject;
> >+      if (wrappedJSObject.console &&
> >+          wrappedJSObject.console.constructor === HUDConsole) {
> >+        delete wrappedJSObject.console;
> >       }
> >     });
> 
> Bug 587734 removes this code entirely, fwiw.

Yes, indeed. I cannot be sure when the patch lands. I'll rebase it when needed.

> >+    if (specHudArr) {
> >+      for (var i = 0; i < specHudArr.length; i++) {
> >+        if (specHudArr[i] == id) {
> >+          specHudArr.splice(i, 1);
> >+        }
> >       }
> >     }
> 
> This would be much simpler with filter: 
> this.uriRegistry[uri] = this.uriRegistry[uri].filter(function (e) e != id);

True. Will update the patch accordingly.

Thanks for your comments Gavin! Also, thanks to Dave for the r+!

Comment 19

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Patch is bitrotted.

Comment 21

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch.

Comment 22

[Mihai Sucan [:msucan]]

Attachment: [checked-in] rebased patch

Another patch rebase.

Comment 23

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

Comment on attachment 491841 [details] [diff] [review]
[checked-in] rebased patch

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