Closed Bug 962604 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: IPC, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: nical, Assigned: billm)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file 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?
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.
> Poison-on-free would probably crash in a more useful spot.
Or ASAN, though that's not available on Windows.
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.
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).
(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.
Attached patch cpow-crash-fix (obsolete) — Splinter Review
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.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8368346 - Flags: review?(mrbkap)
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"?
Attachment #8368346 - Flags: review?(mrbkap)
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.
Attachment #8368346 - Attachment is obsolete: true
Attachment #8370468 - Flags: review?(mrbkap)
Comment on attachment 8370468 [details] [diff] [review]
cpow-crash-fix v2

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

Looks good, thanks.
Attachment #8370468 - Flags: review?(mrbkap) → review+
Depends on: 853209
Duplicate of this bug: 910853
https://hg.mozilla.org/mozilla-central/rev/e9a091fd0265
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.