Last Comment Bug 755255 - Remove XPCPerThreadData
: Remove XPCPerThreadData
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 766018
Blocks: ST-XPConnect 766892 756067 763773 765305 766889
  Show dependency treegraph
 
Reported: 2012-05-15 05:52 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-06-22 03:47 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
github URL (placeholder) (60 bytes, text/plain)
2012-05-17 05:37 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details
Part 1 - Hoist mJSContextStack into XPCJSRuntime. v2 (9.57 KB, patch)
2012-06-19 07:21 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 2 - Add a convenience function to grab the JS runtime. v1 (947 bytes, patch)
2012-06-19 07:21 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 3 - Make consumers of GetJSContextStack go through XPCJSRuntime. v1 (12.09 KB, patch)
2012-06-19 07:21 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 4 - Hoist mCallContext into XPCJSRuntime. v1 (6.60 KB, patch)
2012-06-19 07:22 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 5 - Make consumers of mCallContext go through XPCJSRuntime. v1 (9.08 KB, patch)
2012-06-19 07:22 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 6 - Hoist pending exception junk into XPCJSRuntime. v1 (17.63 KB, patch)
2012-06-19 07:22 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 7 - Hoist auto root list into XPCJSRuntime. v2 (11.05 KB, patch)
2012-06-19 07:22 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 8 - Remove Usage of XPCPerThreadData::IsMainThread. v2 (11.80 KB, patch)
2012-06-19 07:23 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 9 - Hoist Resolving* machinery into XPCJSRuntime. v1 (7.95 KB, patch)
2012-06-19 07:23 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 10 - Remove threadsafety checking machinery. v1 (11.88 KB, patch)
2012-06-19 07:23 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 11 - Remove mThreadData from XPCCallContext. v1 (5.33 KB, patch)
2012-06-19 07:24 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 12 - Remove thread iteration in XPCJSRuntime. v1 (8.23 KB, patch)
2012-06-19 07:24 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 13 - Remove XPCPerThreadData. v1 (11.00 KB, patch)
2012-06-19 07:24 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 14 - Rename XPCThreadContext.cpp to XPCJSContextStack.cpp, since that's all that's left. v1 (1004 bytes, patch)
2012-06-19 07:25 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-05-15 05:52:41 PDT
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) (busy with Stylo) 2012-05-16 05:46:37 PDT
Patches here: https://github.com/bholley/mozilla-central/commits/dethread
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-05-16 13:16:37 PDT
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] 2012-05-16 13:51:00 PDT
\o/
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-05-17 03:31:17 PDT
Fixed the shutdown leak. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ceb3f4f2c312
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-05-17 05:37:22 PDT
Created attachment 624710 [details]
github URL (placeholder)

Looks green. Flagging mrbkap for review.
Comment 6 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2012-05-29 14:45:18 PDT
I'm going to start doing this review tomorrow.
Comment 7 Peter Van der Beken [:peterv] 2012-06-12 03:23:27 PDT
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) (busy with Stylo) 2012-06-19 07:20:15 PDT
(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) (busy with Stylo) 2012-06-19 07:21:26 PDT
Created attachment 634396 [details] [diff] [review]
Part 1 - Hoist mJSContextStack into XPCJSRuntime. v2
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:21:39 PDT
Created attachment 634398 [details] [diff] [review]
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) (busy with Stylo) 2012-06-19 07:21:53 PDT
Created attachment 634399 [details] [diff] [review]
Part 3 - Make consumers of GetJSContextStack go through XPCJSRuntime. v1
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:22:07 PDT
Created attachment 634400 [details] [diff] [review]
Part 4 - Hoist mCallContext into XPCJSRuntime. v1
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:22:21 PDT
Created attachment 634401 [details] [diff] [review]
Part 5 - Make consumers of mCallContext go through XPCJSRuntime. v1
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:22:35 PDT
Created attachment 634402 [details] [diff] [review]
Part 6 - Hoist pending exception junk into XPCJSRuntime. v1
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:22:50 PDT
Created attachment 634403 [details] [diff] [review]
Part 7 - Hoist auto root list into XPCJSRuntime. v2
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:23:04 PDT
Created attachment 634404 [details] [diff] [review]
Part 8 - Remove Usage of XPCPerThreadData::IsMainThread. v2
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:23:28 PDT
Created attachment 634406 [details] [diff] [review]
Part 9 - Hoist Resolving* machinery into XPCJSRuntime. v1
Comment 18 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:23:42 PDT
Created attachment 634407 [details] [diff] [review]
Part 10 - Remove threadsafety checking machinery. v1
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:24:07 PDT
Created attachment 634408 [details] [diff] [review]
Part 11 - Remove mThreadData from XPCCallContext. v1
Comment 20 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:24:32 PDT
Created attachment 634410 [details] [diff] [review]
Part 12 - Remove thread iteration in XPCJSRuntime. v1
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:24:58 PDT
Created attachment 634411 [details] [diff] [review]
Part 13 - Remove XPCPerThreadData. v1

\o/
Comment 22 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:25:12 PDT
Created attachment 634412 [details] [diff] [review]
Part 14 - Rename XPCThreadContext.cpp to XPCJSContextStack.cpp, since that's all that's left. v1
Comment 23 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 07:26:48 PDT
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] 2012-06-19 17:27:25 PDT
(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) (busy with Stylo) 2012-06-20 01:16:42 PDT
(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) (PTO until Jan. 2, 2017) 2012-06-20 14:44:06 PDT
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) (PTO until Jan. 2, 2017) 2012-06-20 15:01:31 PDT
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) (PTO until Jan. 2, 2017) 2012-06-20 15:04:17 PDT
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) (PTO until Jan. 2, 2017) 2012-06-20 15:07:53 PDT
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) (PTO until Jan. 2, 2017) 2012-06-20 15:18:07 PDT
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) (PTO until Jan. 2, 2017) 2012-06-20 15:57:16 PDT
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) (PTO until Jan. 2, 2017) 2012-06-20 16:47:58 PDT
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) (busy with Stylo) 2012-06-21 01:25:34 PDT
(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) (busy with Stylo) 2012-06-21 01:43:29 PDT
(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) (busy with Stylo) 2012-06-21 01:48:08 PDT
Gave this one last try push: https://tbpl.mozilla.org/?tree=Try&rev=b7611b9a7145
Comment 36 Bobby Holley (:bholley) (busy with Stylo) 2012-06-21 07:17:07 PDT
Looks green. Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/7136e735672e
(and ancestors)
Comment 37 Andrew McCreight [:mccr8] 2012-06-21 07:23:06 PDT
Huzzah!
Comment 38 Bobby Holley (:bholley) (busy with Stylo) 2012-06-21 07:24:17 PDT
Stay tuned for the next round of fun over in bug 766889.

Note You need to log in before you can comment on or make changes to this bug.