Intermittent leak in browser_dbg_bug723069_editor-breakpoints.js, browser_dbg_bug723071_editor-breakpoints-pane.js of browser_dbg_script-switching.html

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Developer Tools: Debugger
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: emorley, Assigned: vporof)

Tracking

({intermittent-failure, mlk})

Trunk
Firefox 17
x86_64
Mac OS X
intermittent-failure, mlk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Rev4 MacOSX Lion 10.7 mozilla-inbound debug test mochitest-other on 2012-07-03 06:57:57 PDT for push da871640d448

slave: talos-r4-lion-058

https://tbpl.mozilla.org/php/getParsedLog.php?id=13194837&tree=Mozilla-Inbound

{
TEST-UNEXPECTED-FAIL | ShutdownLeaks | leaked 8 DOMWindow(s) and 1 DocShell(s) until shutdown

...

[browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js]
  1 window(s) [url = http://example.com/browser/browser/devtools/debugger/test/browser_dbg_script-switching.html]
}
Priority: -- → P3
(Reporter)

Comment 1

5 years ago
https://tbpl.mozilla.org/php/getParsedLog.php?id=13355320&tree=Mozilla-Inbound#error0

[browser/devtools/debugger/test/browser_dbg_bug731394_editor-contextmenu.js]
  1 window(s) [url = http://example.com/browser/browser/devtools/debugger/test/browser_dbg_script-switching.html]
Summary: Intermittent leak in browser_dbg_bug723069_editor-breakpoints.js → Intermittent leak in browser_dbg_bug723069_editor-breakpoints.js & browser_dbg_bug731394_editor-contextmenu.js of browser_dbg_script-switching.html
(Reporter)

Updated

5 years ago
Duplicate of this bug: 751085
(Reporter)

Updated

5 years ago
Blocks: 731394
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Reporter)

Comment 6

5 years ago
https://tbpl.mozilla.org/php/getParsedLog.php?id=13576680&tree=Firefox
Summary: Intermittent leak in browser_dbg_bug723069_editor-breakpoints.js & browser_dbg_bug731394_editor-contextmenu.js of browser_dbg_script-switching.html → Intermittent leak in browser_dbg_bug723069_editor-breakpoints.js, browser_dbg_bug723071_editor-breakpoints-pane.js & browser_dbg_bug731394_editor-contextmenu.js of browser_dbg_script-switching.html
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 48

5 years ago
Created attachment 643054 [details] [diff] [review]
v1

Yeah..

There were a disturbing number of bad things happening.
This fixes stuff on my machine. Running through try soon.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #643054 - Flags: review?(rcampbell)
Comment hidden (Treeherder Robot)
Comment on attachment 643054 [details] [diff] [review]
v1

in debugger-controller.js

+    DebuggerView.destroyEditor();

Not about the change specifically, but the method itself. In debugger-view.js, destroyEditor tells Controller.Breakpoints to destroy() itself. It feels like that sort of thing should be handled in the controller directly.

Maybe add back the DebuggerController.Breakpoints.destroy() method and remove that line from DebuggerView.destroyEditor.

in debugger-view.js

destroyPanes(),
     Prefs.variablesWidth = variables.getAttribute("width");
+
+    let bkps = document.getElementById("breakpoints");
+    let frames = document.getElementById("stackframes");
+    bkps.parentNode.removeChild(bkps);
+    frames.parentNode.removeChild(frames);
+
+    stackframes.parentNode.removeChild(stackframes);
+    variables.parentNode.removeChild(variables);
   },

Maybe this is crazy talk but do you need to also destroy any individual breakpoints the container may be holding onto? For completeness? Or is that crazy talk?

(am I talking crazy?)

ah, no. this.empty() in destroy(). Ignore above craziness.

-    let commandsset = document.createElement("commandsset");
-    commandsset.setAttribute("id", commandsetId);
+    let commandset = document.createElement("commandset");
+    commandset.setAttribute("id", commandsetId);

/me winces.

-      commandsset.appendChild(command);
+      commandset.appendChild(command);

ok, who reviewed this? You're all on notice!

-    document.documentElement.appendChild(commandsset);
+    document.documentElement.appendChild(commandset);

really???

ahem. sorry for the above discursion.

This looks good though I would like to see the setup stuff changed around as mentioned at the beginning. R+ with that and a successful try run.
Attachment #643054 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 51

5 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #50)
> Comment on attachment 643054 [details] [diff] [review]
> v1
> 
> in debugger-controller.js
> 
> +    DebuggerView.destroyEditor();
> 
> Not about the change specifically, but the method itself. In
> debugger-view.js, destroyEditor tells Controller.Breakpoints to destroy()
> itself. It feels like that sort of thing should be handled in the controller
> directly.
> 
> Maybe add back the DebuggerController.Breakpoints.destroy() method and
> remove that line from DebuggerView.destroyEditor.
> 

The breakpoints initialization depends on the editor finishing loading, and that happens in DebuggerView. Since the .initialize() call happens there, and two lines close to it there's .destroy(), I think it's a good idea to keep the two calls together.

> in debugger-view.js
> 
> destroyPanes(),
>      Prefs.variablesWidth = variables.getAttribute("width");
> +
> +    let bkps = document.getElementById("breakpoints");
> +    let frames = document.getElementById("stackframes");
> +    bkps.parentNode.removeChild(bkps);
> +    frames.parentNode.removeChild(frames);
> +
> +    stackframes.parentNode.removeChild(stackframes);
> +    variables.parentNode.removeChild(variables);
>    },
> 
> Maybe this is crazy talk but do you need to also destroy any individual
> breakpoints the container may be holding onto? For completeness? Or is that
> crazy talk?
> 
> (am I talking crazy?)

Not crazy talk.

> 
> ah, no. this.empty() in destroy(). Ignore above craziness.

Yes.
Although this wasn't the source of the leak, it's good to be safe.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 114

5 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #50)
> 
> This looks good though I would like to see the setup stuff changed around as
> mentioned at the beginning. R+ with that and a successful try run.

https://tbpl.mozilla.org/?tree=Try&rev=38916a993023
(Assignee)

Comment 115

5 years ago
Created attachment 643746 [details] [diff] [review]
v1.1
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
https://hg.mozilla.org/integration/fx-team/rev/66aed0c15d1e
Whiteboard: [orange] → [orange][fixed-in-fx-team]
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
https://hg.mozilla.org/mozilla-central/rev/66aed0c15d1e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(In reply to TinderboxPushlog Robot from comment #182)
> ryanvm%gmail.com
> https://tbpl.mozilla.org/php/getParsedLog.php?id=13755727&tree=Mozilla-
> Inbound
> Rev4 MacOSX Snow Leopard 10.6 mozilla-inbound debug test mochitest-other on
> 2012-07-22 07:57:10
> slave: talos-r4-snow-017
> 
> TEST-UNEXPECTED-FAIL | ShutdownLeaks | leaked 4 DOMWindow(s) and 1
> DocShell(s) until shutdown
> TEST-UNEXPECTED-FAIL | browser/base/content/test/browser_bug597218.js |
> leaked 1 window(s) until shutdown [url =
> chrome://browser/content/tabview.html]
> TEST-UNEXPECTED-FAIL | browser/base/content/test/browser_bug597218.js |
> leaked 1 docShell(s) until shutdown
> TEST-UNEXPECTED-FAIL |
> browser/components/places/tests/browser/browser_views_liveupdate.js | leaked
> 1 window(s) until shutdown [url = about:blank]
> TEST-UNEXPECTED-FAIL |
> browser/devtools/debugger/test/browser_dbg_bug731394_editor-contextmenu.js |
> leaked 1 window(s) until shutdown [url =
> http://example.com/browser/browser/devtools/debugger/test/browser_dbg_script-
> switching.html]
> TEST-UNEXPECTED-FAIL |
> toolkit/components/social/test/browser/browser_frameworker.js | leaked 1
> window(s) until shutdown [url = about:blank]

This was on an inbound push from today. And yes, m-c is in sync with inbound with respect to the patch for this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 184

5 years ago
Unfortunately, a lot of other leaking tests have been aggregated into this one, although the causes for each one were different.

In the logs, I see only browser_dbg_bug731394_editor-contextmenu.js leaking in mozilla-inbound, while the other leaks occur only on aurora (where the patch hasn't landed yet).

Let's keep an eye on this and see if the breakpoint specific tests still leak on the trees untouched by the patch.
(Reporter)

Comment 185

5 years ago
(In reply to Victor Porof from comment #184)
> Unfortunately, a lot of other leaking tests have been aggregated into this
> one, although the causes for each one were different.
> 
> In the logs, I see only browser_dbg_bug731394_editor-contextmenu.js leaking
> in mozilla-inbound, while the other leaks occur only on aurora (where the
> patch hasn't landed yet).

I've broken out the browser_dbg_bug731394_editor-contextmenu.js case to bug 751085 again (which had been made a dupe of this bug previously).
Summary: Intermittent leak in browser_dbg_bug723069_editor-breakpoints.js, browser_dbg_bug723071_editor-breakpoints-pane.js & browser_dbg_bug731394_editor-contextmenu.js of browser_dbg_script-switching.html → Intermittent leak in browser_dbg_bug723069_editor-breakpoints.js, browser_dbg_bug723071_editor-breakpoints-pane.js of browser_dbg_script-switching.html
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 199

5 years ago
Seems fixed on m-c, basically only Aurora notices so far.
(Reporter)

Updated

5 years ago
Attachment #643054 - Attachment is obsolete: true
(Reporter)

Comment 200

5 years ago
(In reply to Victor Porof [:vp] from comment #199)
> Seems fixed on m-c, basically only Aurora notices so far.

Is this safe to backport to aurora? If so, would you mind requesting for approval? :-)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 205

5 years ago
Comment on attachment 643746 [details] [diff] [review]
v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Orange test fix
User impact if declined: None, this patch touches test-aware code, for a Developer Tool feature
Testing completed (on m-c, etc.): on m-c and fx-team
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.
Attachment #643746 - Flags: approval-mozilla-aurora?
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)

Updated

5 years ago
Attachment #643746 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Reporter)

Comment 220

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/6f3913c43f45
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
status-firefox16: --- → fixed
Resolution: --- → FIXED
Whiteboard: [orange][fixed-in-fx-team] → [orange]
Keywords: intermittent-failure
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.