Closed Bug 755255 Opened 8 years ago Closed 8 years ago

Remove XPCPerThreadData

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(15 files)

60 bytes, text/plain
Details
9.57 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
947 bytes, patch
mrbkap
: review+
Details | Diff | Splinter Review
12.09 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
6.60 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
9.08 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
17.63 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.05 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.80 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.95 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.88 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
5.33 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
8.23 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.00 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1004 bytes, patch
mrbkap
: review+
Details | Diff | Splinter Review
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.
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. :-)
\o/
Fixed the shutdown leak. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ceb3f4f2c312
Looks green. Flagging mrbkap for review.
Attachment #624710 - Flags: review?(mrbkap)
I'm going to start doing this review tomorrow.
Blocks: 763773
Blocks: 765305
(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.
Depends on: 766018
Attachment #624710 - Flags: review?(mrbkap)
I started getting sick of typing nsXPConnect::GetXPConnect()->GetRuntime().
Attachment #634398 - Flags: review?(mrbkap)
\o/
Attachment #634411 - Flags: review?(mrbkap)
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.
(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.
(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 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.
Attachment #634396 - Flags: review?(mrbkap) → review+
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.
Attachment #634398 - Flags: review?(mrbkap) → review+
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.
Attachment #634399 - Flags: review?(mrbkap) → review+
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.
Attachment #634400 - Flags: review?(mrbkap) → review+
Attachment #634401 - Flags: review?(mrbkap) → review+
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);

?
Attachment #634402 - Flags: review?(mrbkap) → review+
Attachment #634403 - Flags: review?(mrbkap) → review+
Attachment #634404 - Flags: review?(mrbkap) → review+
Attachment #634406 - Flags: review?(mrbkap) → review+
Attachment #634407 - Flags: review?(mrbkap) → review+
Attachment #634408 - Flags: review?(mrbkap) → review+
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.
Attachment #634410 - Flags: review?(mrbkap) → review+
Comment on attachment 634411 [details] [diff] [review]
Part 13 - Remove XPCPerThreadData. v1

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

Woo-hoo!
Attachment #634411 - Flags: review?(mrbkap) → review+
Attachment #634412 - Flags: review?(mrbkap) → review+
Blocks: 766889
Blocks: 766892
(In reply to Blake Kaplan (:mrbkap) from comment #27)
> Please do file a followup on merging the two convenience functions.

Filed bug 766892.
(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. :-)
Looks green. Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/7136e735672e
(and ancestors)
Huzzah!
Stay tuned for the next round of fun over in bug 766889.
You need to log in before you can comment on or make changes to this bug.