1329682 - Crash in mozilla::dom::cache::CacheStreamControlParent::ActorDestroy

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

This bug was filed from the Socorro interface and is 
report bp-68a9f8ef-f589-4598-a8ef-c38a22170103.
=============================================================

This is happening because the SendPCacheStreamControlConstructor() is failing and destroying the actor.  The CacheStreamControlParent::ActorDestroy() expects that mStreamList has been set, but that is not the case here.  It should handle this gracefully.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Gracefully handle immediate ActorDestroy() calls in CacheStreamControlParent. r=asuth

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8825081 [details] [diff] [review]
Gracefully handle immediate ActorDestroy() calls in CacheStreamControlParent. r=asuth

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

Restating: AutoParentOpResult::SerializeReadStream new()s the CacheStreamControlParent and issues the SendPCacheStreamControlConstructor call.  If the IPC channel Send() call fails in the autogenerated SendPCacheStreamControlConstructor call because the child is gone, the actor will be destroyed.  mStreamList is null at that point because it's only set later in SerializeReadStream.  There is no other potential fallout because the code in SerializeReadStream already checks and returns with an NS_WARNING (and a nice comment, like the one you're adding, thanks!).

::: dom/cache/CacheStreamControlParent.cpp
@@ +84,5 @@
>  CacheStreamControlParent::ActorDestroy(ActorDestroyReason aReason)
>  {
>    NS_ASSERT_OWNINGTHREAD(CacheStreamControlParent);
>    CloseAllReadStreamsWithoutReporting();
> +  // If the initial PSendStreamControlConstructor() fails we will

nit: SendPCacheStreamControlConstructor.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/325c95d5ccf9
Gracefully handle immediate ActorDestroy() calls in CacheStreamControlParent. r=asuth

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8825081 [details] [diff] [review]
Gracefully handle immediate ActorDestroy() calls in CacheStreamControlParent. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Service workers
[User impact if declined]: About 5 crashes per week in release channel.
[Is this code covered by automated tests?]: Yes, but it does not catch this rare corner case.
[Has the fix been verified in Nightly?]: We don't have exact steps to reproduce.
[Needs manual test from QE? If yes, steps to reproduce]: None
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It adds a single nullptr check.
[String changes made/needed]: None

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/325c95d5ccf9

Comment 6

[Gerry Chang [:gchang]]

Comment on attachment 8825081 [details] [diff] [review]
Gracefully handle immediate ActorDestroy() calls in CacheStreamControlParent. r=asuth

Fix a crash. Beta51+ & Aurora52+. Should be in 51 beta 14.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0e97ca9a78348eb062b8ea29592a147ba35aa5d6

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8710e5625d12710f7d9c5afbfb954011ee45aa14