Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Description • 8 years ago

If you need concise or verbose CC/GC log I can upload it to somewhere.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 1 • 8 years ago

Log in browser console while javascript.options.mem.log=true .

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 3 • 8 years ago

find_roots result:

0x17d1029c0 [DOMEventTargetHelper ]
    --[mScriptOwner]--> 0x1234c6920 [JS Object (Window)]
    --[UnwrapDOMObject(obj)]--> 0x1254d6400 [nsGlobalWindow #37 inner https://web.telegram.org/#/im?p=@YSITD]

    Root 0x17d1029c0 is a ref counted object with 1 unknown edge(s).
    known edges:
       0x17d853190 [IDBObjectStore] --[mTransaction]--> 0x17d1029c0

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 4 • 8 years ago

Leak statistics when enabled XPCOM_MEM_REFCNT_LOG

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 5 • 8 years ago

Parsed from attachment 8745913 [details]

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 6 • 8 years ago

What I do is:

1. open Firefox with a clean profile
2. set memory.ghost_window_timeout_seconds to 1
3. login to web.telegram.org
4. randomly scrolling chats, clicking links on the chats
5. close the tab
6. open about:memory
7. GC & CC
8. measure memory usage

Sometimes it will cause 2 ghost windows, sometimes it doesn't.
Just repeat the steps until the ghost windows appear.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 7 • 8 years ago

Can not reproduce on Linux (Nightly).
The above logs were generated on Mac OS (Gecko 46).

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 8 • 8 years ago

Can not reproduce on Nightly and Mac OS too.
Maybe this is fixed in m-c?

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

I'll try to find some more time tomorrow to look at this.
(sorry wpan, had to run to a meeting today)

If this is FF46 only, finding the fix-range (not regression range this time) would be great, so that we can push the fixes to FF47 (assuming it leaks too) and to FF46 (leaks are bad, so even release builds should get those fixes asap, if possible, IMO)

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 11 • 8 years ago

OK, it may takes some time though.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 12 • 8 years ago

Does not leak on Linux with 46, maybe a Mac OS only problem.

For Mac OS, 47 still got leaks, but only one ghost-window each time, not two.
48 does not seems to leak, need to check if it is an e10s-related issue.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 13 • 8 years ago

Seems not e10s-related.

I guess now I have to bisect then.

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

ahaa, I don't have OSX with me now.

Andrew, could you perhaps help here?
Looks like telegram needs some phonenumber to register to, but a prepaid number was ok too.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 15 • 8 years ago

Another refcnt allocation log during bisecting.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 16 • 8 years ago

This log shows a normal, balanced version of attachment 8745914 [details].
This object should be cycle collectable via IDBObjectStore, but I don't know why sometimes it doesn't work.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 17 • 8 years ago

I guess I've found which object is holding those IDBTransaction objects.
They are hold by CycleCollectedJSRuntime.

For some reasons IDBTransaction will be created while RecursionDepth() is 0,
but CycleCollectedJSRuntime::ProcessMetastableStateQueue never run at aRecursionDepth == 0 until its destruction, so they don't have a chance to leave the queue.

Bug 1195755 also related to this I guess.
I've tried bholley's testcase in that bug, while it does not leak nsGlobalWindow, it still caused shutdown crash.
I'll find another reduced testcase later.
The attached log is the leaked object's refcnt.log.

Do you have any suggestion, Kyle?

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 18 • 8 years ago

BTW, to reproduce this on web.telegram.org, I've found an easier way:

1. open Firefox with a clean profile
2. set memory.ghost_window_timeout_seconds to 1
3. login to web.telegram.org
4. find some groups that have unread messages
5. keep clicking those groups
6. close the tab
7. open about:memory
8. GC & CC
9. measure memory usage

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 19 • 8 years ago

Can you get the stack for where we create the IDBTransaction with RecursionDepth() == 0?

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 20 • 8 years ago

I disabled the ifndef and let it crashes at MOZ_ASSERT (the code is based on the most recent mozilla-beta).

	Andrew McCreight [:mccr8]
	Comment 21 • 8 years ago

It looks like wcpan and khuey are investigating this.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 22 • 8 years ago

Yuck.  We should probably do a microtask checkpoint (i.e. call http://mxr.mozilla.org/mozilla-central/source/xpcom/base/CycleCollectedJSRuntime.cpp#1368 after processing Cocoa events).  Unfortunately I don't know where that is ... probably somewhere in the Mac appshell implementation.

I *think* on other platforms this doesn't happen (i.e. we call into the native event loop from the XPCOM event loop, not the other way around) but we should verify that.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 23 • 8 years ago

This is a reduced testcase.

It looks weird because the original site uses jQuery/AngularJS, that has their own event/promise implementation. This is the closest code path that I can come up with.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 24 • 8 years ago

I should correct my previous inspection, obviously this will affect on all branches, in non-e10s mode.
In e10s mode, content processes will spin another event loop so they won't cause this problem.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #22)
> Yuck.  We should probably do a microtask checkpoint (i.e. call
> http://mxr.mozilla.org/mozilla-central/source/xpcom/base/
> CycleCollectedJSRuntime.cpp#1368 after processing Cocoa events). 
> Unfortunately I don't know where that is ... probably somewhere in the Mac
> appshell implementation.
> 
> I *think* on other platforms this doesn't happen (i.e. we call into the
> native event loop from the XPCOM event loop, not the other way around) but
> we should verify that.

From my inspection, both CycleCollectedJSRuntime::RunInMetastableState and CycleCollectedJSRuntime::ProcessMetastableStateQueue are called from the XPCOM event loop on Linux and Windows.

So we have several options:

1. Call CycleCollectedJSRuntime::AfterProcessTask in somewhere.

I have tried this in several places (in nsAppShell::ProcessGeckoEvents ... etc.), but I always got it crashes.
Maybe I tried wrong places.

2. Tweak the event loop to make it behaves like other platforms. (i.e. always call from the XPCOM event loop)

I personally prefer this option, but I guess this will introduce some side effects, just like option 1.

3. Hack the recursion count

Set it to mRecursionDepth + 1 if it is less then mRecursionDepth.
Seems passed all tests. I think this is easier to back-port to earlier branches.

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 25 • 8 years ago

I think we should do #3 for the moment.

If you were waiting on a response from me before proceeding, you should have used needinfo.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 26 • 8 years ago

OK, I'll remember that.

I've tested this patch in my daily use, didn't see any ghost window again.

Also passed tests on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=320f10229392

I'll send a review request after you are back.

	Ben Kelly [:bkelly, not reviewing]
	Comment 28 • 8 years ago

Comment on attachment 8762838 [details] [diff] [review]
Bug-1261143-Hack-recursion-depth-in-metastable-state.patch

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

Could we include a comment describing why we need to do this?  The nested event loop situation on Mac is kind of obscure and it's not immddiately clear what symptom this code is addressing.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 29 • 8 years ago

Is it possible to increase the recursion depth in the cocoa appshell?  I would prefer that to papering over it here ...

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 30 • 8 years ago

We talked in person, and the answer is no.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 31 • 8 years ago

Comment on attachment 8762838 [details] [diff] [review]
Bug-1261143-Hack-recursion-depth-in-metastable-state.patch

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

Please add a comment explaining this.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 32 • 8 years ago

Add comment that points to this bug.

	Pulsebot
	Comment 33 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2b3bf3ebafe
Hack recursion depth in metastable state for Mac OS. r=khuey

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

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 35 • 8 years ago

Kyle, do you think it would be safe to uplift this?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 36 • 8 years ago

I would be ok with uplifting it to Aurora.

	Ben Kelly [:bkelly, not reviewing]
	Comment 37 • 8 years ago

Wei-Chen, do you mind getting this uplifted to aurora 49?  We want to uplift since it leads to more memory leaks on mac.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 38 • 8 years ago

Comment on attachment 8763774 [details] [diff] [review]
Bug-1261143-Hack-recursion-depth-in-metastable-state.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1264566
[User impact if declined]: Mac users may suffer from memory leaks on certain sites and lead to bad experience.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=320f10229392
[Risks and why]: It does not break IndexedDB or cause crash, I think it's safe to uplift.
[String/UUID change made/needed]: None.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 39 • 8 years ago

Comment on attachment 8763774 [details] [diff] [review]
Bug-1261143-Hack-recursion-depth-in-metastable-state.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1261143
[User impact if declined]: Mac users may suffer from memory leaks on certain sites and lead to bad experience.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=320f10229392
[Risks and why]: It does not break IndexedDB or cause crash, I think it's safe to uplift.
[String/UUID change made/needed]: None.

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 40 • 8 years ago

Revised the bug number.

	Marcia Knous [:marcia]
	Comment 41 • 8 years ago

Tracking 49+ for this ghost windows issue.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 42 • 8 years ago

Comment on attachment 8763774 [details] [diff] [review]
Bug-1261143-Hack-recursion-depth-in-metastable-state.patch

Regression from 45, let's uplift this memory leak fix.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 43 • 8 years ago

This would be good to verify in aurora, maybe with the test case here and the one in bug 1195755 (or known affected sites)

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/8902dc8b9ab1

	Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact mboldan@mozilla.com
	Comment 45 • 8 years ago

I tried reproducing this bug on multiple stations using Fx 48.0a1, build ID 20160411030231 and 20160331030231 but could not do so. Can you please verify the fix?

	Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) Assignee
	Comment 46 • 8 years ago

I can verify this on 48.0b6.

1. open attachment 8751693 [details]
2. click the button
3. close the tab
4. GC & CC in about:memory
5. measure and search bmoattachments, you should see something marked as detached
6. wait until it becomes ghost-window (somehow memory.ghost_window_timeout_seconds does not seems to work in this case)