Closed Bug 1102052 Opened 10 years ago Closed 10 years ago

Don't ignore child-side PBackground protocol errors

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- unaffected
firefox35 --- fixed
firefox36 --- fixed

People

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

References

Details

Attachments

(1 file)

Attached patch Patch, v1Splinter Review
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.
Attachment #8525760 - Flags: review?(mrbkap)
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).
(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 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.
Attachment #8525760 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/7b95b87d4085
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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
Attachment #8525760 - Flags: approval-mozilla-aurora?
Attachment #8525760 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.