Web Console cleanup: Kill getHeadsUpDisplay() and other methods

RESOLVED FIXED in Firefox 8

Status

()

Firefox
Developer Tools
P3
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: pcwalton, Assigned: msucan)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(11 attachments, 14 obsolete attachments)

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

Description

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

Comment 1

7 years ago
Correction: This function doesn't return the output node. Rather it returns the HUD box.
(Reporter)

Updated

7 years ago
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
(Reporter)

Comment 2

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

Comment 3

7 years ago
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: 593956
(Reporter)

Comment 4

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

Updated

7 years ago
Depends on: 580618

Updated

7 years ago
Assignee: nobody → mihai.sucan

Comment 5

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

Comment 6

7 years ago
Created attachment 499067 [details] [diff] [review]
Part 1: HUDService - remove getHeadsUpDisplay()
Attachment #499067 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

7 years ago
Created attachment 499068 [details] [diff] [review]
Part 2: HUDService - remove getOutputNodeById()
Attachment #499068 - Flags: review?(gavin.sharp)
(Assignee)

Comment 8

7 years ago
Created attachment 499069 [details] [diff] [review]
Part 3: HUDService - remove getConsoleOutputNode()
Attachment #499069 - Flags: review?(gavin.sharp)
(Assignee)

Comment 9

7 years ago
Created attachment 499071 [details] [diff] [review]
Part 4: HUDService - remove getChromeWindowForContentWindow()
Attachment #499071 - Flags: review?(gavin.sharp)
(Assignee)

Comment 10

7 years ago
Created attachment 499072 [details] [diff] [review]
Part 5: HUDService - cleanup clear/unregisterDisplay()
Attachment #499072 - Flags: review?(gavin.sharp)
(Assignee)

Comment 11

7 years ago
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).
Attachment #499074 - Flags: review?(gavin.sharp)
(Assignee)

Comment 12

7 years ago
Created attachment 499075 [details] [diff] [review]
Part 7: tests - displaysIndex()

Remove the use of displaysIndex() in the HUDService tests.
Attachment #499075 - Flags: review?(gavin.sharp)
(Assignee)

Comment 13

7 years ago
Created attachment 499076 [details] [diff] [review]
Part 8: tests - getHeadsUpDisplay()

Remove the use of getHeadsUpDisplay() in the HUDService tests.
Attachment #499076 - Flags: review?(gavin.sharp)
(Assignee)

Comment 14

7 years ago
Created attachment 499077 [details] [diff] [review]
Part 9: tests - getOutputNodeById()

Remove the use of getOutputNodeById() in the HUDService tests.
Attachment #499077 - Flags: review?(gavin.sharp)
(Assignee)

Comment 15

7 years ago
Created attachment 499079 [details] [diff] [review]
Part 10: tests - cleanup

Remove tests that are no longer relevant.
Attachment #499079 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
Depends on: 611789
Whiteboard: [has-patch][needs-review][post-fx4]

Updated

6 years ago
Whiteboard: [has-patch][needs-review][post-fx4] → [has-patch][needs-review][post-fx4] [console-1]

Updated

6 years ago
Duplicate of this bug: 603176
(Assignee)

Updated

6 years ago
Blocks: 578462
Are these all up to date? We should probably try to get these landed soon.
(Assignee)

Comment 18

6 years ago
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!
Created attachment 540784 [details] [diff] [review]
dummy attachment so that I can obsolete many patches at once
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
(Assignee)

Comment 20

6 years ago
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)
Attachment #543494 - Flags: review?(dcamp)
(Assignee)

Comment 21

6 years ago
Created attachment 543495 [details] [diff] [review]
[in-fx-team] Part 2: HUDService - remove getOutputNodeById()
Attachment #543495 - Flags: review?(dcamp)
(Assignee)

Comment 22

6 years ago
Created attachment 543496 [details] [diff] [review]
[in-fx-team] Part 3: HUDService - remove getConsoleOutputNode()
Attachment #543496 - Flags: review?(dcamp)
(Assignee)

Comment 23

6 years ago
Created attachment 543497 [details] [diff] [review]
[in-fx-team] Part 4: HUDService - remove getChromeWindowForContentWindow()
Attachment #543497 - Flags: review?(dcamp)
(Assignee)

Comment 24

6 years ago
Created attachment 543498 [details] [diff] [review]
Part 5: HUDService - cleanup clear/unregisterDisplay()
Attachment #543498 - Flags: review?(dcamp)
(Assignee)

Comment 25

6 years ago
Created attachment 543499 [details] [diff] [review]
[in-fx-team] Part 6: HUDService - remove displaysIndex() and more
Attachment #543499 - Flags: review?(dcamp)
(Assignee)

Comment 26

6 years ago
Created attachment 543500 [details] [diff] [review]
[in-fx-team] Part 7: tests - displaysIndex()
Attachment #543500 - Flags: review?(dcamp)
(Assignee)

Comment 27

6 years ago
Created attachment 543501 [details] [diff] [review]
Part 8: tests - getHeadsUpDisplay()
Attachment #543501 - Flags: review?(dcamp)
(Assignee)

Comment 28

6 years ago
Created attachment 543502 [details] [diff] [review]
[in-fx-team] Part 9: tests - getOutputNodeById()
Attachment #543502 - Flags: review?(dcamp)
(Assignee)

Comment 29

6 years ago
Created attachment 543504 [details] [diff] [review]
[in-fx-team] Part 10: tests - cleanup
Attachment #543504 - Flags: review?(dcamp)
(Assignee)

Updated

6 years ago
Summary: Web Console cleanup: Kill getHeadsUpDisplay() → Web Console cleanup: Kill getHeadsUpDisplay() and other methods
Version: unspecified → Trunk

Updated

6 years ago
Attachment #543494 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #543495 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #543496 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #543497 - Flags: review?(dcamp) → review+

Comment 30

6 years ago
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+

Updated

6 years ago
Attachment #543500 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #543501 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #543502 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #543504 - Flags: review?(dcamp) → review+
(Assignee)

Comment 31

6 years ago
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/mihai.sucan@gmail.com-68267edfdb12

Updated

6 years ago
Attachment #543498 - Flags: review?(dcamp) → review+
(Assignee)

Comment 32

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

Updated

6 years ago
Attachment #543495 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #543496 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #543497 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #543498 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #543499 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #543500 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #543501 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #543502 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
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?
(Assignee)

Comment 34

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

Comment 37

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

Comment 38

6 years ago
Created attachment 544438 [details] [diff] [review]
[in-fx-team] Part 8: tests - getHeadsUpDisplay()

Rebased the patch.
Attachment #543501 - Attachment is obsolete: true
(Assignee)

Comment 39

6 years ago
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().
Attachment #544439 - Flags: review?(dcamp)

Updated

6 years ago
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?
(Assignee)

Comment 42

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

Comment 43

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

Comment 44

6 years ago
Created attachment 544495 [details] [diff] [review]
[in-fx-team] Part 5: HUDService - cleanup clear/unregisterDisplay()

Comments addressed.
Attachment #544437 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Whiteboard: [has-patch][needs-review][post-fx4] [console-1] → [has-patch][post-fx4] [console-1][land-in-fx-team]
(Assignee)

Comment 45

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

Comment 46

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

Comment 47

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

Comment 48

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

Comment 49

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

Comment 50

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

Comment 51

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

Comment 52

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

Comment 53

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

Comment 54

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

Comment 55

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

Updated

6 years ago
Whiteboard: [has-patch][post-fx4] [console-1][land-in-fx-team] → [has-patch][post-fx4] [console-1][fixed-in-fx-team]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [has-patch][post-fx4] [console-1][fixed-in-fx-team] → [has-patch][post-fx4] [console-1]
Target Milestone: --- → Firefox 8
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.
(Assignee)

Comment 60

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

Comment 62

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

Comment 63

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

Comment 65

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

Comment 67

6 years ago
(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?
(Assignee)

Comment 69

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

Comment 71

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

Comment 72

6 years ago
For reference: I opened bug 672470 and submitted a patch which fixes memory leaks in the Web Console code.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 592519
Whiteboard: [has-patch][post-fx4] [console-1] → [has-patch][post-fx4] [console-1][qa-]
You need to log in before you can comment on or make changes to this bug.