Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Remove XPCPerThreadData

RESOLVED FIXED in mozilla16

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(15 attachments)

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 here: https://github.com/bholley/mozilla-central/commits/dethread
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

5 years ago
\o/
Fixed the shutdown leak. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ceb3f4f2c312
Created attachment 624710 [details]
github URL (placeholder)

Looks green. Flagging mrbkap for review.
Attachment #624710 - Flags: review?(mrbkap)

Updated

5 years ago
Blocks: 756067
I'm going to start doing this review tomorrow.
Blocks: 763773
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).
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)
Created attachment 634396 [details] [diff] [review]
Part 1 - Hoist mJSContextStack into XPCJSRuntime. v2
Attachment #634396 - Flags: review?(mrbkap)
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().
Attachment #634398 - Flags: review?(mrbkap)
Created attachment 634399 [details] [diff] [review]
Part 3 - Make consumers of GetJSContextStack go through XPCJSRuntime. v1
Attachment #634399 - Flags: review?(mrbkap)
Created attachment 634400 [details] [diff] [review]
Part 4 - Hoist mCallContext into XPCJSRuntime. v1
Attachment #634400 - Flags: review?(mrbkap)
Created attachment 634401 [details] [diff] [review]
Part 5 - Make consumers of mCallContext go through XPCJSRuntime. v1
Attachment #634401 - Flags: review?(mrbkap)
Created attachment 634402 [details] [diff] [review]
Part 6 - Hoist pending exception junk into XPCJSRuntime. v1
Attachment #634402 - Flags: review?(mrbkap)
Created attachment 634403 [details] [diff] [review]
Part 7 - Hoist auto root list into XPCJSRuntime. v2
Attachment #634403 - Flags: review?(mrbkap)
Created attachment 634404 [details] [diff] [review]
Part 8 - Remove Usage of XPCPerThreadData::IsMainThread. v2
Attachment #634404 - Flags: review?(mrbkap)
Created attachment 634406 [details] [diff] [review]
Part 9 - Hoist Resolving* machinery into XPCJSRuntime. v1
Attachment #634406 - Flags: review?(mrbkap)
Created attachment 634407 [details] [diff] [review]
Part 10 - Remove threadsafety checking machinery. v1
Attachment #634407 - Flags: review?(mrbkap)
Created attachment 634408 [details] [diff] [review]
Part 11 - Remove mThreadData from XPCCallContext. v1
Attachment #634408 - Flags: review?(mrbkap)
Created attachment 634410 [details] [diff] [review]
Part 12 - Remove thread iteration in XPCJSRuntime. v1
Attachment #634410 - Flags: review?(mrbkap)
Created attachment 634411 [details] [diff] [review]
Part 13 - Remove XPCPerThreadData. v1

\o/
Attachment #634411 - Flags: review?(mrbkap)
Created attachment 634412 [details] [diff] [review]
Part 14 - Rename XPCThreadContext.cpp to XPCJSContextStack.cpp, since that's all that's left. v1
Attachment #634412 - 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+

Updated

5 years ago
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+

Updated

5 years ago
Attachment #634403 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #634404 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #634406 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #634407 - Flags: review?(mrbkap) → review+

Updated

5 years ago
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+

Updated

5 years ago
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. :-)
Gave this one last try push: https://tbpl.mozilla.org/?tree=Try&rev=b7611b9a7145
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.
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.