1102052 - Don't ignore child-side PBackground protocol errors

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

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

Attachment: Patch, v1

Protocol errors on child-side PBackground are being silently dropped at the moment. I can't figure out any way that this causes problems for the parent.

Attached patch causes child processes using PBackground to crash, just like errors in PContent.

For the parent process (i.e. inter-thread PBackground) we have a hard DEBUG-only assertion, and then we try to clean up the channel in both DEBUG and non-DEBUG builds. This at least means that we don't leak memory.

Comment 1

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

Blake, I'd love to get this in before the next merge (so that I can backport to aurora before the new IDB goes to beta).

Comment 2

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

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #0)
> I can't figure out any way that this causes problems for the parent.

Er, I can't figure out any way that this causes problems for the parent *in the multi-process case*. In the single process case we can leak memory at least.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8525760 [details] [diff] [review]
Patch, v1

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

::: ipc/glue/BackgroundChildImpl.cpp
@@ +87,5 @@
> +
> +  MOZ_ASSERT(false, "PBackground processing error!");
> +
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    BackgroundChild::CloseForCurrentThread();

Ben and I chatted about this and decided that we're going to try to hold a hard line and crash all the time, even in same-process PBackground. We might have to go back on this decision, let's give it a shot in the meantime.

That also means that we can remove the MOZ_ASSERT(false) and always give ourselves the nicer error code generated below.

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b95b87d4085

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7b95b87d4085

Comment 6

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

Comment on attachment 8525760 [details] [diff] [review]
Patch, v1

Approval Request Comment
[Feature/regressing bug #]: Bug 956218, though it didn't matter until bug 994190
[User impact if declined]: If a child process crashes at just the right moment we may leak in the parent process and block further database activity.
[Describe test coverage new/current, TBPL]: This patch forces the child to crash (just like it always should have) so we'll see it in crash stats if this case is ever hit.
[Risks and why]: Very low risk
[String/UUID change made/needed]: None

Comment 7

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/56f456359253