Last Comment Bug 700512 - Workers + Files exposes threadsafety assertions with DataOwner
: Workers + Files exposes threadsafety assertions with DataOwner
Status: RESOLVED FIXED
[sg:high][qa?]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-07 16:51 PST by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2013-07-25 10:18 PDT (History)
9 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
fixed
+
fixed
-
unaffected


Attachments
Patch (5.45 KB, patch)
2011-11-08 05:16 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (5.53 KB, patch)
2011-11-08 08:11 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
jonas: review-
Details | Diff | Splinter Review
Patch (5.77 KB, patch)
2011-11-08 08:22 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
jonas: review+
benjamin: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-07 16:51:02 PST
Jonas reports that bent has seen threadsafety assertions from DataOwner when manipulating File objects in Workers in certain ways.
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-07 18:52:55 PST
Yeah, we need to make the DataOwner refcounting threadsafe. As things stand now, if a memory-blob is used on both the main thread and a worker thread, the refcount can end up both too high and too low due to races.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-08 05:16:30 PST
Created attachment 572786 [details] [diff] [review]
Patch
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-08 08:11:31 PST
Created attachment 572820 [details] [diff] [review]
Patch
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-08 08:15:52 PST
Comment on attachment 572820 [details] [diff] [review]
Patch

Err.. you only want to delete if the refcount goes to 0, right?
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-08 08:22:18 PST
Created attachment 572824 [details] [diff] [review]
Patch
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-11 05:17:28 PST
If we're respinning 8.0, perhaps we should consider including this?  It's a pretty easy fix to a new bug in 8.0.
Comment 7 Alex Keybl [:akeybl] 2011-11-11 11:26:18 PST
[Triage Comment]
This is sg:high and therefore doesn't meet the criteria for landing on the release branch for a chemspill.
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-16 04:45:03 PST
Review ping.
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-16 17:49:57 PST
Wait... I don't like that this doesn't stabilize the refcount. Having inconsistent implementations of Release seems like a recipe for headaches, and having no stabilization seems like a recipe for double-free.
Comment 10 Benjamin Smedberg [:bsmedberg] 2011-11-17 08:23:05 PST
refcount stabilization was a terrible idea. We should assert if you try to addref a dying object, not propagate stabilization to new macros where it's not already potentially used.
Comment 11 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-17 08:36:00 PST
Fatally asserting sounds fine, but I imagine we'd only do that in debug builds since you have to have some other state bit to track that. For opt builds we'd just have no protection against double frees?
Comment 12 Benjamin Smedberg [:bsmedberg] 2011-11-17 09:08:18 PST
You really don't need another state bit, just use PR_UINT32_MAX as a sentinel value. I'm not sure that the extra branch is worth the protection in release builds, but we should probably measure to see how much it actually costs.
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-18 07:23:31 PST
Checked in without a test case.

https://hg.mozilla.org/mozilla-central/rev/d20b9d389776
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-18 07:24:56 PST
Comment on attachment 572824 [details] [diff] [review]
Patch

Requesting aurora and beta approval for this security regression introduced in Firefox 8.
Comment 15 christian 2011-11-22 10:03:30 PST
Comment on attachment 572824 [details] [diff] [review]
Patch

[triage comment]
Please land on aurora and beta today if at all possible.
Comment 16 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-22 10:09:56 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/a45dd5ac1f90
https://hg.mozilla.org/releases/mozilla-beta/rev/6858e50813de
Comment 17 Daniel Veditz [:dveditz] 2011-11-22 13:59:27 PST
Does this affect the 1.9.2 branch? (qawanted)
Comment 18 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-22 14:04:15 PST
No, File objects are main thread only in 1.9.2.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-07 13:39:41 PST
Is there something QA can do to verify this fix?
Comment 20 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-22 09:20:59 PDT
The tests for this landed in Bug 698621 for reasons that I don't understand.

Note You need to log in before you can comment on or make changes to this bug.