592469 - Web Console cleanup: Kill getHeadsUpDisplay() and other methods

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Patrick Walton (:pcwalton)]

getHeadsUpDisplay() (which actually returns the output node, so should be renamed) should be a member of the HeadsUpDisplay object, not the HUD Service object.

Comment 1

[Patrick Walton (:pcwalton)]

Correction: This function doesn't return the output node. Rather it returns the HUD box.

Comment 2

[Patrick Walton (:pcwalton)]

The plan is now to kill getHeadsUpDisplay() entirely.

Comment 3

[Kevin Dangoor]

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?

Comment 4

[Patrick Walton (:pcwalton)]

(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.

Comment 5

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 6

[Mihai Sucan [:msucan]]

Attachment: Part 1: HUDService - remove getHeadsUpDisplay()

Comment 7

[Mihai Sucan [:msucan]]

Attachment: Part 2: HUDService - remove getOutputNodeById()

Comment 8

[Mihai Sucan [:msucan]]

Attachment: Part 3: HUDService - remove getConsoleOutputNode()

Comment 9

[Mihai Sucan [:msucan]]

Attachment: Part 4: HUDService - remove getChromeWindowForContentWindow()

Comment 10

[Mihai Sucan [:msucan]]

Attachment: Part 5: HUDService - cleanup clear/unregisterDisplay()

Comment 11

[Mihai Sucan [:msucan]]

Attachment: Part 6: HUDService - remove displaysIndex()

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).

Comment 12

[Mihai Sucan [:msucan]]

Attachment: Part 7: tests - displaysIndex()

Remove the use of displaysIndex() in the HUDService tests.

Comment 13

[Mihai Sucan [:msucan]]

Attachment: Part 8: tests - getHeadsUpDisplay()

Remove the use of getHeadsUpDisplay() in the HUDService tests.

Comment 14

[Mihai Sucan [:msucan]]

Attachment: Part 9: tests - getOutputNodeById()

Remove the use of getOutputNodeById() in the HUDService tests.

Comment 15

[Mihai Sucan [:msucan]]

Attachment: Part 10: tests - cleanup

Remove tests that are no longer relevant.

Comment 17

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

Are these all up to date? We should probably try to get these landed soon.

Comment 18

[Mihai Sucan [:msucan]]

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!

Comment 19

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

Attachment: dummy attachment so that I can obsolete many patches at once

Comment 20

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] Part 1: HUDService - remove getHeadsUpDisplay()

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)

Comment 21

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] Part 2: HUDService - remove getOutputNodeById()

Comment 22

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] Part 3: HUDService - remove getConsoleOutputNode()

Comment 23

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] Part 4: HUDService - remove getChromeWindowForContentWindow()

Comment 24

[Mihai Sucan [:msucan]]

Attachment: Part 5: HUDService - cleanup clear/unregisterDisplay()

Comment 25

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] Part 6: HUDService - remove displaysIndex() and more

Comment 26

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] Part 7: tests - displaysIndex()

Comment 27

[Mihai Sucan [:msucan]]

Attachment: Part 8: tests - getHeadsUpDisplay()

Comment 28

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] Part 9: tests - getOutputNodeById()

Comment 29

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] Part 10: tests - cleanup

Comment 30

[Dave Camp (:dcamp)]

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

Comment 31

[Mihai Sucan [:msucan]]

Try server results are green:

http://tbpl.mozilla.org/?tree=Try&rev=68267edfdb12

Builds and logs:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-68267edfdb12

Comment 32

[Mihai Sucan [:msucan]]

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!

Comment 33

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

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?

Comment 34

[Mihai Sucan [:msucan]]

(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 35

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

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.

Comment 36

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

(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.

Comment 37

[Mihai Sucan [:msucan]]

Attachment: Part 5: HUDService - cleanup clear/unregisterDisplay()

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!

Comment 38

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] Part 8: tests - getHeadsUpDisplay()

Rebased the patch.

Comment 39

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] Part 11: tests - clearDisplay()

New patch to clean the tests - to update the calls for clearDisplay().

Comment 40

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

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...

Comment 41

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

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?

Comment 42

[Mihai Sucan [:msucan]]

(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.

Comment 43

[Mihai Sucan [:msucan]]

(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+!

Comment 44

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] Part 5: HUDService - cleanup clear/unregisterDisplay()

Comments addressed.

Comment 45

[Mihai Sucan [:msucan]]

Comment on attachment 543494 [details] [diff] [review]
[in-fx-team] Part 1: HUDService - remove getHeadsUpDisplay()

http://hg.mozilla.org/integration/fx-team/rev/978c21f5c68f

Comment 46

[Mihai Sucan [:msucan]]

Comment on attachment 543495 [details] [diff] [review]
[in-fx-team] Part 2: HUDService - remove getOutputNodeById()

http://hg.mozilla.org/integration/fx-team/rev/2f5e953d2fbe

Comment 47

[Mihai Sucan [:msucan]]

Comment on attachment 543496 [details] [diff] [review]
[in-fx-team] Part 3: HUDService - remove getConsoleOutputNode()

http://hg.mozilla.org/integration/fx-team/rev/e5bda2ebf1e4

Comment 48

[Mihai Sucan [:msucan]]

Comment on attachment 543497 [details] [diff] [review]
[in-fx-team] Part 4: HUDService - remove getChromeWindowForContentWindow()

http://hg.mozilla.org/integration/fx-team/rev/152cb06533a6

Comment 49

[Mihai Sucan [:msucan]]

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

Comment 50

[Mihai Sucan [:msucan]]

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

Comment 51

[Mihai Sucan [:msucan]]

Comment on attachment 543500 [details] [diff] [review]
[in-fx-team] Part 7: tests - displaysIndex()

http://hg.mozilla.org/integration/fx-team/rev/1f098ada09bb

Comment 52

[Mihai Sucan [:msucan]]

Comment on attachment 544438 [details] [diff] [review]
[in-fx-team] Part 8: tests - getHeadsUpDisplay()

http://hg.mozilla.org/integration/fx-team/rev/94b065b8ec4e

Comment 53

[Mihai Sucan [:msucan]]

Comment on attachment 543502 [details] [diff] [review]
[in-fx-team] Part 9: tests - getOutputNodeById()

http://hg.mozilla.org/integration/fx-team/rev/abcf5e88d5a1

Comment 54

[Mihai Sucan [:msucan]]

Comment on attachment 543504 [details] [diff] [review]
[in-fx-team] Part 10: tests - cleanup

http://hg.mozilla.org/integration/fx-team/rev/1aaee943f66b

Comment 55

[Mihai Sucan [:msucan]]

Comment on attachment 544439 [details] [diff] [review]
[in-fx-team] Part 11: tests - clearDisplay()

http://hg.mozilla.org/integration/fx-team/rev/59ae546b1da7

Comment 56

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

http://hg.mozilla.org/mozilla-central/rev/978c21f5c68f
http://hg.mozilla.org/mozilla-central/rev/2f5e953d2fbe
http://hg.mozilla.org/mozilla-central/rev/e5bda2ebf1e4
http://hg.mozilla.org/mozilla-central/rev/152cb06533a6
http://hg.mozilla.org/mozilla-central/rev/11286078b11a
http://hg.mozilla.org/mozilla-central/rev/a3657ba60787
http://hg.mozilla.org/mozilla-central/rev/1f098ada09bb
http://hg.mozilla.org/mozilla-central/rev/94b065b8ec4e
http://hg.mozilla.org/mozilla-central/rev/abcf5e88d5a1
http://hg.mozilla.org/mozilla-central/rev/1aaee943f66b
http://hg.mozilla.org/mozilla-central/rev/59ae546b1da7

Comment 57

[Dão Gottwald [:dao]]

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.

Comment 58

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

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)?

Comment 59

[Dão Gottwald [:dao]]

A followup bug is fine as long as someone's working on it.

Comment 60

[Mihai Sucan [:msucan]]

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.

Comment 61

[Dão Gottwald [:dao]]

It's constant, and these kind of leaks currently go unnoticed. See bug 658738 for details.

Comment 62

[Mihai Sucan [:msucan]]

(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.)

Comment 63

[Mihai Sucan [:msucan]]

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.

Comment 64

[Dão Gottwald [:dao]]

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.

Comment 65

[Mihai Sucan [:msucan]]

(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.

Comment 66

[Dão Gottwald [:dao]]

(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.

Comment 67

[Mihai Sucan [:msucan]]

(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...

Comment 68

[Dão Gottwald [:dao]]

> 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?

Comment 69

[Mihai Sucan [:msucan]]

(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.

Comment 70

[Dão Gottwald [:dao]]

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.

Comment 71

[Mihai Sucan [:msucan]]

(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.)

Comment 72

[Mihai Sucan [:msucan]]

For reference: I opened bug 672470 and submitted a patch which fixes memory leaks in the Web Console code.