Closed
Bug 1271905
Opened 8 years ago
Closed 8 years ago
Crash in mozilla::dom::cache::ReadStream::Inner::Serialize rising in 47
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: philipp, Assigned: bkelly)
Details
(Keywords: crash, regression, Whiteboard: btpp-active)
Crash Data
Attachments
(2 files, 1 obsolete file)
1.46 KB,
patch
|
asuth
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
6.25 KB,
patch
|
asuth
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-091d9b97-c359-477e-ab89-bdbeb2160511. ============================================================= Crashing Thread (70) Frame Module Signature Source 0 xul.dll mozilla::dom::cache::ReadStream::Inner::Serialize(mozilla::dom::cache::CacheReadStream*) dom/cache/ReadStream.cpp:223 1 xul.dll mozilla::dom::cache::ReadStream::Inner::Serialize(mozilla::dom::cache::CacheReadStreamOrVoid*) dom/cache/ReadStream.cpp:205 2 xul.dll mozilla::dom::cache::TypeUtils::SerializeCacheStream(nsIInputStream*, mozilla::dom::cache::CacheReadStreamOrVoid*, mozilla::ErrorResult&) dom/cache/TypeUtils.cpp:514 3 xul.dll mozilla::dom::cache::TypeUtils::ToCacheResponse(mozilla::dom::cache::CacheResponse&, mozilla::dom::Response&, mozilla::ErrorResult&) dom/cache/TypeUtils.cpp:255 4 xul.dll mozilla::dom::cache::AutoChildOpArgs::Add(mozilla::dom::InternalRequest*, mozilla::dom::cache::TypeUtils::BodyAction, mozilla::dom::cache::TypeUtils::SchemeAction, mozilla::dom::Response&, mozilla::ErrorResult&) dom/cache/AutoUtils.cpp:402 5 xul.dll mozilla::dom::cache::Cache::Put(mozilla::dom::RequestOrUSVString const&, mozilla::dom::Response&, mozilla::ErrorResult&) dom/cache/Cache.cpp:416 6 xul.dll mozilla::dom::CacheBinding::put obj-firefox/dom/bindings/CacheBinding.cpp:860 7 xul.dll mozilla::dom::CacheBinding::put_promiseWrapper obj-firefox/dom/bindings/CacheBinding.cpp:880 this signature is rising in builds in the 47 beta cycle. on 47.0b3 this crash is on #34 with 0.3% of all crashes.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
The ReadStream::mControl is nullptr for some reason. Investigating.
Assignee | ||
Comment 2•8 years ago
|
||
I think this is happens when a browser shutdown closes all the streams at the same time someone is trying to put a cache stream back into cache. So the mControl gets cleared in NotedClosed(). We should make Serialize error out if the stream is in this state. Right now its asserting that the stream is open.
Assignee | ||
Comment 3•8 years ago
|
||
[Tracking Requested - why for this release]: Crashing with increasing frequency on beta. We should try to uplift a fix before we merge to release. I don't think we need to uplift to esr45 because service workers are disabled there. The Cache API is much less likely to be used without service workers.
status-firefox46:
--- → wontfix
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox-esr45:
--- → wontfix
tracking-firefox47:
--- → ?
tracking-firefox48:
--- → ?
tracking-firefox49:
--- → ?
Tracked for Fx47 as it's a crash that is increasing in volume across beta builds. Ben, is this something you can help investigate and possibly fix? If not, can you please help find an owner? Thanks!
Flags: needinfo?(bkelly)
Assignee | ||
Comment 5•8 years ago
|
||
Yes. I've assigned myself to the bug and I'm working on it.
Flags: needinfo?(bkelly)
Assignee | ||
Comment 6•8 years ago
|
||
This test change triggers the crash. I'm basically expanding an existing test that creates an orphaned body stream in order to test that we clean up the disk.
Attachment #8751467 -
Flags: review?(bugmail)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8751467 [details] [diff] [review] P2 Test Cache.put() using a Response for a shutdown origin. r=asuth Oh, I meant to make this check for a TypeError before flagging for review. Let me add that code...
Attachment #8751467 -
Flags: review?(bugmail)
Assignee | ||
Comment 8•8 years ago
|
||
Updated test to verify we throw the expected error.
Attachment #8751467 -
Attachment is obsolete: true
Attachment #8751483 -
Flags: review?(bugmail)
Assignee | ||
Comment 9•8 years ago
|
||
This fixes the crash and passes the provided test case. https://treeherder.mozilla.org/#/jobs?repo=try&revision=abb57af681cc
Attachment #8751484 -
Flags: review?(bugmail)
Updated•8 years ago
|
Attachment #8751484 -
Flags: review?(bugmail) → review+
Comment 10•8 years ago
|
||
Comment on attachment 8751483 [details] [diff] [review] P2 Test Cache.put() using a Response for a shutdown origin. r=asuth Review of attachment 8751483 [details] [diff] [review]: ----------------------------------------------------------------- Stating my current understanding of the test (which might be wrong, this is by inspection and I may be missing some stuff): The put triggers the AutoChildOpArgs::Add CacheOpArgs::TCachePutAllArgs branch which invokes TypeUtils::ToCacheResponse which: - marks the body as used which is checked in the second new then() clause. This helps ensure we took the code-path we were expecting. - invokes SerializeCacheStream which will end up down in ReadStream::Inner::Serialize which will generate the error because mState == Closed. Code already exists to check and return the error following that. I don't 100% understand yet the orphaning process that causes the readstream to be closed, but I'm assuming that it's basically: - In the parent, Context::CancelAll gets invoked somehow, and the owning StreamList which is a Context::Activity gets its Cancel invoked. (I'm not clear on how QuotaManagerService::Reset causes the context to get Invalidated though. It seems like something implicit might be happening, like refcounts going to zero. Unless the QuotaInitRunnable stays running longer than I expect...) - This invokes StreamList::CloseAll resulting in the chain CacheStreamControlParent::CloseAll, ::SendCloseAll, CacheStreamControlChild::RecvCloseAll, (StreamControl)::CloseAllReadStreams, [iterated] ReadStream::Inner::Close, ::NoteClosed, [maybe thread bounce], ::NoteClosedOnOwningThread - NoteClosedOnOwningThread does mState.compareExchange(Open, Closed).
Attachment #8751483 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 11•8 years ago
|
||
Yep, that's all correct. I believe the Context::CancelAll() is probably being triggered by browser shutdown. It's possible it's also because QM wants to wipe storage, but I'm less sure of that. In general, though, this control actor does two things: 1) Let's us shutdown the file streams held in the child referencing our cache files. 2) Let's us know when those streams are closed so we can clean up files, etc. Thanks!
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/683942364441 https://hg.mozilla.org/integration/mozilla-inbound/rev/762f3a1b8b09
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8751484 [details] [diff] [review] P1 Don't crash if a closed cache::ReadableStream is serialized. r=asuth Approval Request Comment [Feature/regressing bug #]: Service workers. [User impact if declined]: Crash that has been seen to be growing in beta47. [Describe test coverage new/current, TreeHerder]: New mochitest added in this bug. Existing mochitest and wpt tests to check for regressions. [Risks and why]: Minimal. This only affects service workers and involved making a previously infallible condition fallible. [String/UUID change made/needed]: None.
Attachment #8751484 -
Flags: approval-mozilla-beta?
Attachment #8751484 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8751483 [details] [diff] [review] P2 Test Cache.put() using a Response for a shutdown origin. r=asuth See comment 13.
Attachment #8751483 -
Flags: approval-mozilla-beta?
Attachment #8751483 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
Whiteboard: btpp-active
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/683942364441 https://hg.mozilla.org/mozilla-central/rev/762f3a1b8b09
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8751484 [details] [diff] [review] P1 Don't crash if a closed cache::ReadableStream is serialized. r=asuth Recent crash spike in Fx47 in SWs, Aurora48+, Beta47+
Attachment #8751484 -
Flags: approval-mozilla-beta?
Attachment #8751484 -
Flags: approval-mozilla-beta+
Attachment #8751484 -
Flags: approval-mozilla-aurora?
Attachment #8751484 -
Flags: approval-mozilla-aurora+
Comment on attachment 8751483 [details] [diff] [review] P2 Test Cache.put() using a Response for a shutdown origin. r=asuth Test only patches don't need RelMan review, they are auto-approved.
Attachment #8751483 -
Flags: approval-mozilla-beta?
Attachment #8751483 -
Flags: approval-mozilla-beta+
Attachment #8751483 -
Flags: approval-mozilla-aurora?
Attachment #8751483 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 19•8 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/d768e95103ff remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/805051b9a462 and remote: https://hg.mozilla.org/releases/mozilla-beta/rev/c3ed155abcde remote: https://hg.mozilla.org/releases/mozilla-beta/rev/75849a6f1bd5
Reporter | ||
Comment 20•8 years ago
|
||
thanks for tackling that issue so quickly!
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•