Last Comment Bug 592469 - Web Console cleanup: Kill getHeadsUpDisplay() and other methods
: Web Console cleanup: Kill getHeadsUpDisplay() and other methods
Status: RESOLVED FIXED
[has-patch][post-fx4] [console-1][qa-]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Firefox 8
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
: 592519 603176 (view as bug list)
Depends on: 580618 611789
Blocks: consolecleanup 578462 devtools4
  Show dependency treegraph
 
Reported: 2010-08-31 15:24 PDT by Patrick Walton (:pcwalton)
Modified: 2011-09-01 15:58 PDT (History)
8 users (show)
gavin.sharp: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: HUDService - remove getHeadsUpDisplay() (3.68 KB, patch)
2010-12-21 10:58 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Part 2: HUDService - remove getOutputNodeById() (4.87 KB, patch)
2010-12-21 10:58 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Part 3: HUDService - remove getConsoleOutputNode() (1.26 KB, patch)
2010-12-21 10:59 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Part 4: HUDService - remove getChromeWindowForContentWindow() (1.95 KB, patch)
2010-12-21 11:01 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Part 5: HUDService - cleanup clear/unregisterDisplay() (4.35 KB, patch)
2010-12-21 11:03 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Part 6: HUDService - remove displaysIndex() (2.82 KB, patch)
2010-12-21 11:08 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Part 7: tests - displaysIndex() (25.65 KB, patch)
2010-12-21 11:09 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Part 8: tests - getHeadsUpDisplay() (27.01 KB, patch)
2010-12-21 11:10 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Part 9: tests - getOutputNodeById() (9.26 KB, patch)
2010-12-21 11:11 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Part 10: tests - cleanup (10.13 KB, patch)
2010-12-21 11:12 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
dummy attachment so that I can obsolete many patches at once (6 bytes, patch)
2011-06-21 09:57 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
[in-fx-team] Part 1: HUDService - remove getHeadsUpDisplay() (3.80 KB, patch)
2011-07-01 13:49 PDT, Mihai Sucan [:msucan]
dcamp: review+
gavin.sharp: review+
Details | Diff | Splinter Review
[in-fx-team] Part 2: HUDService - remove getOutputNodeById() (4.67 KB, patch)
2011-07-01 13:50 PDT, Mihai Sucan [:msucan]
dcamp: review+
gavin.sharp: review+
Details | Diff | Splinter Review
[in-fx-team] Part 3: HUDService - remove getConsoleOutputNode() (1.09 KB, patch)
2011-07-01 13:51 PDT, Mihai Sucan [:msucan]
dcamp: review+
gavin.sharp: review+
Details | Diff | Splinter Review
[in-fx-team] Part 4: HUDService - remove getChromeWindowForContentWindow() (2.05 KB, patch)
2011-07-01 13:53 PDT, Mihai Sucan [:msucan]
dcamp: review+
gavin.sharp: review+
Details | Diff | Splinter Review
Part 5: HUDService - cleanup clear/unregisterDisplay() (6.30 KB, patch)
2011-07-01 13:54 PDT, Mihai Sucan [:msucan]
dcamp: review+
Details | Diff | Splinter Review
[in-fx-team] Part 6: HUDService - remove displaysIndex() and more (6.69 KB, patch)
2011-07-01 13:55 PDT, Mihai Sucan [:msucan]
dcamp: review+
gavin.sharp: review+
Details | Diff | Splinter Review
[in-fx-team] Part 7: tests - displaysIndex() (27.86 KB, patch)
2011-07-01 13:56 PDT, Mihai Sucan [:msucan]
dcamp: review+
Details | Diff | Splinter Review
Part 8: tests - getHeadsUpDisplay() (30.59 KB, patch)
2011-07-01 13:57 PDT, Mihai Sucan [:msucan]
dcamp: review+
Details | Diff | Splinter Review
[in-fx-team] Part 9: tests - getOutputNodeById() (8.64 KB, patch)
2011-07-01 13:58 PDT, Mihai Sucan [:msucan]
dcamp: review+
Details | Diff | Splinter Review
[in-fx-team] Part 10: tests - cleanup (12.70 KB, patch)
2011-07-01 13:59 PDT, Mihai Sucan [:msucan]
dcamp: review+
Details | Diff | Splinter Review
Part 5: HUDService - cleanup clear/unregisterDisplay() (7.02 KB, patch)
2011-07-07 04:39 PDT, Mihai Sucan [:msucan]
gavin.sharp: review+
Details | Diff | Splinter Review
[in-fx-team] Part 8: tests - getHeadsUpDisplay() (30.53 KB, patch)
2011-07-07 04:42 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
[in-fx-team] Part 11: tests - clearDisplay() (6.82 KB, patch)
2011-07-07 04:43 PDT, Mihai Sucan [:msucan]
dcamp: review+
Details | Diff | Splinter Review
[in-fx-team] Part 5: HUDService - cleanup clear/unregisterDisplay() (7.97 KB, patch)
2011-07-07 08:26 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review

Description Patrick Walton (:pcwalton) 2010-08-31 15:24:02 PDT
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) 2010-08-31 15:44:35 PDT
Correction: This function doesn't return the output node. Rather it returns the HUD box.
Comment 2 Patrick Walton (:pcwalton) 2010-11-12 11:48:19 PST
The plan is now to kill getHeadsUpDisplay() entirely.
Comment 3 Kevin Dangoor 2010-11-15 08:36:29 PST
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) 2010-11-15 10:47:34 PST
(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 2010-11-22 11:42:07 PST
mass change: filter on PRIORITYSETTING
Comment 6 Mihai Sucan [:msucan] 2010-12-21 10:58:02 PST
Created attachment 499067 [details] [diff] [review]
Part 1: HUDService - remove getHeadsUpDisplay()
Comment 7 Mihai Sucan [:msucan] 2010-12-21 10:58:53 PST
Created attachment 499068 [details] [diff] [review]
Part 2: HUDService - remove getOutputNodeById()
Comment 8 Mihai Sucan [:msucan] 2010-12-21 10:59:46 PST
Created attachment 499069 [details] [diff] [review]
Part 3: HUDService - remove getConsoleOutputNode()
Comment 9 Mihai Sucan [:msucan] 2010-12-21 11:01:53 PST
Created attachment 499071 [details] [diff] [review]
Part 4: HUDService - remove getChromeWindowForContentWindow()
Comment 10 Mihai Sucan [:msucan] 2010-12-21 11:03:20 PST
Created attachment 499072 [details] [diff] [review]
Part 5: HUDService - cleanup clear/unregisterDisplay()
Comment 11 Mihai Sucan [:msucan] 2010-12-21 11:08:09 PST
Created attachment 499074 [details] [diff] [review]
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] 2010-12-21 11:09:32 PST
Created attachment 499075 [details] [diff] [review]
Part 7: tests - displaysIndex()

Remove the use of displaysIndex() in the HUDService tests.
Comment 13 Mihai Sucan [:msucan] 2010-12-21 11:10:47 PST
Created attachment 499076 [details] [diff] [review]
Part 8: tests - getHeadsUpDisplay()

Remove the use of getHeadsUpDisplay() in the HUDService tests.
Comment 14 Mihai Sucan [:msucan] 2010-12-21 11:11:53 PST
Created attachment 499077 [details] [diff] [review]
Part 9: tests - getOutputNodeById()

Remove the use of getOutputNodeById() in the HUDService tests.
Comment 15 Mihai Sucan [:msucan] 2010-12-21 11:12:42 PST
Created attachment 499079 [details] [diff] [review]
Part 10: tests - cleanup

Remove tests that are no longer relevant.
Comment 16 David Dahl :ddahl 2011-03-14 15:19:46 PDT
*** Bug 603176 has been marked as a duplicate of this bug. ***
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-03-23 12:00:26 PDT
Are these all up to date? We should probably try to get these landed soon.
Comment 18 Mihai Sucan [:msucan] 2011-03-23 12:12:17 PDT
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: gavin@gavinsharp.com] 2011-06-21 09:57:06 PDT
Created attachment 540784 [details] [diff] [review]
dummy attachment so that I can obsolete many patches at once
Comment 20 Mihai Sucan [:msucan] 2011-07-01 13:49:51 PDT
Created attachment 543494 [details] [diff] [review]
[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] 2011-07-01 13:50:43 PDT
Created attachment 543495 [details] [diff] [review]
[in-fx-team] Part 2: HUDService - remove getOutputNodeById()
Comment 22 Mihai Sucan [:msucan] 2011-07-01 13:51:50 PDT
Created attachment 543496 [details] [diff] [review]
[in-fx-team] Part 3: HUDService - remove getConsoleOutputNode()
Comment 23 Mihai Sucan [:msucan] 2011-07-01 13:53:20 PDT
Created attachment 543497 [details] [diff] [review]
[in-fx-team] Part 4: HUDService - remove getChromeWindowForContentWindow()
Comment 24 Mihai Sucan [:msucan] 2011-07-01 13:54:56 PDT
Created attachment 543498 [details] [diff] [review]
Part 5: HUDService - cleanup clear/unregisterDisplay()
Comment 25 Mihai Sucan [:msucan] 2011-07-01 13:55:57 PDT
Created attachment 543499 [details] [diff] [review]
[in-fx-team] Part 6: HUDService - remove displaysIndex() and more
Comment 26 Mihai Sucan [:msucan] 2011-07-01 13:56:42 PDT
Created attachment 543500 [details] [diff] [review]
[in-fx-team] Part 7: tests - displaysIndex()
Comment 27 Mihai Sucan [:msucan] 2011-07-01 13:57:39 PDT
Created attachment 543501 [details] [diff] [review]
Part 8: tests - getHeadsUpDisplay()
Comment 28 Mihai Sucan [:msucan] 2011-07-01 13:58:22 PDT
Created attachment 543502 [details] [diff] [review]
[in-fx-team] Part 9: tests - getOutputNodeById()
Comment 29 Mihai Sucan [:msucan] 2011-07-01 13:59:02 PDT
Created attachment 543504 [details] [diff] [review]
[in-fx-team] Part 10: tests - cleanup
Comment 30 Dave Camp (:dcamp) 2011-07-01 14:52:58 PDT
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 32 Mihai Sucan [:msucan] 2011-07-05 08:19:57 PDT
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: gavin@gavinsharp.com] 2011-07-06 12:57:47 PDT
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] 2011-07-06 13:22:33 PDT
(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: gavin@gavinsharp.com] 2011-07-06 13:44:45 PDT
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: gavin@gavinsharp.com] 2011-07-06 13:50:51 PDT
(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] 2011-07-07 04:39:46 PDT
Created attachment 544437 [details] [diff] [review]
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] 2011-07-07 04:42:14 PDT
Created attachment 544438 [details] [diff] [review]
[in-fx-team] Part 8: tests - getHeadsUpDisplay()

Rebased the patch.
Comment 39 Mihai Sucan [:msucan] 2011-07-07 04:43:43 PDT
Created attachment 544439 [details] [diff] [review]
[in-fx-team] Part 11: tests - clearDisplay()

New patch to clean the tests - to update the calls for clearDisplay().
Comment 40 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-07 07:45:52 PDT
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: gavin@gavinsharp.com] 2011-07-07 07:48:03 PDT
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] 2011-07-07 07:56:25 PDT
(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] 2011-07-07 08:00:36 PDT
(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] 2011-07-07 08:26:02 PDT
Created attachment 544495 [details] [diff] [review]
[in-fx-team] Part 5: HUDService - cleanup clear/unregisterDisplay()

Comments addressed.
Comment 45 Mihai Sucan [:msucan] 2011-07-08 02:36:06 PDT
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] 2011-07-08 02:36:59 PDT
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] 2011-07-08 02:38:04 PDT
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] 2011-07-08 02:39:29 PDT
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] 2011-07-08 02:41:26 PDT
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] 2011-07-08 02:42:41 PDT
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] 2011-07-08 02:43:47 PDT
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] 2011-07-08 02:45:05 PDT
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] 2011-07-08 02:46:17 PDT
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] 2011-07-08 02:47:36 PDT
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] 2011-07-08 02:48:58 PDT
Comment on attachment 544439 [details] [diff] [review]
[in-fx-team] Part 11: tests - clearDisplay()

http://hg.mozilla.org/integration/fx-team/rev/59ae546b1da7
Comment 57 Dão Gottwald [:dao] 2011-07-15 08:03:47 PDT
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: gavin@gavinsharp.com] 2011-07-15 11:31:12 PDT
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] 2011-07-15 11:36:09 PDT
A followup bug is fine as long as someone's working on it.
Comment 60 Mihai Sucan [:msucan] 2011-07-15 11:43:57 PDT
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] 2011-07-15 11:54:59 PDT
It's constant, and these kind of leaks currently go unnoticed. See bug 658738 for details.
Comment 62 Mihai Sucan [:msucan] 2011-07-15 11:59:27 PDT
(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] 2011-07-16 04:13:05 PDT
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] 2011-07-16 08:55:19 PDT
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] 2011-07-18 09:53:50 PDT
(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] 2011-07-18 10:19:07 PDT
(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] 2011-07-18 10:22:30 PDT
(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] 2011-07-18 10:28:11 PDT
> 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] 2011-07-18 10:32:16 PDT
(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] 2011-07-18 10:58:31 PDT
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] 2011-07-18 11:02:39 PDT
(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] 2011-07-19 05:49:10 PDT
For reference: I opened bug 672470 and submitted a patch which fixes memory leaks in the Web Console code.
Comment 73 Mihai Sucan [:msucan] 2011-07-22 03:51:17 PDT
*** Bug 592519 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.