Bug 1541628 Comment 6 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

(In reply to :Gijs (he/him) from comment #5)

> Created attachment 9056094 [details]
> Leaked things
> 
> A lot leaks. I expect it's just the entire window leaking and everything it holds references to...

Except the hidden window also shows up as leaking...

I looked at the docs you linked... in the 4th log (which I assume is the last one), looking for browser.xul, I get:

ranger:cclogs gkruitbosch$ python ../heapgraph/find_roots.py cc*-4.log 0x12d137c00
Parsing cc-edges.2590-4.log. Done loading graph. 

0x12dc90d30 [FragmentOrElement (XUL) menu id='historyUndoMenu' chrome://browser/content/browser.xul]
    --[Preserved wrapper]--> 0xc94b0a1a700 [JS Object (XULMenuElement)]
    --[group_global]--> 0x1c401ef8b240 [JS Object (Window)]
    --[UnwrapDOMObject(obj)]--> 0x12d137c00 [nsGlobalWindowInner # 4 inner chrome://browser/content/browser.xul]

    Root 0x12dc90d30 is a ref counted object with 2 unknown edge(s).
    known edges:
       0x12dc90dc0 [FragmentOrElement (XUL) menupopup id='historyUndoPopup' chrome://browser/content/browser.xul] --[GetParent()]--> 0x12dc90d30
       0xc94b0a1a700 [JS Object (XULMenuElement)] --[UnwrapDOMObject(obj)]--> 0x12dc90d30
       0x12dc90ca0 [FragmentOrElement (XUL) menuitem id='hiddenTabsMenu' chrome://browser/content/browser.xul] --[mNextSibling]--> 0x12dc90d30

(For the hidden window, the last mention is in -2.log, and the both -3 and -4 don't mention either hiddenWindow.xul or its address, based on `grep`.)


Is this basically saying the history undo menupopup are holding a reference to the window? You told me to ignore the bits about DMD, so I'm a bit lost as to how to go from there - it's not immediately obvious to me why this would be the case... If I had to guess, some JSM is holding onto that menuitem, but I can't seem to figure out how to find out which jsm that'd be...


(In reply to Andrew McCreight [:mccr8] from comment #4)

> Do you also get a leak if you run a mochitest? Some people (including myself) get leaks all of the time on OSX.

I've seen some leaks before. I don't normally think much of them if infra isn't seeing the same thing when running a test. In this case, I'm aware there are no automated tests for the forget button at all, because mochitest hates you closing the initial window (which the forget button will cause to happen), so back when I wrote the code for the forget button, there wasn't a way to test it. I guess these days perhaps we could write a marionette test...

Anyway, if I just `./mach run` and close the browser after waiting a bit, I don't see leaks. Nor if I ./mach run, open another window, close the first window myself, and then quit the browser. So something the forget button is doing certainly seems "off"...

I don't normally run debug builds, so I don't normally see leaks - I was forced to to investigate some other work I was doing that was hitting debug-only assertions... and then I reviewed a patch for the forget button in bug 1495861 and here we are. :-)
(In reply to :Gijs (he/him) from comment #5)

> Created attachment 9056094 [details]
> Leaked things
> 
> A lot leaks. I expect it's just the entire window leaking and everything it holds references to...

Except the hidden window also shows up as leaking...

I looked at the docs you linked... in the 4th log (which I assume is the last one), looking for browser.xul, I get:

```
$ python ../heapgraph/find_roots.py cc*-4.log 0x12d137c00
Parsing cc-edges.2590-4.log. Done loading graph. 

0x12dc90d30 [FragmentOrElement (XUL) menu id='historyUndoMenu' chrome://browser/content/browser.xul]
    --[Preserved wrapper]--> 0xc94b0a1a700 [JS Object (XULMenuElement)]
    --[group_global]--> 0x1c401ef8b240 [JS Object (Window)]
    --[UnwrapDOMObject(obj)]--> 0x12d137c00 [nsGlobalWindowInner # 4 inner chrome://browser/content/browser.xul]

    Root 0x12dc90d30 is a ref counted object with 2 unknown edge(s).
    known edges:
       0x12dc90dc0 [FragmentOrElement (XUL) menupopup id='historyUndoPopup' chrome://browser/content/browser.xul] --[GetParent()]--> 0x12dc90d30
       0xc94b0a1a700 [JS Object (XULMenuElement)] --[UnwrapDOMObject(obj)]--> 0x12dc90d30
       0x12dc90ca0 [FragmentOrElement (XUL) menuitem id='hiddenTabsMenu' chrome://browser/content/browser.xul] --[mNextSibling]--> 0x12dc90d30
```

(For the hidden window, the last mention is in -2.log, and the both -3 and -4 don't mention either hiddenWindow.xul or its address, based on `grep`.)


Is this basically saying the history undo menupopup are holding a reference to the window? You told me to ignore the bits about DMD, so I'm a bit lost as to how to go from there - it's not immediately obvious to me why this would be the case... If I had to guess, some JSM is holding onto that menuitem, but I can't seem to figure out how to find out which jsm that'd be...


(In reply to Andrew McCreight [:mccr8] from comment #4)

> Do you also get a leak if you run a mochitest? Some people (including myself) get leaks all of the time on OSX.

I've seen some leaks before. I don't normally think much of them if infra isn't seeing the same thing when running a test. In this case, I'm aware there are no automated tests for the forget button at all, because mochitest hates you closing the initial window (which the forget button will cause to happen), so back when I wrote the code for the forget button, there wasn't a way to test it. I guess these days perhaps we could write a marionette test...

Anyway, if I just `./mach run` and close the browser after waiting a bit, I don't see leaks. Nor if I ./mach run, open another window, close the first window myself, and then quit the browser. So something the forget button is doing certainly seems "off"...

I don't normally run debug builds, so I don't normally see leaks - I was forced to to investigate some other work I was doing that was hitting debug-only assertions... and then I reviewed a patch for the forget button in bug 1495861 and here we are. :-)

Back to Bug 1541628 Comment 6