Web Console cleanup: Remove the window registry

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
8 years ago
4 months ago

People

(Reporter: pcwalton, Assigned: msucan)

Tracking

Trunk
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [patchclean:1223])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

8 years ago
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.

Updated

8 years ago
Blocks: 593956

Updated

8 years ago
Assignee: nobody → mihai.sucan

Comment 1

8 years ago
mass change: filter on PRIORITYSETTING
Priority: -- → P3
(Assignee)

Comment 2

8 years ago
Created attachment 493248 [details] [diff] [review]
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!
Attachment #493248 - Flags: feedback?
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
Whiteboard: [patchclean:1125]
Version: unspecified → Trunk
(Assignee)

Updated

8 years ago
Depends on: 603750
(Assignee)

Updated

8 years ago
Attachment #493248 - Flags: feedback? → feedback?(rcampbell)
Comment on attachment 493248 [details] [diff] [review]
proposed patch

this looks nice, mihai.
Attachment #493248 - Flags: feedback?(rcampbell) → feedback+
(Assignee)

Comment 4

8 years ago
Comment on attachment 493248 [details] [diff] [review]
proposed patch

Thanks for your quick feedback+ Robert!

Asking for review from Gavin.
Attachment #493248 - Flags: review?(gavin.sharp)
(Assignee)

Updated

8 years ago
Blocks: 594523
(Assignee)

Comment 5

8 years ago
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!
blocking2.0: --- → ?
blocking2.0: ? → betaN+

Updated

8 years ago
Severity: normal → blocker
Priority: P3 → P1
(Assignee)

Updated

8 years ago
Depends on: 603706, 606498
(Assignee)

Comment 6

8 years ago
Comment on attachment 493248 [details] [diff] [review]
proposed patch

Asking for review from Shawn.
Attachment #493248 - Flags: review?(gavin.sharp) → review?(sdwilsh)

Updated

8 years ago
Severity: blocker → normal
(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.
(Assignee)

Comment 8

8 years ago
(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.
(Assignee)

Comment 9

8 years ago
Created attachment 496609 [details] [diff] [review]
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().
Attachment #496609 - Flags: feedback?(rcampbell)
(Assignee)

Updated

8 years ago
Whiteboard: [patchclean:1125] → [patchclean:1209]
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)
Attachment #496609 - Flags: feedback?(rcampbell) → feedback+
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
Attachment #493248 - Flags: review?(sdwilsh) → review+
(In reply to comment #11)
> nit: use complete sentences with punctuation please

Nit: use complete sentences with punctuation please.
(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
(Assignee)

Comment 14

8 years ago
(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+!
(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.
(Assignee)

Comment 16

8 years ago
Created attachment 499012 [details] [diff] [review]
[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.
Attachment #493248 - Attachment is obsolete: true
(Assignee)

Comment 17

8 years ago
Created attachment 499014 [details] [diff] [review]
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().
Attachment #496609 - Attachment is obsolete: true
Attachment #499014 - Flags: review?(gavin.sharp)
(Assignee)

Updated

8 years ago
Whiteboard: [patchclean:1209] → [patchclean:1221]
(Assignee)

Comment 18

8 years ago
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.
Attachment #499014 - Attachment is obsolete: true
Attachment #499014 - Flags: review?(gavin.sharp)
(Assignee)

Updated

8 years ago
Whiteboard: [patchclean:1221] → [patchclean:1221][checkin]
(Assignee)

Updated

8 years ago
Blocks: 592469
Created attachment 499521 [details]
mochitest-browser.chrome.log

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

(attaching rebased patch after this)
(Assignee)

Comment 21

8 years ago
Created attachment 499529 [details] [diff] [review]
[checked-in] rebased patch

Fixed the patch rebase. No test failures here.
Attachment #499012 - Attachment is obsolete: true
Attachment #499521 - Attachment is obsolete: true
Attachment #499523 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Whiteboard: [patchclean:1221][checkin] → [patchclean:1223][checkin]
Comment on attachment 499529 [details] [diff] [review]
[checked-in] rebased patch

http://hg.mozilla.org/mozilla-central/rev/babc86ced878
Attachment #499529 - Attachment description: rebased patch → [checked-in] rebased patch
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:1223][checkin] → [patchclean:1223]
(Assignee)

Updated

8 years ago
Duplicate of this bug: 594523

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.