Closed Bug 965982 Opened 6 years ago Closed 6 years ago

QuotaManager: TransactionThreadPool needs to be shutdown before we shutdown IO thread

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 --- wontfix
firefox28 + fixed
firefox29 --- fixed
firefox30 --- fixed
firefox-esr24 28+ fixed
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 28+ fixed
b2g-v1.3 + fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: janv, Assigned: janv)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [qa-][adv-main28+][adv-esr24.4+])

Attachments

(2 files)

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
Blocks: 964561
Attached patch fixSplinter Review
Assignee: nobody → Jan.Varga
Attachment #8368190 - Flags: review?(bent.mozilla)
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+
What security rating should this get?  Is this still sec-high without bug 964561?
(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.
https://hg.mozilla.org/mozilla-central/rev/6fcd9f9c189f

What other branches are affected by this?
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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.
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
(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 ;)
Sorry about that.
So what security rating should this have?
Flags: needinfo?(Jan.Varga)
Well, the dangling pointer is used to make a virtual function call, I guess sec-critical ?
Flags: needinfo?(Jan.Varga)
Attached patch b2g18 versionSplinter Review
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)
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 :)
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+
Jan, were you going to do the security approval and branch uplift requests?
Flags: needinfo?(Jan.Varga)
yes, I'll do it
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 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?
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+
Matt, can you please evaluate if this needs testing before release?
Flags: needinfo?(mwobensmith)
Flags: needinfo?(mwobensmith)
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-main28+][adv-esr24.4+]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.