755255 - Remove XPCPerThreadData

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

We've had a runtime assertion against off-main/CC thread XPConnect usage for a while now. It's time to start ripping out all the threading goop. This is the first step.

Comment 1

[Bobby Holley (:bholley)]

Patches here: https://github.com/bholley/mozilla-central/commits/dethread

Comment 2

[Bobby Holley (:bholley)]

Patches are done, but there's a small leak that one of them introduces that I need to figure out. Hopefully I'll post for review soon.

Also CCing luke, since I'm sure he'll be happy to know that this is happening. :-)

Comment 3

[Luke Wagner [:luke]]

\o/

Comment 4

[Bobby Holley (:bholley)]

Fixed the shutdown leak. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ceb3f4f2c312

Comment 5

[Bobby Holley (:bholley)]

Attachment: github URL (placeholder)

Looks green. Flagging mrbkap for review.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

I'm going to start doing this review tomorrow.

Comment 7

[Peter Van der Beken [:peterv]]

https://github.com/bholley/mozilla-central/commit/3786481906eba200bf4961e03ae6099ee3690c71 is pretty bogus, TraceXPCGlobal explicitly checks for a null scope. I oppose that patch (and https://github.com/bholley/mozilla-central/commit/2c501610c53973ecee6d35b2432c1f7f2e59403f).

Comment 8

[Bobby Holley (:bholley)]

(In reply to Peter Van der Beken [:peterv] from comment #7)
> https://github.com/bholley/mozilla-central/commit/
> 3786481906eba200bf4961e03ae6099ee3690c71 is pretty bogus, TraceXPCGlobal
> explicitly checks for a null scope. I oppose that patch (and
> https://github.com/bholley/mozilla-central/commit/
> 2c501610c53973ecee6d35b2432c1f7f2e59403f).

The patch to fix this crash is now in bug 766018. I've removed those patches from this stack.

Blake says he now prefers bugzilla patches to github urls, so I'll post them here.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 1 - Hoist mJSContextStack into XPCJSRuntime. v2

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add a convenience function to grab the JS runtime. v1

I started getting sick of typing nsXPConnect::GetXPConnect()->GetRuntime().

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make consumers of GetJSContextStack go through XPCJSRuntime. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 4 - Hoist mCallContext into XPCJSRuntime. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 5 - Make consumers of mCallContext go through XPCJSRuntime. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 6 - Hoist pending exception junk into XPCJSRuntime. v1

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 7 - Hoist auto root list into XPCJSRuntime. v2

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 8 - Remove Usage of XPCPerThreadData::IsMainThread. v2

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 9 - Hoist Resolving* machinery into XPCJSRuntime. v1

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 10 - Remove threadsafety checking machinery. v1

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 11 - Remove mThreadData from XPCCallContext. v1

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 12 - Remove thread iteration in XPCJSRuntime. v1

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 13 - Remove XPCPerThreadData. v1

\o/

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 14 - Rename XPCThreadContext.cpp to XPCJSContextStack.cpp, since that's all that's left. v1

Comment 23

[Bobby Holley (:bholley)]

There you go, blake. ;-) I know the patches look daunting, but they're all just removal / hoistage. Should be pretty easy to review. And once this lands, we can kill all the locks in XPC, which could be a nice free perf win.

Comment 24

[Andrew McCreight [:mccr8]]

(In reply to Bobby Holley (:bholley) from comment #10)
> I started getting sick of typing nsXPConnect::GetXPConnect()->GetRuntime().

You can also do nsXPConnect::GetRuntimeInstance() instead, which is slightly shorter.

Comment 25

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #24)
> (In reply to Bobby Holley (:bholley) from comment #10)
> > I started getting sick of typing nsXPConnect::GetXPConnect()->GetRuntime().
> 
> You can also do nsXPConnect::GetRuntimeInstance() instead, which is slightly
> shorter.

Yeah, I noticed that half-way through. I'd rather not change these patches though. We can unify them in a followup if need be.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 634396 [details] [diff] [review]
Part 1 - Hoist mJSContextStack into XPCJSRuntime. v2

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

::: js/xpconnect/src/nsXPConnect.cpp
@@ +106,5 @@
>  
>      XPCPerThreadData::CleanupAllThreads();
> +
> +    // This needs to happen exactly here, otherwise we leak at shutdown. I don't
> +    // know why. :-(

We have code that intentionally leaks if you touch it after system shutdown (see e.g. nsXPCWrappedJS::SystemIsBeingShutDown), so that could be the reason for this.

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 634398 [details] [diff] [review]
Part 2 - Add a convenience function to grab the JS runtime. v1

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

Please do file a followup on merging the two convenience functions.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 634399 [details] [diff] [review]
Part 3 - Make consumers of GetJSContextStack go through XPCJSRuntime. v1

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3831,5 @@
> +    XPCJSContextStack *stack = XPCJSRuntime::Get()->GetJSContextStack();
> +    MOZ_ASSERT(stack);
> +    if (!stack->Push(sandcx->GetJSContext())) {
> +        JS_ReportError(cx,
> +                       "Unable to initialize XPConnect with the sandbox context");

Can this rewrap now?

::: js/xpconnect/src/XPCThreadContext.cpp
@@ +425,5 @@
>  nsXPCJSContextStackIterator::Reset(nsIJSContextStack *aStack)
>  {
>      NS_ASSERTION(aStack == nsXPConnect::GetXPConnect(),
>                   "aStack must be implemented by XPConnect singleton");
>      XPCPerThreadData* data = XPCPerThreadData::GetData(nsnull);

Seems like data is unused now.

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 634400 [details] [diff] [review]
Part 4 - Hoist mCallContext into XPCJSRuntime. v1

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1036,5 @@
>      if (mJSCycleCollectionContext)
>          JS_DestroyContextNoGC(mJSCycleCollectionContext);
>  
> +    if (mCallContext)
> +        mCallContext->SystemIsBeingShutDown();

This happens later now, but it looks like that's OK. It looks like the comment in XPCCallContext::SystemIsBeingShutDown is out of date now, as well.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 634402 [details] [diff] [review]
Part 6 - Hoist pending exception junk into XPCJSRuntime. v1

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

::: js/xpconnect/src/XPCThrower.cpp
@@ +184,5 @@
> +    nsIExceptionManager * exceptionManager = XPCJSRuntime::Get()->GetExceptionManager();
> +    if (exceptionManager) {
> +       // Ask the provider for the exception, if there is no provider
> +       // we expect it to set e to null
> +        exceptionManager->GetExceptionFromProvider(rv,

The comment looks mis-indented.

::: js/xpconnect/src/xpcprivate.h
@@ +788,5 @@
> +    {
> +        if (EnsureExceptionManager())
> +            return mExceptionManager->GetCurrentException(aException);
> +        nsCOMPtr<nsIException> out = mPendingException;
> +        out.forget(aException);

Why not just

NS_IF_ADDREF(*aException = mPendingException);

?

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 634410 [details] [diff] [review]
Part 12 - Remove thread iteration in XPCJSRuntime. v1

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +687,5 @@
>              // that might no longer be otherwise reachable from the wrappers
>              // or the wrapperprotos.
>  
>              // Skip this part if XPConnect is shutting down. We get into
>              // bad locking problems with the thread iteration otherwise.

We should track down whether we need this check in the new world, now that we don't have any thread iteration to do here.

@@ +697,3 @@
>  
> +                XPCCallContext* ccxp = XPCJSRuntime::Get()->GetCallContext();
> +                while (ccxp) {

This (here and below) looks like a for loop to me, up to you whether it's worth the change, though.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 634411 [details] [diff] [review]
Part 13 - Remove XPCPerThreadData. v1

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

Woo-hoo!

Comment 33

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #27)
> Please do file a followup on merging the two convenience functions.

Filed bug 766892.

Comment 34

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #28)
> Seems like data is unused now.

It goes away in a later patch.

(In reply to Blake Kaplan (:mrbkap) from comment #29)
> This happens later now, but it looks like that's OK. It looks like the
> comment in XPCCallContext::SystemIsBeingShutDown is out of date now, as well.

(In reply to Blake Kaplan (:mrbkap) from comment #31)
> >              // Skip this part if XPConnect is shutting down. We get into
> >              // bad locking problems with the thread iteration otherwise.
> 
> We should track down whether we need this check in the new world, now that
> we don't have any thread iteration to do here.

I'll handle both of these in the following where I grep for "lock" and "thread" in xpconnect and drive that number to zero.

(In reply to Blake Kaplan (:mrbkap) from comment #30)
> > +        nsCOMPtr<nsIException> out = mPendingException;
> > +        out.forget(aException);
> 
> Why not just
> 
> NS_IF_ADDREF(*aException = mPendingException);

I was under the impression that it was preferred to avoid NS_* addreffing macros in new gecko code. I cribbed this style from some of the b2g dom code.

> This (here and below) looks like a for loop to me, up to you whether it's
> worth the change, though.

I'll save it for later. :-)

Comment 35

[Bobby Holley (:bholley)]

Gave this one last try push: https://tbpl.mozilla.org/?tree=Try&rev=b7611b9a7145

Comment 36

[Bobby Holley (:bholley)]

Looks green. Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/7136e735672e
(and ancestors)

Comment 37

[Andrew McCreight [:mccr8]]

Huzzah!

Comment 38

[Bobby Holley (:bholley)]

Stay tuned for the next round of fun over in bug 766889.

Comment 39

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/98e7912c05a7
https://hg.mozilla.org/mozilla-central/rev/58da526df282
https://hg.mozilla.org/mozilla-central/rev/43340a612078
https://hg.mozilla.org/mozilla-central/rev/dde0b55ba4f4
https://hg.mozilla.org/mozilla-central/rev/e957355fd066
https://hg.mozilla.org/mozilla-central/rev/77f929c7118c
https://hg.mozilla.org/mozilla-central/rev/f841f07b2aee
https://hg.mozilla.org/mozilla-central/rev/0581a34dde2a
https://hg.mozilla.org/mozilla-central/rev/1d3ccb427cd3
https://hg.mozilla.org/mozilla-central/rev/86d61e9ab6f2
https://hg.mozilla.org/mozilla-central/rev/f29ad0bb7eb3
https://hg.mozilla.org/mozilla-central/rev/6b5d68a20855
https://hg.mozilla.org/mozilla-central/rev/1e9daec13345
https://hg.mozilla.org/mozilla-central/rev/7136e735672e