Closed Bug 592469 Opened 10 years ago Closed 9 years ago

Web Console cleanup: Kill getHeadsUpDisplay() and other methods

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 8

People

(Reporter: pcwalton, Assigned: msucan)

References

Details

(Whiteboard: [has-patch][post-fx4] [console-1][qa-])

Attachments

(11 files, 14 obsolete files)

3.80 KB, patch
dcamp
: review+
Gavin
: review+
Details | Diff | Splinter Review
4.67 KB, patch
dcamp
: review+
Gavin
: review+
Details | Diff | Splinter Review
1.09 KB, patch
dcamp
: review+
Gavin
: review+
Details | Diff | Splinter Review
2.05 KB, patch
dcamp
: review+
Gavin
: review+
Details | Diff | Splinter Review
6.69 KB, patch
dcamp
: review+
Gavin
: review+
Details | Diff | Splinter Review
27.86 KB, patch
dcamp
: review+
Details | Diff | Splinter Review
8.64 KB, patch
dcamp
: review+
Details | Diff | Splinter Review
12.70 KB, patch
dcamp
: review+
Details | Diff | Splinter Review
30.53 KB, patch
Details | Diff | Splinter Review
6.82 KB, patch
dcamp
: review+
Details | Diff | Splinter Review
7.97 KB, patch
Details | Diff | Splinter Review
getHeadsUpDisplay() (which actually returns the output node, so should be renamed) should be a member of the HeadsUpDisplay object, not the HUD Service object.
Correction: This function doesn't return the output node. Rather it returns the HUD box.
Summary: Rename getHeadsUpDisplay() and move it to the HeadsUpDisplay object in the Web Console → Web Console cleanup: Rename getHeadsUpDisplay() and move it to the HeadsUpDisplay object in the Web Console
The plan is now to kill getHeadsUpDisplay() entirely.
Summary: Web Console cleanup: Rename getHeadsUpDisplay() and move it to the HeadsUpDisplay object in the Web Console → Web Console cleanup: Kill getHeadsUpDisplay()
I know that there are other bugs that change how the console object is accessed. What work is represented by this bug? Is it just to remove the code or is there still other refactoring that will be required?
Blocks: devtools4
(In reply to comment #3)
> I know that there are other bugs that change how the console object is
> accessed. What work is represented by this bug? Is it just to remove the code
> or is there still other refactoring that will be required?

The changes tie in heavily with bug 580618. Essentially, we want to replace all instances of getHeadsUpDisplay() with accesses to the hudReferences array introduced by Mihai's patch to that one. The change should be basically mechanical.
Depends on: 580618
Assignee: nobody → mihai.sucan
mass change: filter on PRIORITYSETTING
Priority: -- → P3
Attachment #499067 - Flags: review?(gavin.sharp)
Attachment #499068 - Flags: review?(gavin.sharp)
Attachment #499069 - Flags: review?(gavin.sharp)
Attachment #499071 - Flags: review?(gavin.sharp)
Attachment #499072 - Flags: review?(gavin.sharp)
This patch also removes the unused HUDService.getContentWindowFromHUDId() method. The new HUDService.getHudByWindow() is used by tests - it's the most common "operation" we perform in tests (get the HUD object from the window object).
Attachment #499074 - Flags: review?(gavin.sharp)
Attached patch Part 7: tests - displaysIndex() (obsolete) — Splinter Review
Remove the use of displaysIndex() in the HUDService tests.
Attachment #499075 - Flags: review?(gavin.sharp)
Remove the use of getHeadsUpDisplay() in the HUDService tests.
Attachment #499076 - Flags: review?(gavin.sharp)
Remove the use of getOutputNodeById() in the HUDService tests.
Attachment #499077 - Flags: review?(gavin.sharp)
Attached patch Part 10: tests - cleanup (obsolete) — Splinter Review
Remove tests that are no longer relevant.
Attachment #499079 - Flags: review?(gavin.sharp)
Status: NEW → ASSIGNED
Depends on: 611789
Whiteboard: [has-patch][needs-review][post-fx4]
Whiteboard: [has-patch][needs-review][post-fx4] → [has-patch][needs-review][post-fx4] [console-1]
Duplicate of this bug: 603176
Blocks: 578462
Are these all up to date? We should probably try to get these landed soon.
There have been minimal changes since December, to the Web Console code.

I think you can review them as-is. I'll update the patches with review comments and rebase. Some of the parts do not apply cleanly due to minor changes through out the Web Console code, but nothing serious.

Alternatively, I can rebase the patches before review. How do you want us to proceed?

Please also note that we have 3 patches in queue to land (marked as [checkin]). After those land we'll need to once again rebase these patches.

Thanks!
Attachment #499067 - Attachment is obsolete: true
Attachment #499068 - Attachment is obsolete: true
Attachment #499069 - Attachment is obsolete: true
Attachment #499071 - Attachment is obsolete: true
Attachment #499072 - Attachment is obsolete: true
Attachment #499074 - Attachment is obsolete: true
Attachment #499075 - Attachment is obsolete: true
Attachment #499076 - Attachment is obsolete: true
Attachment #499077 - Attachment is obsolete: true
Attachment #499079 - Attachment is obsolete: true
Attachment #499067 - Flags: review?(gavin.sharp)
Attachment #499068 - Flags: review?(gavin.sharp)
Attachment #499069 - Flags: review?(gavin.sharp)
Attachment #499071 - Flags: review?(gavin.sharp)
Attachment #499072 - Flags: review?(gavin.sharp)
Attachment #499074 - Flags: review?(gavin.sharp)
Attachment #499075 - Flags: review?(gavin.sharp)
Attachment #499076 - Flags: review?(gavin.sharp)
Attachment #499077 - Flags: review?(gavin.sharp)
Attachment #499079 - Flags: review?(gavin.sharp)
Attachment #540784 - Attachment is obsolete: true
I have completed a manual rebase for all the 10 patches. There has been breakage for each of them, but fixes were not too hard.

All tests pass locally. Will push to the try server.

Cleanup prioritized to help with incoming work on Web Console remoting support (e10s).

Looking forward for the review comments. Thank you!

(please note that the cleanup being done by these patches is pretty much agreed upon by Gavin Sharp and Shawn Wilsher, see the comments in bug 611789)
Attachment #543494 - Flags: review?(dcamp)
Attachment #543498 - Flags: review?(dcamp)
Attachment #543501 - Flags: review?(dcamp)
Attachment #543504 - Flags: review?(dcamp)
Summary: Web Console cleanup: Kill getHeadsUpDisplay() → Web Console cleanup: Kill getHeadsUpDisplay() and other methods
Version: unspecified → Trunk
Attachment #543494 - Flags: review?(dcamp) → review+
Attachment #543495 - Flags: review?(dcamp) → review+
Attachment #543496 - Flags: review?(dcamp) → review+
Attachment #543497 - Flags: review?(dcamp) → review+
Comment on attachment 543499 [details] [diff] [review]
[in-fx-team] Part 6: HUDService - remove displaysIndex() and more

review note, getHudByWindow isn't used in this patch, but is used in later (test) patches
Attachment #543499 - Flags: review?(dcamp) → review+
Attachment #543500 - Flags: review?(dcamp) → review+
Attachment #543501 - Flags: review?(dcamp) → review+
Attachment #543502 - Flags: review?(dcamp) → review+
Attachment #543504 - Flags: review?(dcamp) → review+
Attachment #543498 - Flags: review?(dcamp) → review+
Comment on attachment 543494 [details] [diff] [review]
[in-fx-team] Part 1: HUDService - remove getHeadsUpDisplay()

Thank you Dave for the r+!

Gavin: Updated the patches! ;) We have them reviewed within the team. We'd very much appreciate a quick review for this batch of patches. Thank you!
Attachment #543494 - Flags: review?(gavin.sharp)
Attachment #543495 - Flags: review?(gavin.sharp)
Attachment #543496 - Flags: review?(gavin.sharp)
Attachment #543497 - Flags: review?(gavin.sharp)
Attachment #543498 - Flags: review?(gavin.sharp)
Attachment #543499 - Flags: review?(gavin.sharp)
Attachment #543500 - Flags: review?(gavin.sharp)
Attachment #543501 - Flags: review?(gavin.sharp)
Attachment #543502 - Flags: review?(gavin.sharp)
Attachment #543504 - Flags: review?(gavin.sharp)
Attachment #543494 - Flags: review?(gavin.sharp) → review+
Attachment #543495 - Flags: review?(gavin.sharp) → review+
Attachment #543496 - Flags: review?(gavin.sharp) → review+
Attachment #543497 - Flags: review?(gavin.sharp) → review+
Attachment #543499 - Flags: review?(gavin.sharp) → review+
Comment on attachment 543498 [details] [diff] [review]
Part 5: HUDService - cleanup clear/unregisterDisplay()

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

>   clearDisplay: function HS_clearDisplay(aHUD)

>-    if (hudRef) {
>-      hudRef.cssNodes = {};
>-    }
>-
>-    var outputNode = aHUD.querySelector(".hud-output-node");
>-
>-    while (outputNode.firstChild) {
>-      outputNode.removeChild(outputNode.firstChild);
>-    }
>-
>-    aHUD.lastTimestamp = 0;

I realize this was duplicating stuff that jsterm.clearOutput was doing, but it seems odd for this stuff to be done there rather than here. Can it move the other way instead? Seems like it's not particularly related to the JSTerm object.

>   unregisterDisplay: function HS_unregisterDisplay(aHUD)

>-    // remove the HeadsUpDisplay object from memory
>-    if ("cssNodes" in this.hudReferences[id]) {
>-      delete this.hudReferences[id].cssNodes;
>-    }

This is unnecessary because cssNodes is reset in clearOutput/clearDisplay I guess? OK.

>-    ownerDoc = outputNode.ownerDocument;
>-    ownerDoc.getElementById(id).parentNode.removeChild(outputNode);

>+    if (hud.consolePanel) {
>+      hud.consolePanel.parentNode.removeChild(hud.consolePanel);
>+    }
>+    else {
>+      hud.parentNode.removeChild(hud.HUDBox);
>+    }

I don't really understand how these are equivalent. In particular, I don't know what the consolePanel change is about. Why does it affect whether the HUDBox get removed?
(In reply to comment #33)
> Comment on attachment 543498 [details] [diff] [review] [review]
> Part 5: HUDService - cleanup clear/unregisterDisplay()
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >   clearDisplay: function HS_clearDisplay(aHUD)
> 
> >-    if (hudRef) {
> >-      hudRef.cssNodes = {};
> >-    }
> >-
> >-    var outputNode = aHUD.querySelector(".hud-output-node");
> >-
> >-    while (outputNode.firstChild) {
> >-      outputNode.removeChild(outputNode.firstChild);
> >-    }
> >-
> >-    aHUD.lastTimestamp = 0;
> 
> I realize this was duplicating stuff that jsterm.clearOutput was doing, but
> it seems odd for this stuff to be done there rather than here. Can it move
> the other way instead? Seems like it's not particularly related to the
> JSTerm object.

The Web Console output node is "worked on" by JSTerm mostly. We have a lot of stuff in JSTerm.

If you want I can move the code in the other method. Do you want this?

I can even remove jsterm.clearOutput() entirely, so we don't cause so much confusion. Shall I do so?


> >   unregisterDisplay: function HS_unregisterDisplay(aHUD)
> 
> >-    // remove the HeadsUpDisplay object from memory
> >-    if ("cssNodes" in this.hudReferences[id]) {
> >-      delete this.hudReferences[id].cssNodes;
> >-    }
> 
> This is unnecessary because cssNodes is reset in clearOutput/clearDisplay I
> guess? OK.

Yep, and we delete this.hudReferences[id] entirely. No point in deleting some of the properties individually.


> >-    ownerDoc = outputNode.ownerDocument;
> >-    ownerDoc.getElementById(id).parentNode.removeChild(outputNode);
> 
> >+    if (hud.consolePanel) {
> >+      hud.consolePanel.parentNode.removeChild(hud.consolePanel);
> >+    }
> >+    else {
> >+      hud.parentNode.removeChild(hud.HUDBox);
> >+    }
> 
> I don't really understand how these are equivalent. In particular, I don't
> know what the consolePanel change is about. Why does it affect whether the
> HUDBox get removed?

The hud.HUDBox lives in the hud.consolePanel when the Web Console is positioned as a window (as a xul:panel). That's why the difference.

Hmm, I can update this to:

> >+    hud.parentNode.removeChild(hud.HUDBox);
> >+    if (hud.consolePanel) {
> >+      hud.consolePanel.parentNode.removeChild(hud.consolePanel);
> >+    }

... to make it less confusing. Would this be fine?
Comment on attachment 543500 [details] [diff] [review]
[in-fx-team] Part 7: tests - displaysIndex()

I don't think I really need to review all of these test changes.
Attachment #543500 - Flags: review?(gavin.sharp)
Attachment #543501 - Flags: review?(gavin.sharp)
Attachment #543502 - Flags: review?(gavin.sharp)
Attachment #543504 - Flags: review?(gavin.sharp)
(In reply to comment #34)
> The Web Console output node is "worked on" by JSTerm mostly. We have a lot
> of stuff in JSTerm.

Hmm, OK, I guess I had a different expectation, but that may not reflect reality.

> If you want I can move the code in the other method. Do you want this?
> 
> I can even remove jsterm.clearOutput() entirely, so we don't cause so much
> confusion. Shall I do so?

I'm tempted to say yes, but I think you probably know what makes most sense, so go with what you think is best.

> Hmm, I can update this to:
...
> ... to make it less confusing. Would this be fine?

Yeah that sounds fine.
Updated the patch.

Decided to keep jsterm.clearOutput() and removed HUDService.clearDisplay(). At the moment this what seems to make sense. Ultimately, HUDService should hold no methods specific to a HUD object.

Please let me know if the patch is fine, because I'll have to rebase the console storage patches on top of this cleanup ASAP. Thank you!
Attachment #543498 - Attachment is obsolete: true
Attachment #544437 - Flags: review?(gavin.sharp)
Attachment #543498 - Flags: review?(gavin.sharp)
Rebased the patch.
Attachment #543501 - Attachment is obsolete: true
New patch to clean the tests - to update the calls for clearDisplay().
Attachment #544439 - Flags: review?(dcamp)
Attachment #544439 - Flags: review?(dcamp) → review+
Comment on attachment 544437 [details] [diff] [review]
Part 5: HUDService - cleanup clear/unregisterDisplay()

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

>+   * @param string aHUD
>+   *        The ID of a HUD.
>    * @returns void
>    */
>   unregisterDisplay: function HS_unregisterDisplay(aHUD)

Can you make this aHUDId? Slightly confusing to have an "aHUD" that refers to an ID rather than a hud object.

>   clearOutput: function JST_clearOutput()

>+    let hud = HUDService.getHudReferenceById(this.hudId);

Seems like we should perhaps consider having jsterm objects should hold references to the hud directly at some point...
Attachment #544437 - Flags: review?(gavin.sharp) → review+
Comment on attachment 544437 [details] [diff] [review]
Part 5: HUDService - cleanup clear/unregisterDisplay()

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

>   unregisterDisplay: function HS_unregisterDisplay(aHUD)

>-    ownerDoc = outputNode.ownerDocument;
>-    ownerDoc.getElementById(id).parentNode.removeChild(outputNode);

>+    hud.HUDBox.parentNode.removeChild(hud.HUDBox);
>+    if (hud.consolePanel) {
>+      hud.consolePanel.parentNode.removeChild(hud.consolePanel);
>+    }

One more question about these differences - is it a bug fix that we now remove the entire HUDBox/consolePanel when we used to only remove the outputNode? Were we "leaking" elements when you toggled the web console on and off repeatedly?
(In reply to comment #41)
> Comment on attachment 544437 [details] [diff] [review] [review]
> Part 5: HUDService - cleanup clear/unregisterDisplay()
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >   unregisterDisplay: function HS_unregisterDisplay(aHUD)
> 
> >-    ownerDoc = outputNode.ownerDocument;
> >-    ownerDoc.getElementById(id).parentNode.removeChild(outputNode);
> 
> >+    hud.HUDBox.parentNode.removeChild(hud.HUDBox);
> >+    if (hud.consolePanel) {
> >+      hud.consolePanel.parentNode.removeChild(hud.consolePanel);
> >+    }
> 
> One more question about these differences - is it a bug fix that we now
> remove the entire HUDBox/consolePanel when we used to only remove the
> outputNode? Were we "leaking" elements when you toggled the web console on
> and off repeatedly?

No, it's not a bug fix. It's only a cleanup to make the code less confusing than it was.
(In reply to comment #40)
> Comment on attachment 544437 [details] [diff] [review] [review]
> Part 5: HUDService - cleanup clear/unregisterDisplay()
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >+   * @param string aHUD
> >+   *        The ID of a HUD.
> >    * @returns void
> >    */
> >   unregisterDisplay: function HS_unregisterDisplay(aHUD)
> 
> Can you make this aHUDId? Slightly confusing to have an "aHUD" that refers
> to an ID rather than a hud object.

Sure, I'll update the patch accordingly. And I'll look into making this method even less confusing.

Thank you for the r+!
Comments addressed.
Attachment #544437 - Attachment is obsolete: true
Whiteboard: [has-patch][needs-review][post-fx4] [console-1] → [has-patch][post-fx4] [console-1][land-in-fx-team]
Comment on attachment 543494 [details] [diff] [review]
[in-fx-team] Part 1: HUDService - remove getHeadsUpDisplay()

http://hg.mozilla.org/integration/fx-team/rev/978c21f5c68f
Attachment #543494 - Attachment description: Part 1: HUDService - remove getHeadsUpDisplay() → [in-fx-team] Part 1: HUDService - remove getHeadsUpDisplay()
Comment on attachment 543495 [details] [diff] [review]
[in-fx-team] Part 2: HUDService - remove getOutputNodeById()

http://hg.mozilla.org/integration/fx-team/rev/2f5e953d2fbe
Attachment #543495 - Attachment description: Part 2: HUDService - remove getOutputNodeById() → [in-fx-team] Part 2: HUDService - remove getOutputNodeById()
Comment on attachment 543496 [details] [diff] [review]
[in-fx-team] Part 3: HUDService - remove getConsoleOutputNode()

http://hg.mozilla.org/integration/fx-team/rev/e5bda2ebf1e4
Attachment #543496 - Attachment description: Part 3: HUDService - remove getConsoleOutputNode() → [in-fx-team] Part 3: HUDService - remove getConsoleOutputNode()
Comment on attachment 543497 [details] [diff] [review]
[in-fx-team] Part 4: HUDService - remove getChromeWindowForContentWindow()

http://hg.mozilla.org/integration/fx-team/rev/152cb06533a6
Attachment #543497 - Attachment description: Part 4: HUDService - remove getChromeWindowForContentWindow() → [in-fx-team] Part 4: HUDService - remove getChromeWindowForContentWindow()
Comment on attachment 544495 [details] [diff] [review]
[in-fx-team] Part 5: HUDService - cleanup clear/unregisterDisplay()

http://hg.mozilla.org/integration/fx-team/rev/11286078b11a
Attachment #544495 - Attachment description: Part 5: HUDService - cleanup clear/unregisterDisplay() → [in-fx-team] Part 5: HUDService - cleanup clear/unregisterDisplay()
Comment on attachment 543499 [details] [diff] [review]
[in-fx-team] Part 6: HUDService - remove displaysIndex() and more

http://hg.mozilla.org/integration/fx-team/rev/a3657ba60787
Attachment #543499 - Attachment description: Part 6: HUDService - remove displaysIndex() and more → [in-fx-team] Part 6: HUDService - remove displaysIndex() and more
Comment on attachment 543500 [details] [diff] [review]
[in-fx-team] Part 7: tests - displaysIndex()

http://hg.mozilla.org/integration/fx-team/rev/1f098ada09bb
Attachment #543500 - Attachment description: Part 7: tests - displaysIndex() → [in-fx-team] Part 7: tests - displaysIndex()
Comment on attachment 544438 [details] [diff] [review]
[in-fx-team] Part 8: tests - getHeadsUpDisplay()

http://hg.mozilla.org/integration/fx-team/rev/94b065b8ec4e
Attachment #544438 - Attachment description: Part 8: tests - getHeadsUpDisplay() → [in-fx-team] Part 8: tests - getHeadsUpDisplay()
Comment on attachment 543502 [details] [diff] [review]
[in-fx-team] Part 9: tests - getOutputNodeById()

http://hg.mozilla.org/integration/fx-team/rev/abcf5e88d5a1
Attachment #543502 - Attachment description: Part 9: tests - getOutputNodeById() → [in-fx-team] Part 9: tests - getOutputNodeById()
Comment on attachment 543504 [details] [diff] [review]
[in-fx-team] Part 10: tests - cleanup

http://hg.mozilla.org/integration/fx-team/rev/1aaee943f66b
Attachment #543504 - Attachment description: Part 10: tests - cleanup → [in-fx-team] Part 10: tests - cleanup
Comment on attachment 544439 [details] [diff] [review]
[in-fx-team] Part 11: tests - clearDisplay()

http://hg.mozilla.org/integration/fx-team/rev/59ae546b1da7
Attachment #544439 - Attachment description: Part 11: tests - clearDisplay() → [in-fx-team] Part 11: tests - clearDisplay()
Whiteboard: [has-patch][post-fx4] [console-1][land-in-fx-team] → [has-patch][post-fx4] [console-1][fixed-in-fx-team]
These changes made us leak data:text/html,Web%20Console%20test%20for%20bug%20626484 during mochitest-browser-chrome (bug 658738). Any idea what's going on and what a quick fix could be? I'm eager to back this out otherwise, as new leaks are creeping in faster than we're fixing lingering ones in bug 658738.
I appreciate the desire to make progress on bug 658738, but I don't think backing this out is the right tradeoff. We should aggressively attempt to track down the leak, though - Mihai, is that something you can look into (in a followup bug)?
A followup bug is fine as long as someone's working on it.
I will look into this.

Does this patch cause constant memleaks? When I wrote these patches I sent them to try server, had no memleaks.
It's constant, and these kind of leaks currently go unnoticed. See bug 658738 for details.
(In reply to comment #61)
> It's constant, and these kind of leaks currently go unnoticed. See bug
> 658738 for details.

Ah, that's unfortunate. I hope this kind of leaks will start to be tracked by the automated test systems as soon as possible.

I'll look into what's causing the leaks. Shouldn't be too hard to fix this, once I have a reliable way to test it on my local system.

(I hope to have a patch over the weekend.)
I just did a local "backout" of these patches, to see if I get memory leaks when I revert these changes.

I still do get this kind of memleaks, minor differences. So, these patches do not have a direct impact.
Can you be more specific about what you're seeing? If data:text/html,Web%20Console%20test%20for%20bug%20626484 is part of the minor difference, then that's exactly what I'm talking about. I spent an hour bisecting this. It first showed up on this bug's fx-team landing and then on the fx-team -> mozilla-central merge.
(In reply to comment #64)
> Can you be more specific about what you're seeing? If
> data:text/html,Web%20Console%20test%20for%20bug%20626484 is part of the
> minor difference, then that's exactly what I'm talking about. I spent an
> hour bisecting this. It first showed up on this bug's fx-team landing and
> then on the fx-team -> mozilla-central merge.

Here's what I have:

I ran all the HUDService tests on my system from fxteam r72737, no changes:

http://pastebin.mozilla.org/1274945

I see a bunch DOM windows leaked.

If I go back to fxteam r72418, before the Web Console cleanup patches landed, I get the following DOM window leaks:

http://pastebin.mozilla.org/1274947

There were fewer, but we still had some of them.

If I only back-out the patch from the current fxteam revision 72737:

http://pastebin.mozilla.org/1274948

I still get a bad number of DOM windows leaked.

It looks to me like the Web Console cleanup patches have only affected the manifestation of the leak. Additionally, something outside of the Web Console has affected the number of windows being leaked, because if I backout these patches we still get a lot more memleaks than we had in r72418, so ... something else since that revision until now ... exposes more of these leaks. (afaik we had no other Web Console patches landed in fxteam since then.)

I've began work on hunting down the memory leak(s) we have in the code. Going to submit a patch when I managed to track them down.
(In reply to comment #65)
> (In reply to comment #64)
> > Can you be more specific about what you're seeing? If
> > data:text/html,Web%20Console%20test%20for%20bug%20626484 is part of the
> > minor difference, then that's exactly what I'm talking about. I spent an
> > hour bisecting this. It first showed up on this bug's fx-team landing and
> > then on the fx-team -> mozilla-central merge.
> 
> Here's what I have:
> 
> I ran all the HUDService tests on my system from fxteam r72737, no changes:
> 
> http://pastebin.mozilla.org/1274945
> 
> I see a bunch DOM windows leaked.
> 
> If I go back to fxteam r72418, before the Web Console cleanup patches
> landed, I get the following DOM window leaks:
> 
> http://pastebin.mozilla.org/1274947
> 
> There were fewer, but we still had some of them.
> 
> If I only back-out the patch from the current fxteam revision 72737:
> 
> http://pastebin.mozilla.org/1274948
> 
> I still get a bad number of DOM windows leaked.

Again, that's not what I'm talking about. I know the list is long and growing (hence me nagging in bugs!). What I'm asking you here is to focus on "data:text/html,Web%20Console%20test%20for%20bug%20626484", which entered the list with this check-in. Your sample logs actually support this.
(In reply to comment #66)
> Again, that's not what I'm talking about. I know the list is long and
> growing (hence me nagging in bugs!). What I'm asking you here is to focus on
> "data:text/html,Web%20Console%20test%20for%20bug%20626484", which entered
> the list with this check-in. Your sample logs actually support this.

Agreed, but that leak is not related to the test file which opens that data URI. Unfortunately, if I modify that test to *only* open a tab, open the Web Console, close the Web Console, then remove the tab, ... I still get the memory leak. That's very ugly. We need to find the root problem...
> Unfortunately, if I modify that test to *only* open a tab, open the Web
> Console, close the Web Console, then remove the tab, ... I still get the
> memory leak.

In an aggregated log or when running that test on its own?
With or without this bug's patches?
(In reply to comment #68)
> > Unfortunately, if I modify that test to *only* open a tab, open the Web
> > Console, close the Web Console, then remove the tab, ... I still get the
> > memory leak.
> 
> In an aggregated log or when running that test on its own?

When running that test on its own.

> With or without this bug's patches?

With these patches.

Looking through the code I did find some stuff we never removed, patching those... but still they don't fix *this* leak. Need to look into the code more.
Running the test on its own won't give you useful results without further modifications, as the window won't be destroyed synchronously. Try waiting ~15 seconds between removing the tab and finishing the test.
(In reply to comment #70)
> Running the test on its own won't give you useful results without further
> modifications, as the window won't be destroyed synchronously. Try waiting
> ~15 seconds between removing the tab and finishing the test.

Thanks for your suggestion!

Currently I am tracking down the memleak(s).

For the minimal test I was running I got it down to no memleak. I removed all the addEventListener() calls that have no paired removeEventListener(). It seems we need to properly remove all events, to all elements in the UI. Unfortunately, the code we have is not very nice about having balanced calls to add and remove event listeners.


(This is something that could explain why the code cleanup patches here have changed the memleak behavior, without actually adding/removing memleaks.)
For reference: I opened bug 672470 and submitted a patch which fixes memory leaks in the Web Console code.
Duplicate of this bug: 592519
Whiteboard: [has-patch][post-fx4] [console-1] → [has-patch][post-fx4] [console-1][qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.