Closed Bug 1073597 Opened 10 years ago Closed 10 years ago

Don't get a CPOW manager if there are no CPOWs

Categories

(Core :: DOM: Content Processes, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(3 files, 4 obsolete files)

With the patch from bug 1035454 applied, B2G emulator Mochitest 10 hits some weird crash in dom/ipc/tests/test_NuwaProcessDeadlock.html 

The top of the crash stack looks like this:

libxul.so!NS_DebugBreak [nsDebugImpl.cpp:ebc9b75f7344 : 479 + 0x5]
libxul.so!mozilla::dom::PContentChild::SendPJavaScriptConstructor(mozilla::jsipc::PJavaScriptChild*) [PContentChild.cpp : 765 + 0x15]
libxul.so!mozilla::dom::PContentChild::SendPJavaScriptConstructor() [PContentChild.cpp : 734 + 0xd]
libxul.so!mozilla::dom::ContentChild::GetCPOWManager() [ContentChild.cpp:ebc9b75f7344 : 1256 + 0x5]
libxul.so!ChildProcessMessageManagerCallback::DoSendAsyncMessage(JSContext*, nsAString_internal const&, mozilla::dom::StructuredCloneData const&, JS::Handle<JSObject*>, nsIPrincipal*) [nsFrameMessageManager.cpp:ebc9b75f7344 : 1760 + 0x9]
libxul.so!nsFrameMessageManager::DispatchAsyncMessageInternal(JSContext*, nsAString_internal const&, mozilla::dom::StructuredCloneData const&, JS::Handle<JSObject*>, nsIPrincipal*) [nsFrameMessageManager.cpp:ebc9b75f7344 : 670 + 0xf]

We're asserting because we failed to send a message.

For some reason, the crash is then blamed on dom/tests/mochitest/bugs/test_bug132255.html, which is the test we always time out on at the end of M10.  Then later we get further child process crashes, maybe because the parent process has crashed.

Here's a try run with an example of the crash:
  https://tbpl.mozilla.org/?tree=Try&rev=ebc9b75f7344

Anyways, Bill pointed out that there's no particular reason we should be doing anything with CPOWs on B2G, so I wrote a hacky patch that just disables all of the GetCPOWManager() stuff on B2G.  That at least clears up the crash:
  https://tbpl.mozilla.org/?tree=Try&rev=fed47c2cc453
Looking at the code some more, the problem might be that we're not null-checking aCpows in ChildProcessMessageManagerCallback::DoSendAsyncMessage like we do in every other place that calls GetCPOWManager().  I might try that as a slightly simpler approach.  It would still probably be nice to keep GetCPOWManager() returning null on B2G.
Assignee: nobody → continuation
As I said, I'll try something with slightly less #ifdefs but here's what I have so far.
Additionally, this patch makes GetCPOWManager return null on B2G.  The first purpose of this is it quickly indicates any location that uses the manager without checking if there are any CPOWs. Secondly, the other way GetCPOWManager is used is in this pattern:
  CpowIdHolder cpows(Manager()->GetCPOWManager(), aCpows);
This calls GetCPOWManager() whether or not there are any CPOWs, so we have to return null so we don't send a message in that circumstance.  If this is too ugly, I might be able to refactor it so we lazily call GetCPOWManager() inside the CpowIdHolder ctor when we have cpows, or something.

try run: https://tbpl.mozilla.org/?tree=Try&rev=4463898350c0

The failures are all other stuff in the patch stack.
Attachment #8498986 - Flags: review?(bugs)
Comment on attachment 8498986 [details] [diff] [review]
I think the underlying problem here was that some of the places that called wrap did not check if there were any CPOWs, so B2G ended up sending messages to create the manager at some weird time.  This makes calls consistent by doing the check.

So doesn't this crash if one tries to pass object to send(A)syncMessage on b2g?
I think we should throw on b2g in such case.

And need to document in nsIMessageManager that passing objects works only on non-b2g.
Attachment #8498986 - Flags: review?(bugs) → review-
Attachment #8496090 - Attachment is obsolete: true
Attachment #8498986 - Attachment is obsolete: true
Attachment #8499196 - Attachment is obsolete: true
Summary: Don't try to get a CPOW manager on B2G → Don't get a CPOW manager if there are no CPOWs
Sorry for the bugspam, just fixing up various annoying header forward-declaration things.
Attachment #8499206 - Attachment is obsolete: true
Comment on attachment 8499195 [details] [diff] [review]
part 1 - Don't get a CPOW manager to wrap unless there are CPOWs.

I removed the parts that made GetCPOWManager() return null.  Instead, I am fixing things so we never call GetCPOWManager() unless there are CPOWs.  The first part adds the missing null checks to the places where we call GetCPOWManager()->Wrap().
Attachment #8499195 - Flags: review?(bugs)
Comment on attachment 8499228 [details] [diff] [review]
part 2 - Factor out common base class for GetCPOWManager.

Part 2 is the most tedious patch. It does three things:

- Change the return type of GetCPOWManager() from either JavaScriptChild or JavaScriptParent to their common base class, JavaScriptShared.  The only methods we call on the manager are Wrap and Unwrap, so this is okay.  (This is the bulk of the patch.)

- Add a new class CPOWManagerGetter, and hoist the declaration of GetCPOWManager() from nsIContentParent and nsIContentChild into this class. Part 3 will take advantage of this.

- While I was there, I added MOZ_OVERRIDE to all of the GetCPOWManager().

try run: https://tbpl.mozilla.org/?tree=Try&rev=5f123e75dc9c
Attachment #8499228 - Flags: review?(bugs)
Comment on attachment 8499197 [details] [diff] [review]
part 3 - Lazify creation of the CPOW manager in CpowIdHolder.

Now for the payoff from part 2.  This changes the ctor of CpowIdHolder from taking JavaScriptShared* to taking CPOWManagerGetter*.  CPOWManagerGetter is just a thunk that returns JavaScriptShared* on demand.  We create the JavaScriptShared* right in the CpowIdHolder ctor if it is needed.  This way, we call GetCPOWManager() in almost the same place, and the same amount of times, though that may be unnecessary.

There are a lot of changes to dom code in this patch, but they all just change the first ctor argument from a CPOW manager to a CPOW manager getter, so I feel like a separate DOM peer review is unnecessary.  Feel free to review it if you feel like it, though.
Attachment #8499197 - Flags: review?(wmccloskey)
An alternative approach would be to make the common base class have Unwrap and Wrap calls, which would be implemented lazily by calling GetCPOWManager() as needed.  The advantage of this is that it would avoid the fragility of the sort seen in my part 1 patch, but it would also require a fair amount of boilerplate, and the types would also have to change if the underlying JavaScriptShared types changed, which would be a pain.
by the "common base class" I mean CPOWManagerGetter, though obviously in this approach it would have a different name.
Attachment #8499195 - Flags: review?(bugs) → review+
Comment on attachment 8499197 [details] [diff] [review]
part 3 - Lazify creation of the CPOW manager in CpowIdHolder.

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

::: js/ipc/JavaScriptShared.cpp
@@ +516,5 @@
>      return true;
>  }
>  
> +CpowIdHolder::CpowIdHolder(dom::CPOWManagerGetter *managerGetter, const InfallibleTArray<CpowEntry> &cpows)
> +    : js_(nullptr),

Indent two spaces I think.

::: js/ipc/JavaScriptShared.h
@@ +15,5 @@
>  
>  namespace mozilla {
> +
> +namespace dom {
> +class CPOWManagerGetter;

It seems like maybe it would be easier to define CPOWManagerGetter in JavaScriptShared.h?
Attachment #8499197 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #15)
> It seems like maybe it would be easier to define CPOWManagerGetter in
> JavaScriptShared.h?

I'm just trying to avoid dragging JavaScriptShared.h into nsIContentParent.h and nsIContentChild.h (see part 2).
OK, that's fine. It might still be nice to put it in js/ipc and the mozilla::jsipc namespace though. It seems like it belongs there.
(In reply to Bill McCloskey (:billm) from comment #17)
> OK, that's fine. It might still be nice to put it in js/ipc and the
> mozilla::jsipc namespace though. It seems like it belongs there.

Yeah, it is a bit odd. It doesn't entirely make sense in JS because it is something that exists only for DOM, but also it is very related to JS IPC so it doesn't really make sense here either. It would have to go in js/public or something, as all JS exports are being done there. I poked at that a little bit. I don't remember if there were any additional difficulties beyond the js/public ness. I'll see what Olli thinks.
Oh, never mind. We could export it from js/ipc using moz.build, but that's a lot of trouble for this one little class.
Comment on attachment 8499228 [details] [diff] [review]
part 2 - Factor out common base class for GetCPOWManager.

I think this is fine given that CPOWManagerGetter is only implemented in DOM land.
Attachment #8499228 - Flags: review?(bugs) → review+
You need to log in before you can comment on or make changes to this bug.