Ting-Yu Chou [:ting] (away) Assignee
	Description • 8 years ago

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

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 1 • 8 years ago

(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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 8 years ago

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

	Mike Conley (:mconley) (:⚙️)
	Comment 8 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 8 years ago

(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

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 12 • 8 years ago

(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

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 13 • 8 years ago

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

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 14 • 8 years ago

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

	Tim Taubert [:ttaubert] (inactive)
	Comment 15 • 8 years ago

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.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 16 • 8 years ago

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.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 17 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 18 • 8 years ago

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?

	Mike Conley (:mconley) (:⚙️)
	Comment 19 • 8 years ago

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

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 21 • 8 years ago

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

	Neil Deakin
	Comment 22 • 8 years ago

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.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 23 • 8 years ago

Thanks for reviewing.

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

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 24 • 8 years ago

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

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

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 25 • 8 years ago

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

Try looks good.

	Pulsebot
	Comment 26 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 27 • 8 years ago
bugherder

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