1249226 - Leaked nsFrameLoader which entrains nsInProcessTabChildGlobal

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Ting-Yu Chou [:ting] (away)]

In bug 1195295, by running:

  ./mach mochitest browser/base/content/test/general/browser_syncui.js

I observed nsFrameLoader which DestroyComplete() has been called but still has refcnt >1 and gets destroyed very late when nsCycleCollector::Shutdown().

This entrains nsInProcessTabChildGlobal and a lot of things.

I've spotted two places leak nsFrameLoader [1][2]. The wip gets rid of one nsInProcessTabChildGlobal for the test browser_syncui.js. But it got strange errorr on Try [3] for test chunk bc7, and I am still checking. Also not sure are they all the leaks.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#397
[2] https://dxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm#176
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c69d2a640b96

Comment 1

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #0)
> nsInProcessTabChildGlobal for the test browser_syncui.js. But it got strange
> errorr on Try [3] for test chunk bc7, and I am still checking. Also not sure

The error is:

01:05:18     INFO -  Assertion failure: aEntry->mRefCnt->get() != 0 (SelectPointersVisitor: snow-white object in the purple buffer), at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/xpcom/base/nsCycleCollector.cpp:1206

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I see two different assertions there in that try run
(1) Assertion failure: !mScanInProgress (Attempted to call Suspect() while a scan was in progress), at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/xpcom/base/nsCycleCollector.cpp:3437 
and then the
(2) Assertion failure: aEntry->mRefCnt->get() != 0 (SelectPointersVisitor: snow-white object in the purple buffer), at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/xpcom/base/nsCycleCollector.cpp:1206  

(1) is a bug in CloseLog or in nsCycleCollector::ScanRoots, depending on how you see it.
    Moving mLogger->End(); mLogger = nullptr; to be outside the scope where AutoRestore<bool> ar(mScanInProgress); is should fix it.


(2) looks more worrisome.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for testing

So this could help with those assertions.
The change to nsCycleCollector::ScanRoots is fine, but I'm not so happy with
nsCycleCollector::FinishAnyIncrementalGCInProgress().
Finishing IGC shouldn't cause anything to become suspected.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for debugging

This is for debugging the aEntry->mRefCnt->get() != 0 assertion.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Pushed some patches to try 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4246af3adf46
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c54b3e5b76fc
to see if they reveal something useful.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(and there is still another leak, coming from observer service keeping xul:browser element alive, since browser element's ctor adds the element itself to service as a strong ref.)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

and we seem to have then some weakmap doing xul:browser->some_object_in_tabchildglobal, which means the whole tabchildglobal stays alive.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #7)
> and we seem to have then some weakmap doing
> xul:browser->some_object_in_tabchildglobal, which means the whole
> tabchildglobal stays alive.

I remove such a WeakMap in my patch in bug 1195295. I assume we're talking about the same one.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

And thanks to mccr8, slight clarification. It is browser.permanentKey which is the key.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #8)
> I remove such a WeakMap in my patch in bug 1195295. I assume we're talking
> about the same one.
Based on the variable names, yes.
xul:browser element leaking is bug 1249439

Comment 11

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v1

Comment 12

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #1)
> 01:05:18     INFO -  Assertion failure: aEntry->mRefCnt->get() != 0
> (SelectPointersVisitor: snow-white object in the purple buffer), at
> c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/xpcom/base/
> nsCycleCollector.cpp:1206

I tried to reproduce the assertion locally, but no luck, the chunk's tests are different from Try. The command I used was:

  $ ./mach mochitest -f browser-chrome --chunk-by-runtime --total-chunks 7 --this-chunk 7

Comment 13

[Ting-Yu Chou [:ting] (away)]

This no longer blocks bug 1195295 since bug 1249439 and bug 1249795 could fix also the leak.

Comment 14

[Ting-Yu Chou [:ting] (away)]

The assertions no longer appears with the latest m-c. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d27bb8af09ab

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8721155 [details] [diff] [review]
patch v1

Review of attachment 8721155 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/SessionStore.jsm
@@ +784,5 @@
> +
> +        if (aMessage.data.isFinal &&
> +            this._lastKnownFrameLoader.has(browser.permanentKey)) {
> +          this._lastKnownFrameLoader.delete(browser.permanentKey);
> +        }

_lastKnownFrameLoader is a WeakMap, we shouldn't have to do that. If not doing this here causes a leak we should look deeper than just fixing the symptom? Maybe I'm misunderstanding what's happening here but the WeakMap shouldn't keep objects alive.

Comment 16

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8721155 [details] [diff] [review]
patch v1

:ttaubert is right, I should've checked this further.

The actual leak is |_prevFocus|, it points to a xul browser that has been removed from dom tree, which then keeps the |permanentKey| alive:

  https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/popup.xml#293

But I am not sure which event to listen to clear it, am checking around.

Comment 17

[Ting-Yu Chou [:ting] (away)]

:smaug, I am not sure how to fix comment 16. There's a pagehide event when the xul browser unbinds from dom tree, but I don't know is it relevant for clearing |_prevFocus|. Also I wonder is it worth fixing because |_prevFocus| will be set to null when receive popuphidden event, which I guess in general use cases it arrives much earlier than the test (popuphidden when nsWindow::OnContainerFocusOutEvent).

What do you think?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Sorry, I have no idea what _prevFocus is about. That is outside Gecko code after all ;)
But if it is _guaranteed_ that _prevFocus is cleared _soon_ after closing a tab, perhaps then
not worth to change the handling. (but I don't immediately see that being guaranteed)

I wonder if _prevFocus could be an nsIWeakReference to the relevant node?

Comment 19

[Mike Conley (:mconley) (:⚙️)]

(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #18)
> Sorry, I have no idea what _prevFocus is about. That is outside Gecko code
> after all ;)
> But if it is _guaranteed_ that _prevFocus is cleared _soon_ after closing a
> tab, perhaps then
> not worth to change the handling. (but I don't immediately see that being
> guaranteed)
> 
> I wonder if _prevFocus could be an nsIWeakReference to the relevant node?

An nsIWeakReference makes sense. (It looks like the point of _prevFocus is to hold a reference to what was focused before the popup opened so that the focus can be restored once the popup closes.)

Comment 20

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v2

Comment 21

[Ting-Yu Chou [:ting] (away)]

smaug & mconley, thanks for your comment, I didn't know there's an API to get weak reference.

Comment 22

[Neil Deakin]

Comment on attachment 8723390 [details] [diff] [review]
patch v2

Seems ok. I guess this could happen if the popup frame is destroyed while it is open.

Comment 23

[Ting-Yu Chou [:ting] (away)]

Thanks for reviewing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=037a6ac04ffc

Comment 24

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v3

Minor fixes for accessing the weak reference |_prevFocus|, no need for another view.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcfa45916e26

Comment 25

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #24)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcfa45916e26

Try looks good.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b1df2d21ba2d

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b1df2d21ba2d