Crash in mozilla::dom::cache::Context::~Context

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM
--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks: 1 bug, {crash})

unspecified
mozilla54
x86
Windows 10
crash
Points:
---

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 fixed, firefox54 fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Assignee)

Description

a year ago
This bug was filed from the Socorro interface and is 
report bp-89657161-7203-4169-81e7-904e72170122.
=============================================================

This is a diagnostic assert triggering:

https://hg.mozilla.org/releases/mozilla-aurora/annotate/a891cb2fa602/dom/cache/Context.cpp#l954

It seems a Context is being destroyed while it has a mNextContext.  That next context is also then being destroyed.  Must be a shutdown condition.
(Assignee)

Comment 1

a year ago
The crash reason actually says:

MOZ_RELEASE_ASSERT(!mData)

Which makes more sense.
(Assignee)

Comment 2

a year ago
Created attachment 8829477 [details] [diff] [review]
Clear Cache API shared data if Context fails to initialize. r=asuth

I don't think this bug has any user observable behavior in release.  It is triggering because we are trying to create a Cache API Manager/Context pair during shutdown.  That causes us to take this failure path:

https://dxr.mozilla.org/mozilla-central/source/dom/cache/Context.cpp#993

The ~Context() destructor, however, wants to ensure that we release our shared Data object on the target IO thread.  Unfortunately that can't happen if the quota system is never opened.  We avoid dooming the shared Data object here:

https://dxr.mozilla.org/mozilla-central/source/dom/cache/Context.cpp#924

Therefore, lets just explicitly clear mData to signal that there is no shared Data to release.  This maintains the destructor invariants.
Attachment #8829477 - Flags: review?(bugmail)
Attachment #8829477 - Flags: review?(bugmail) → review+

Comment 3

a year ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a522c83358fa
Clear Cache API shared data if Context fails to initialize. r=asuth
(Assignee)

Comment 4

a year ago
We only need to uplift this to aurora since its just a diagnostic assertion problem.
status-firefox51: --- → wontfix
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr45: --- → wontfix
(Assignee)

Comment 5

a year ago
Comment on attachment 8829477 [details] [diff] [review]
Clear Cache API shared data if Context fails to initialize. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Cache API
[User impact if declined]: Diagnostic assertions in aurora.  Should not have any impact in release code.
[Is this code covered by automated tests?]: Extensive tests, but they don't trigger this shutdown condition.
[Has the fix been verified in Nightly?]: Its a low frequency race condition and difficult to verify.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal risk
[Why is the change risky/not risky?]: There is minimal risk of regressions because this simply clears some state if an operation is canceled due to shutdown.  This is a relatively rare condition.
[String changes made/needed]: None
Attachment #8829477 - Flags: approval-mozilla-aurora?

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a522c83358fa
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8829477 [details] [diff] [review]
Clear Cache API shared data if Context fails to initialize. r=asuth

Fix a crash. Aurora53+.
Attachment #8829477 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 8

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/334bf10fc3d7
status-firefox53: affected → fixed
You need to log in before you can comment on or make changes to this bug.