Closed Bug 1329682 Opened 4 years ago Closed 4 years ago

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

Categories

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

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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 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.
Attachment #8825081 - Flags: review?(bugmail) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/325c95d5ccf9
Gracefully handle immediate ActorDestroy() calls in CacheStreamControlParent. r=asuth
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
Attachment #8825081 - Flags: approval-mozilla-beta?
Attachment #8825081 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/325c95d5ccf9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.
Attachment #8825081 - Flags: approval-mozilla-beta?
Attachment #8825081 - Flags: approval-mozilla-beta+
Attachment #8825081 - Flags: approval-mozilla-aurora?
Attachment #8825081 - Flags: approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.