1350960 - DocGroup keeping a strong reference to main thread only object

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

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

DocGroup is threadsafe class, and may be destroyed on any thread, so it keeping a strong pointer to a main-thread only object is not acceptable.
I guess we need to release the pointer in a safe manner when DocGroup is going away, so release the member variable always in the main thread.

Comment 1

[John Dai[:jdai]]

Attachment: patch, v1

I added NS_ReleaseOnMainThread in DocGroup dtor in order to release CustomElementReactionsStack in main thread.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcde1ba569a81f40394ee7d7443a99e2e4d505be&group_state=expanded&filter-tier=1

Comment 2

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

Comment on attachment 8851921 [details] [diff] [review]
patch, v1

hmm, this is actually a bit tricky.
Reactions stack may have strong pointers to Elements, but nothing guarantees DocGroup outlives them. Yet the elements should be released by dispatching to the right DocGroup or TabGroup...

So, since DocGroup has mTabGroup, the releasing runnable should be dispatch to that.
Sorry about not thinking this earlier.

Comment 3

[John Dai[:jdai]]

Attachment: patch, v2

The chrome TabGroup dispatches directly to the main thread.[1][2] I leverage NS_ReleaseOnMainThread in that case. For those not in the main thread case, I use NS_ProxyRelease in order to dispatch to TabGroup existing thread.

[1] https://searchfox.org/mozilla-central/source/dom/base/TabGroup.cpp#31
[2] https://searchfox.org/mozilla-central/source/xpcom/threads/Dispatcher.cpp#184

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf929d38c7fcc2371820175d1de13c419acaf3f1&filter-tier=1&group_state=expanded

Comment 4

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #3)
> Created attachment 8852765 [details] [diff] [review]
> patch, v2
> 
> The chrome TabGroup dispatches directly to the main thread.[1][2] I leverage
> NS_ReleaseOnMainThread in that case. For those not in the main thread case,
> I use NS_ProxyRelease in order to dispatch to TabGroup existing thread.
> 
> [1] https://searchfox.org/mozilla-central/source/dom/base/TabGroup.cpp#31
> [2]
> https://searchfox.org/mozilla-central/source/xpcom/threads/Dispatcher.cpp#184
> 
> try result:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=cf929d38c7fcc2371820175d1de13c419acaf3f1&filter-
> tier=1&group_state=expanded

The reason I use NS_ReleaseOnMainThread is because I hit a MOZ_ASSERT[1]. This assert intents to said that XPCOMShutdown dtor is earlier than DocGroup.

[1] https://searchfox.org/mozilla-central/source/xpcom/threads/Dispatcher.cpp#155
[2] https://searchfox.org/mozilla-central/source/xpcom/threads/Dispatcher.cpp#199

Comment 5

[John Dai[:jdai]]

Attachment: patch, v3

Revise the EventTargetFor task category parameter for TaskCategory::GarbageCollection to TaskCategory::Other.

Comment 6

[John Dai[:jdai]]

Comment on attachment 8852856 [details] [diff] [review]
patch, v3

Change reviewer to William since it's custom element relative stuff.

Comment 7

[John Dai[:jdai]]

Comment on attachment 8852856 [details] [diff] [review]
patch, v3

Cancel the review first, I need to find out comment #4 root cause.

Comment 8

[John Dai[:jdai]]

The problem is about timing issue, we want to use mEventTargets from EventTargetFor[1] in DocGroup dtor, however, it had been set as NULL via do_GetMainThread()[2]. The detail code flow as following:

When we close a tab with e10s enabled, Contentprocess::CleanUp[3] will be called, it calls ShutdownXPCOM[4] to clean up mMainThread[5]. Later, when it's executing nsGlobalWindow dtor[6], the mEventTargets always set to NULL via do_GetMainThread()[2][7].  


[1] https://searchfox.org/mozilla-central/source/xpcom/threads/Dispatcher.cpp#155
[2] https://searchfox.org/mozilla-central/source/xpcom/threads/Dispatcher.cpp#199
[3] https://searchfox.org/mozilla-central/source/dom/ipc/ContentProcess.cpp#261
[4] https://searchfox.org/mozilla-central/source/xpcom/build/XPCOMInit.cpp#914
[5] https://searchfox.org/mozilla-central/source/xpcom/threads/nsThreadManager.cpp#187
[6] https://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#1770-1772
[7] https://searchfox.org/mozilla-central/source/xpcom/threads/nsThreadUtils.cpp#159

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8852856 [details] [diff] [review]
patch, v3

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

::: dom/base/DocGroup.cpp
@@ +46,5 @@
>  DocGroup::~DocGroup()
>  {
>    MOZ_ASSERT(mDocuments.IsEmpty());
> +  if (NS_IsMainThread()) {
> +    NS_ReleaseOnMainThread(mReactionsStack.forget());

If you're on the main thread, you shouldn't need to do anything. We'll release mReactionsStack synchronously, which is fine. I think this solves the problem you're worried about.

@@ +48,5 @@
>    MOZ_ASSERT(mDocuments.IsEmpty());
> +  if (NS_IsMainThread()) {
> +    NS_ReleaseOnMainThread(mReactionsStack.forget());
> +  } else {
> +    nsIEventTarget* target = EventTargetFor(TaskCategory::Other);

This should always return a non-null pointer. If we're not on the main thread, then we haven't reached here yet:
https://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/xpcom/threads/nsThreadManager.cpp#187
since the code before it waits for all threads to shut down.

Comment 10

[John Dai[:jdai]]

Attachment: patch, v4

Address comment #9, which releases CustomElementReactionsStack in non-main thread since it'll release CustomElementReactionsStack synchronously in main thread.

Comment 11

[John Dai[:jdai]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=990dd3a2776d9c7eb57ce691c3a6f6533f6a496d&filter-tier=1&group_state=expanded

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63a210e2f901
Release CustomElementReactionsStack in TabGroup thread when DocGroup is going away. r=billm

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/63a210e2f901