1167431 - Kill child processes that misuse PBackground

Status: NEW

(Core :: IPC, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

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.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Diff of generated files (not for checkin)

This is a diff of one generated file with the lower.py changes.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Oops, forgot to qref.

Comment 3

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

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?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ah, I was looking at ProtocolErrorBreakpoint, not MessageChannel. MessageChannel does always warn, so I will drop the warning in the new code.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1.1

I moved this to BackgroundImpl and it's just different enough that I figured I should request review again.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7842a044a183

Comment 8

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9e49636a440

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Windows problems, they look a little funny. Maybe this needed a clobber?

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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...

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1.1

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.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/316c0601d7db

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86f581db40d4

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 15

[Andrew McCreight [:mccr8]]

MOZ_UNSAFE_REF

Comment 16

[Nathan Froyd [:froydnj]]

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?

Comment 17

[Andrew McCreight [:mccr8]]

If you tag a field with MOZ_UNSAFE_REF, the analysis won't complain about it.

Comment 18

[Nathan Froyd [:froydnj]]

That's a lot easier than my suggestion. :)

Comment 19

[Andrew McCreight [:mccr8]]

Oh, apparently this is for the lambda analysis, so maybe the UNSAFE REF won't work. I'll re-ni? Ehsan. :)

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!

Comment 21

[(no longer active)]

We talked on IRC and came to the conclusion of not using lambdas here.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.