Closed Bug 597151 Opened 12 years ago Closed 12 years ago

The HUDConsoleObserver is never removed

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: [patchclean:1119])

Attachments

(1 file, 4 obsolete files)

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.
(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.
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.
Attached patch WIP patch. (obsolete) — Splinter Review
WIP patch attached. No unit tests yet, but informal testing suggests that the bug is fixed.
Assignee: nobody → pwalton
Status: NEW → ASSIGNED
Attachment #476472 - Flags: feedback?(mihai.sucan)
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.
Attachment #476472 - Flags: feedback?(mihai.sucan) → feedback+
Blocks: devtools924
No longer blocks: devtools924
blocking2.0: --- → ?
Nominating for blocking2.0 because this results in a lot of spam in the Error Console.
blocking2.0: ? → betaN+
Assigning to Mihai to confirm that the problem still exists and that the patch is still good for it.
Assignee: pwalton → mihai.sucan
Blocks: devtools4
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.
(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.
Attached patch proposed patch (obsolete) — Splinter Review
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!
Attachment #476472 - Attachment is obsolete: true
Attachment #487591 - Flags: feedback?(rcampbell)
Whiteboard: [patchclean:1102]
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.
Attachment #487591 - Flags: feedback?(rcampbell) → feedback+
(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.
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 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.
Attachment #487591 - Flags: review?(dtownsend)
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.
(argh, this last comment was meant for bug 606055. sorry!)
Attachment #487591 - Flags: review?(dtownsend) → review+
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.
(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+!
Attached patch updated patch (obsolete) — Splinter Review
Updated patch.
Attachment #487591 - Attachment is obsolete: true
Whiteboard: [patchclean:1102] → [patchclean:1108]
Keywords: checkin-needed
Whiteboard: [patchclean:1108] → [patchbitrot]
Attached patch rebased patch (obsolete) — Splinter Review
Rebased patch.
Attachment #488847 - Attachment is obsolete: true
Whiteboard: [patchbitrot] → [patchclean:1115]
Whiteboard: [patchclean:1115] → [patchclean:1115][checkin]
Whiteboard: [patchclean:1115][checkin] → [needs-rebase:1119]
Another patch rebase.
Attachment #490592 - Attachment is obsolete: true
Whiteboard: [needs-rebase:1119] → [patchclean:1119][checkin]
Blocks: 601909
Comment on attachment 491841 [details] [diff] [review]
[checked-in] rebased patch

http://hg.mozilla.org/mozilla-central/rev/ad46adf99fc3
Attachment #491841 - Attachment description: rebased patch → [checked-in] rebased patch
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:1119][checkin] → [patchclean:1119]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.