Open Bug 1167431 Opened 7 years ago Updated 7 years ago

Kill child processes that misuse PBackground

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

ASSIGNED
Tracking Status
firefox41 --- affected

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch Patch, v1 (obsolete) — Splinter Review
Right now we just leave half-baked actors in the manager's array if their RecvFooConstructor message returns false. Normally this triggers the death of the child process so it will get cleaned up eventually, but we can do better.

Also, we need to handle such failures on PBackground.
Attachment #8609059 - Flags: review?(wmccloskey)
This is a diff of one generated file with the lower.py changes.
Attached patch Patch, v1 (obsolete) — Splinter Review
Oops, forgot to qref.
Attachment #8609059 - Attachment is obsolete: true
Attachment #8609059 - Flags: review?(wmccloskey)
Attachment #8609074 - Flags: review?(wmccloskey)
Comment on attachment 8609074 [details] [diff] [review]
Patch, v1

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

::: ipc/glue/BackgroundParentImpl.cpp
@@ +107,5 @@
> +  {
> +    nsAutoCString warning;
> +    warning.AssignLiteral("BackgroundParentImpl::ProcessingError: ");
> +    warning.AppendASCII(aReason);
> +    NS_WARNING(warning.get());

Doesn't MessageChannel already print something very similar to this?
Attachment #8609074 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #3)
> Doesn't MessageChannel already print something very similar to this?

It does, but I can't convince myself that it does so in all cases.
Ah, I was looking at ProtocolErrorBreakpoint, not MessageChannel. MessageChannel does always warn, so I will drop the warning in the new code.
Attached patch Patch, v1.1 (obsolete) — Splinter Review
I moved this to BackgroundImpl and it's just different enough that I figured I should request review again.
Attachment #8609074 - Attachment is obsolete: true
Attachment #8609601 - Flags: review?(wmccloskey)
Attachment #8609601 - Flags: review?(wmccloskey) → review+
Windows problems, they look a little funny. Maybe this needed a clobber?
Ah, so the problem is BackgroundParentImpl::RecvPBackgroundTestConstructor() calls PBackgroundTestParent::Send__delete__(), which unconditionally deletes the subtree and actor. Then it returns the result of the actual send. If the send failed this will be false, and we'll then go into the new cleanup code and double-delete.

Not sure how to handle this pattern really...
Attached patch Patch, v1.1Splinter Review
This is the same patch you already reviewed but without the IPDL changes. That pattern is hard to fix, and this behavior of killing misbehaving PBackground consumers is important, so I'd like to split this out.
Attachment #8609061 - Attachment is obsolete: true
Attachment #8609601 - Attachment is obsolete: true
Attachment #8627808 - Flags: review?(wmccloskey)
Summary: Properly clean up actors whose RecvFooConstructor method returns false → Kill child processes that misuse PBackground
Attachment #8627808 - Flags: review?(wmccloskey) → review+
So the static analysis build failed here because the analysis thinks I'm crazy for using a raw pointer to an AddRef'd class. In this case though I have been very careful to do it correctly, so I kinda want a way to tell the SA to leave me alone... I can't use a smart pointer because I'm not allowed to AddRef on this thread.

Ehsan, Nathan, is there a way to make this work?
Flags: needinfo?(nfroyd)
Flags: needinfo?(ehsan)
MOZ_UNSAFE_REF
mccr8 sounds like he had an idea, though I don't understand it.

My suggestion goes something like:

nsRefPtr<ContentParent> content = GetContentParent();
...
nsRefPtr<nsRunnable> runnable = NS_RunnableFunctionWithArg<StoreBlahPassByBleh>(
  [owningReason](nsRefPtr<ContentParent>&& aContent) {
    // assert main-threaded-ness

    nsRefPtr<ContentParent> content(Move(aContent));
    content->HardKill(owningReason.get());
  }, content.forget();
);

which makes the ownership transfer...slightly more obvious?

Obviously we don't have NS_RunnableFunctionWithArg, but since we've added the static analysis for lambdas, maybe doing so is more feasible than it was a month or so ago.  I think StoreBlahPassByBleh should be StoreCopyPassByRRef<nsRefPtr<ContentParent>>.

Does that seem reasonable?
Flags: needinfo?(nfroyd)
If you tag a field with MOZ_UNSAFE_REF, the analysis won't complain about it.
That's a lot easier than my suggestion. :)
Flags: needinfo?(ehsan)
Oh, apparently this is for the lambda analysis, so maybe the UNSAFE REF won't work. I'll re-ni? Ehsan. :)
Flags: needinfo?(ehsan)
Yeah, I couldn't see anything in the code for the analysis that would check MOZ_UNSAFE_REF. But I don't claim to fully understand that code either!
We talked on IRC and came to the conclusion of not using lambdas here.
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.