962604 - crash using IPDL from the wrong thread with e10s with IPDL message PJavaScript::Get (firefox nightly)

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Nicolas Silva [:nical]]

Attachment: backtrace

In nightly with "browser.tabs.remote" = true, I quickly (and consistently) run into this crash when going to about:config on windows 7.
It's probably not related but I also flipped the pref to disable accelerated layers.

AssertWorkerThread() is failing.
The debugger indicates that the message that we are processing in MessageChannel::Call (aMsg->name_) is "PJavascript::Msg_Get".

I am not sure this is the right component for the bug, is there a e10s-specific component?

Comment 1

[Benjamin Smedberg]

This is weird. This is the worker thread. I suspect that we're calling a method on a dead actor or on a channel that has already been deleted, in which case IPC is not the right component but we don't have a better place to put it.

Is this a debug build? Poison-on-free would probably crash in a more useful spot.

Comment 2

[Andrew McCreight [:mccr8]]

> Poison-on-free would probably crash in a more useful spot.
Or ASAN, though that's not available on Windows.

Comment 3

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

When we navigate to about:config, we're switching the tab to be in-process. If no other tabs are open at the time, that means that the child will get shut down. So a use-after-free makes sense. I'll audit the CPOW code to see if anything pops out. If nothing does, then we can try poisoning.

Comment 4

[Benjamin Smedberg]

JavaScriptParent doesn't have any ActorDestroy method, which is essential for parent-side methods (e.g. if the child crashes, you need to invalidate state).

Comment 5

[Nicolas Silva [:nical]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> Is this a debug build? Poison-on-free would probably crash in a more useful
> spot.

Yes, this is a debug build.

Comment 6

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

Attachment: cpow-crash-fix

Benjamin is right. The problem is that we're sending a CPOW message after the child process has exited. The rules for IPDL is that you're not allowed to send any messages after the ActorDestroyed method is called on an actor. Once that's happened, the MessageChannel has been destroyed and any attempt to send an urgent message will use freed memory.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8368346 [details] [diff] [review]
cpow-crash-fix

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

::: js/ipc/JavaScriptParent.cpp
@@ +94,5 @@
>  };
>  
>  CPOWProxyHandler CPOWProxyHandler::singleton;
>  
> +#define FORWARD(call, ...)                                              \

I might have taken the argument list, as in:

#define FORWARD(call, args) \
... \
return parent->call args;

But that's a matter of taste, so feel free to ignore this comment.

@@ +484,5 @@
>  CPOWProxyHandler::className(JSContext *cx, HandleObject proxy)
>  {
> +    JavaScriptParent *parent = ParentOf(proxy);
> +    if (!parent->active())
> +        return nullptr;

According to comments in the code, this should be infallible (see also <http://mxr.mozilla.org/mozilla-central/source/js/src/jsproxy.cpp?rev=beb52f820ac5#2691>). I wonder if you shouldn't follow the same pattern to avoid callers assuming a non-null return value.

@@ +687,4 @@
>  {
> +    JavaScriptParent *parent = ParentOf(proxy);
> +    if (!parent->active())
> +        return false;

This will silently halt the script. Did you mean to set *bp to false and return true to indicate "no exception"?

Comment 8

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

Attachment: cpow-crash-fix v2

I made the changes to FORWARD and className.

For DOMInstanceOf, I realized that there's no reason it can't behave like the other normal methods. So now it gets passed a JSContext and it throws an exception if things go wrong.

InstanceOf still has to act like an XPCOM method, so I left that one alone.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8370468 [details] [diff] [review]
cpow-crash-fix v2

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

Looks good, thanks.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9a091fd0265

Comment 12

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/e9a091fd0265