611789 - Web Console cleanup: Remove the window registry

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Patrick Walton (:pcwalton)]

The window registry shouldn't be keeping strong references to content windows, as this is brittle. We now have window IDs that we can use instead, so that leaks won't be as catastrophic.

Comment 1

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 2

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch.

Please see bug 594523 comment #14. This patch does the following:

- removes various registries from the HUDService: _headsUpDisplays, displayRegistry, windowRegistry and uriRegistry.

- we still keep: activatedContext (to keep track for which notification box IDs we are open) and hudReferences (hudIds mapped to HUD objects).

- adds a new windowIds object which maps outer top window IDs to hudIds.

- removes the deleteHeadsUpDisplay() and displays() methods.

- simplifies the registerDisplay(), unregisterDisplay(), getHudIdByWindow() methods.

- keeps the getHeadsUpDisplay(), getOutputNodeById() and displaysIndex() methods *only* for compatibility with older tests. changed them to use the new internals, such that they no longer pose technical issues.

If you want I can go ahead and remove these methods entirely, but that would cause more "patch noise": I'll have to update all code that uses these three methods.

- removed fallback code from getHudIdByWindow() ... which no longer applies (we removed uriRegistry).

- removed the getHUDIdsForScriptError() and getParents() methods, because they are no longer relevant.

All these changes seem big, but they seem to have minimal impact. Only three tests failed, and this patch updates those files as needed.

This cleanup should fix bug 594523 and bug 592469 as well. I believe it is much less error prone, and clearer to maintain, less issues to mix up.

Finally, I'd add that this patch now makes the HUDService entirely reliant on the new nsIScriptError2, and on the outerWindowID of such messages. Things are much cleaner because of this, but it also means we must first get bugs 603706, 603750 and 606498 ready and checked-in first. Otherwise, we'll loose error/warning messages.

Looking forward for your feedback! Thanks!

Comment 3

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

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

this looks nice, mihai.

Comment 4

[Mihai Sucan [:msucan]]

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

Thanks for your quick feedback+ Robert!

Asking for review from Gavin.

Comment 5

[Mihai Sucan [:msucan]]

Asking for blocking2.0+: this patch cleans up important parts of the Web Console internals, making it less error prone, while fixing other bugs as well (594523 and 592469). Bug 594523 is also blocking2.0+.

Thanks!

Comment 6

[Mihai Sucan [:msucan]]

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

Asking for review from Shawn.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> - keeps the getHeadsUpDisplay(), getOutputNodeById() and displaysIndex()
> methods *only* for compatibility with older tests. changed them to use the new
> internals, such that they no longer pose technical issues.
> 
> If you want I can go ahead and remove these methods entirely, but that would
> cause more "patch noise": I'll have to update all code that uses these three
> methods.
Can you please do this in a part 2?  No point in maintaining code that is only used by tests if we do not need to.

> Finally, I'd add that this patch now makes the HUDService entirely reliant on
> the new nsIScriptError2, and on the outerWindowID of such messages. Things are
> much cleaner because of this, but it also means we must first get bugs 603706,
> 603750 and 606498 ready and checked-in first. Otherwise, we'll loose
> error/warning messages.
Do we have an ETA on when those are going to land?  You mentioned to me that this bug blocked other dev-tools work, but it seems like it's still blocked on those three bugs.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to comment #7)
> (In reply to comment #2)
> > - keeps the getHeadsUpDisplay(), getOutputNodeById() and displaysIndex()
> > methods *only* for compatibility with older tests. changed them to use the new
> > internals, such that they no longer pose technical issues.
> > 
> > If you want I can go ahead and remove these methods entirely, but that would
> > cause more "patch noise": I'll have to update all code that uses these three
> > methods.
> Can you please do this in a part 2?  No point in maintaining code that is only
> used by tests if we do not need to.

Sure, I can.

> > Finally, I'd add that this patch now makes the HUDService entirely reliant on
> > the new nsIScriptError2, and on the outerWindowID of such messages. Things are
> > much cleaner because of this, but it also means we must first get bugs 603706,
> > 603750 and 606498 ready and checked-in first. Otherwise, we'll loose
> > error/warning messages.
> Do we have an ETA on when those are going to land?  You mentioned to me that
> this bug blocked other dev-tools work, but it seems like it's still blocked on
> those three bugs.

An ETA strongly depends on reviews bandwidth. Two of the three bugs are ready to land. The third has four patches, one has r+, and the others are very close to r+. I was given an estimate that this week I'll get the reviews.

Comment 9

[Mihai Sucan [:msucan]]

Attachment: Part 2: cleanup

As requested by Shawn, here's the patch that does further code cleanup. I removed the following methods: getHeadsUpDisplay(), getOutputNodeById(), displaysIndex(), getConsoleOutputNode(), getChromeWindowFromContentWindow() and getContentWindowFromHUDId().

Comment 10

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

Comment on attachment 496609 [details] [diff] [review]
Part 2: cleanup

Since we're doing cleanup anyway, can we fix all the methods that have overloaded argument types? E.g., clearDisplay(aHUD) where aHUD can either be a string (id) or a dom node. I would much rather see separate methods like clearDisplayForNode or similar. Maybe that's an over-refactoring and it will make this patch more complicated, but I think it'll make dealing with these methods more explicit and get rid of a lot of repeated  | if (typeof aHUD... | tests around the the file.

Same goes for register/unregisterDisplay and I'm sure others.

-    var outputNode = aHUD.querySelector(".hud-output-node");
+    var outputNode = typeof aHUD == "string" ?
+                     this.hudReferences[aHUD].outputNode :
+                     aHUD.querySelector(".hud-output-node");

use let instead of var. (I know, we use var in a bunch of places, but we shouldn't be and I'd like to replace them)

+    let hudBox = outputNode;
+    while (!hudBox.classList.contains("hud-box")) {
+      hudBox = hudBox.parentNode;
+    }
+
+    hudBox.lastTimestamp = 0;

This seems like it should be a separate method. Maybe resetTimestamp(outputNode) ?

Other than that, not much to say. I skimmed the modified tests and the changes look reasonable.

Lots fewer lines!

(oh, and presumably these patches will have to be rebased)

Comment 11

[Shawn Wilsher :sdwilsh]

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

>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>+  registerDisplay: function HS_registerDisplay(aHUDId)
>   {
>+    // register a display DOM node Id with the service
nit: use complete sentences with punctuation please

>   observe: function HCO_observe(aSubject, aTopic, aData)
>   {
>     if (aTopic == "xpcom-shutdown") {
>       this.uninit();
>       return;
>     }
> 
>+    if (!(aSubject instanceof Ci.nsIScriptError) ||
>+        !(aSubject instanceof Ci.nsIScriptError2) ||
>+        !aSubject.outerWindowID) {
Are we ever going to get something that is an nsIScriptError now?  I think we should log an error to the error console if aSubject is not an nsIScriptError2 object at this point so we can track down any cases that come up.

r=sdwilsh

Comment 12

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

(In reply to comment #11)
> nit: use complete sentences with punctuation please

Nit: use complete sentences with punctuation please.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> Nit: use complete sentences with punctuation please.
I'm glad you noticed that I have a higher grammar standard for comments in code than I do for review comments. :D

Comment 14

[Mihai Sucan [:msucan]]

(In reply to comment #11)
> Comment on attachment 493248 [details] [diff] [review]
> proposed patch
> 
> >+++ b/toolkit/components/console/hudservice/HUDService.jsm
> >+  registerDisplay: function HS_registerDisplay(aHUDId)
> >   {
> >+    // register a display DOM node Id with the service
> nit: use complete sentences with punctuation please

Will update the patch. :)

> >   observe: function HCO_observe(aSubject, aTopic, aData)
> >   {
> >     if (aTopic == "xpcom-shutdown") {
> >       this.uninit();
> >       return;
> >     }
> > 
> >+    if (!(aSubject instanceof Ci.nsIScriptError) ||
> >+        !(aSubject instanceof Ci.nsIScriptError2) ||
> >+        !aSubject.outerWindowID) {
> Are we ever going to get something that is an nsIScriptError now?  I think we
> should log an error to the error console if aSubject is not an nsIScriptError2
> object at this point so we can track down any cases that come up.

You have a point, but I believe this would spam the error console too much. Whenever an nsIScriptError is shown, we'd also issue a warning about it. Not nice.

We still have cases when we receive nsIScriptErrors (not 2s), when those cases are chrome-related, or when they are "not-so-interesting" messages. What I did in the patches that landed is an exhaustive search for cases we are interested into - web page content errors. I did include only a few cases related to chrome code. We also still have nsIConsoleMessages, which are not all converted to nsIScriptError2.

At the moment, I believe we should leave that as is. Or is your suggestion a required change for this patch? I can do it, if you want.

Thanks for your review+!

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> At the moment, I believe we should leave that as is. Or is your suggestion a
> required change for this patch? I can do it, if you want.
Naw, I'm not sure it is worth it at this point.

Comment 16

[Mihai Sucan [:msucan]]

Attachment: [ready-for-checkin] Part 1: drop window registry and friends

Updated the patch to fix the comment. No rebase needed, still applies cleanup.

This is ready for checkin.

Comment 17

[Mihai Sucan [:msucan]]

Attachment: Part 2: more cleanup

Addressed the feedback comments from Robert. Thanks for the feedback+!

Please note that the solution for register/clearDisplay() was a simple fix, not many lines of patches to clean that up.

Also, the problem with reset timestamp is "wider": jsterm.clearOutput() is a duplicate of HUDService.clearDisplay(). I aliased HUDService.clearDisplay() to clearOutput().

Comment 18

[Mihai Sucan [:msucan]]

Comment on attachment 499014 [details] [diff] [review]
Part 2: more cleanup

Canceling review request. We are going to move this code into a separate bug.

Comment 19

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

Attachment: mochitest-browser.chrome.log

with above patch rebased to apply ontop of styling code patch.

(attaching rebased patch after this)

Comment 20

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

Attachment: rebased Part 1

Comment 21

[Mihai Sucan [:msucan]]

Attachment: [checked-in] rebased patch

Fixed the patch rebase. No test failures here.

Comment 22

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

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

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