Closed
Bug 965982
Opened 10 years ago
Closed 10 years ago
QuotaManager: TransactionThreadPool needs to be shutdown before we shutdown IO thread
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: janv, Assigned: janv)
References
Details
(Keywords: csectype-uaf, sec-high, Whiteboard: [qa-][adv-main28+][adv-esr24.4+])
Attachments
(2 files)
2.61 KB,
patch
|
bent.mozilla
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
lsblakk
:
approval-mozilla-esr24+
|
Details | Diff | Splinter Review |
1.76 KB,
patch
|
janv
:
review+
|
Details | Diff | Splinter Review |
IO thread shutdown involves invalidation of file managers which destroy file info objects (setting database refs to zero). The problem is that there can be a running transaction that is about to lower database refs of a file info and since the file info at that time contains invalid information/database refs, the whole process will end up with destroying already destroyed file info objects. It makes much more sense to shutdown the transaction service first and then the IO thread. This must have been a problem for long time, but not so visible. The patch for storing big structured clones in external files makes it much more visible. Ben says we should land this separately and get it possibly on aurora/beta too
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → Jan.Varga
Attachment #8368190 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•10 years ago
|
Summary: QuotaManager: TransactionThreadPool needs to be shutted down before we shutdown IO thread → QuotaManager: TransactionThreadPool needs to be shutdown before we shutdown IO thread
Comment on attachment 8368190 [details] [diff] [review] fix Review of attachment 8368190 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8368190 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 3•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fcd9f9c189f
Comment 4•10 years ago
|
||
What security rating should this get? Is this still sec-high without bug 964561?
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #4) > What security rating should this get? Is this still sec-high without bug > 964561? Well, with bug 964561, some IDB xpcshell tests crash at shutdown on all platforms permanently. It's probably hard to reproduce without bug 964561, but I remember there is a bug about B2G Desktop sometimes crashing at shutdown.
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6fcd9f9c189f What other branches are affected by this?
Status: NEW → RESOLVED
Closed: 10 years ago
status-b2g-v1.4:
--- → fixed
status-firefox29:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Assignee | ||
Comment 7•10 years ago
|
||
The original patch for storing files landed on 2011-12-16, I'm not 100% sure the issue really goes back to that date and the crash itself should be quite rare.
Comment 8•10 years ago
|
||
This needs a security rating. It shouldn't have been checked in without one since it doesn't just affect trunk. You know this, Ryan! https://wiki.mozilla.org/Security/Bug_Approval_Process
status-b2g18:
--- → affected
status-b2g-v1.1hd:
--- → affected
status-b2g-v1.2:
--- → affected
status-b2g-v1.3:
--- → affected
status-b2g-v1.3T:
--- → affected
status-firefox27:
--- → wontfix
status-firefox28:
--- → affected
status-firefox30:
--- → fixed
status-firefox-esr24:
--- → affected
tracking-b2g-v1.2:
--- → ?
tracking-b2g-v1.3:
--- → ?
tracking-firefox28:
--- → ?
tracking-firefox-esr24:
--- → ?
Updated•10 years ago
|
Updated•10 years ago
|
Comment 9•10 years ago
|
||
(In reply to Al Billings [:abillings] from comment #8) > This needs a security rating. It shouldn't have been checked in without one > since it doesn't just affect trunk. You know this, Ryan! > > https://wiki.mozilla.org/Security/Bug_Approval_Process See comment 3. I'm not the one who landed it ;)
Assignee | ||
Comment 10•10 years ago
|
||
Sorry about that.
Assignee | ||
Comment 12•10 years ago
|
||
Well, the dangling pointer is used to make a virtual function call, I guess sec-critical ?
Flags: needinfo?(Jan.Varga)
Comment 13•10 years ago
|
||
On the bright side, the original patch applies with just different offsets down to esr24. Things are a bit different on b2g18, but it was still pretty mechanical. Would be good for Jan to take a quick look to make sure it looks sane.
Attachment #8375112 -
Flags: review?(Jan.Varga)
Comment 14•10 years ago
|
||
So all we need here is a formal sec rating (and I would assume a retroactive security approval) and beta/esr24 approvals and we should be good to go :)
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8375112 [details] [diff] [review] b2g18 version Review of attachment 8375112 [details] [diff] [review]: ----------------------------------------------------------------- looks good
Attachment #8375112 -
Flags: review?(Jan.Varga) → review+
Updated•10 years ago
|
Keywords: csectype-uaf,
sec-high
Updated•10 years ago
|
Comment 16•10 years ago
|
||
Jan, were you going to do the security approval and branch uplift requests?
Flags: needinfo?(Jan.Varga)
Assignee | ||
Comment 17•10 years ago
|
||
yes, I'll do it
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8368190 [details] [diff] [review] fix [Approval Request Comment] Bug caused by (feature/regressing bug #): Enable storing files in IndexedDB, bug 661877. User impact if declined: The browser can sometimes (if there are unfinished IDB transactions involving stored files) crash during shutdown in an unpredictable way. Testing completed (on m-c, etc.): The fix landed on m-c on Jan 31th, 2014. I'm not aware of any issues caused by this change. Risk to taking this patch (and alternatives if risky): The patch should be safe and can be easily reverted. String or IDL/UUID changes made by this patch: No string/IDL/UUID changes.
Attachment #8368190 -
Flags: approval-mozilla-esr24?
Attachment #8368190 -
Flags: approval-mozilla-beta?
Attachment #8368190 -
Flags: approval-mozilla-b2g28?
Attachment #8368190 -
Flags: approval-mozilla-b2g26?
Attachment #8368190 -
Flags: approval-mozilla-b2g18?
Attachment #8368190 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(Jan.Varga)
Comment 19•10 years ago
|
||
Comment on attachment 8368190 [details] [diff] [review] fix Being a sec-high, this has auto approval for B2G once it lands on the affected Firefox branches :-)
Attachment #8368190 -
Flags: approval-mozilla-b2g28?
Attachment #8368190 -
Flags: approval-mozilla-b2g26?
Attachment #8368190 -
Flags: approval-mozilla-b2g18?
Updated•10 years ago
|
Attachment #8368190 -
Flags: approval-mozilla-esr24?
Attachment #8368190 -
Flags: approval-mozilla-esr24+
Attachment #8368190 -
Flags: approval-mozilla-beta?
Attachment #8368190 -
Flags: approval-mozilla-beta+
Attachment #8368190 -
Flags: approval-mozilla-aurora?
Attachment #8368190 -
Flags: approval-mozilla-aurora+
Comment 20•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/c515e8ba94f7 https://hg.mozilla.org/releases/mozilla-esr24/rev/f091588ed491 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/31036b3aed8d https://hg.mozilla.org/releases/mozilla-b2g18/rev/61e234dde6b7
Comment 21•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/c515e8ba94f7 https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/61e234dde6b7
Comment 22•10 years ago
|
||
Matt, can you please evaluate if this needs testing before release?
Flags: needinfo?(mwobensmith)
Updated•10 years ago
|
Flags: needinfo?(mwobensmith)
Whiteboard: [qa-]
Updated•10 years ago
|
Whiteboard: [qa-] → [qa-][adv-main28+][adv-esr24.4+]
Updated•9 years ago
|
Group: core-security
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
•