Closed Bug 770542 Opened 12 years ago Closed 12 years ago

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

Categories

(DevTools :: Debugger, defect, P3)

Product:
DevTools
Component:
Debugger
Platform:
x86_64
macOS
Type:
defect

Tracking

(firefox16 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 17
Tracking Flags:
Tracking Status
firefox16 --- fixed

People

(Reporter: emorley, Assigned: vporof)

References

Details

(Keywords: intermittent-failure, memory-leak)

Attachments

(1 file, 1 obsolete file)

v1.1
10.53 KB, patch
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
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
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
Blocks: 731394
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
Attached patch v1 (obsolete) — — Splinter Review 
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 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+
(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.
(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
Attached patch v1.1 — — Splinter Review 

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  
Whiteboard: [orange] → [orange][fixed-in-fx-team]

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/66aed0c15d1e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

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

    
    

    
  
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.

    
    

    
  
(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

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Seems fixed on m-c, basically only Aurora notices so far.

    
  

        Attachment #643054 -
        Attachment is obsolete: true

    
    

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

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #643746 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
https://hg.mozilla.org/releases/mozilla-aurora/rev/6f3913c43f45
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago

          status-firefox16:
          --- → fixed
Resolution: --- → FIXED
Whiteboard: [orange][fixed-in-fx-team] → [orange]
Whiteboard: [orange]

    
  
Product: Firefox → DevTools

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: