Closed Bug 1249226 Opened 5 years ago Closed 5 years ago

Leaked nsFrameLoader which entrains nsInProcessTabChildGlobal

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: ting, Assigned: ting)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(3 files, 2 obsolete files)

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
(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
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.
Attached patch for testingSplinter Review
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.
Attached patch for debuggingSplinter Review
This is for debugging the aEntry->mRefCnt->get() != 0 assertion.
(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.)
and we seem to have then some weakmap doing xul:browser->some_object_in_tabchildglobal, which means the whole tabchildglobal stays alive.
(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.
And thanks to mccr8, slight clarification. It is browser.permanentKey which is the key.
(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
Attached patch patch v1 (obsolete) — Splinter Review
(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
This no longer blocks bug 1195295 since bug 1249439 and bug 1249795 could fix also the leak.
No longer blocks: 1195295
Priority: P1 → --
Attachment #8721155 - Flags: review?(ttaubert)
The assertions no longer appears with the latest m-c. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d27bb8af09ab
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.
Attachment #8721155 - Flags: review?(ttaubert)
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.
Attachment #8721155 - Attachment is obsolete: true
: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?
Flags: needinfo?(bugs)
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?
Flags: needinfo?(bugs)
(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.)
Whiteboard: [MemShrink] → [MemShrink:P2]
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8723390 - Flags: review?(enndeakin)
smaug & mconley, thanks for your comment, I didn't know there's an API to get weak reference.
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.
Attachment #8723390 - Flags: review?(enndeakin) → review+
Attached patch patch v3Splinter Review
Minor fixes for accessing the weak reference |_prevFocus|, no need for another view.

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

Try looks good.
Keywords: checkin-needed
Assignee: nobody → janus926
Status: NEW → ASSIGNED
Component: General → XUL Widgets
Product: Firefox → Toolkit
https://hg.mozilla.org/mozilla-central/rev/b1df2d21ba2d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.