RESOLVED FIXED 29265
DOM Storage needs to be more careful about where "ThreadSafe" objects are destroyed. 
https://bugs.webkit.org/show_bug.cgi?id=29265
Summary DOM Storage needs to be more careful about where "ThreadSafe" objects are des...
Jeremy Orlow
Reported 2009-09-14 19:16:26 PDT
DOM Storage needs to be more careful about where "ThreadSafe" objects are destroyed. With the current code, there actually isn't a race condition, but it sure would be easy for someone to introduce one. A bunch of ThreadSafeShared objects have RefPtrs to objects that are NOT ThreadSafeShared objects. If it were possible any of these objects' destructors to be fired off the main thread, then the you'd have a race condition. The code should be more clear and self-documenting about how things related to each other. Since the lifetime of a LocalStorageTask is bounded by the LocalStorageThread which is bounded by the StorageSyncManager, StorageAreaImpl, and StorageAreaSync, there's no reason for LocalStorageTask to store anything other than pointers. By breaking this dependency, we can eliminate the risk. Note that we _could_ have LocalStorageThread's task queue just store LocalStorageTask*'s rather than RefPtr<LocalStorageTask>s but then we'd need to manually take care of deleting. It'd probably also be possible to change LocalStorageThread around so that it needn't hold onto a reference of itself and have a more deterministic shutdown, but my initial attempts to do so failed, and I decided it wasn't worth changing. The queue is killed before hand, so the thread is 100% impotent before the main thread continues anyway. The constructors and destructors of StorageSyncManager, StorageAreaImpl, and StorageAreaSync now have ASSERTs to verify they're running on the main thread. I'm fairly positive that it'd be impossible to hit these asserts and the fact that these classes are no longer ThreadSafeShared should make it clear how they're meant to be used, but I think it's worth it to be extra sure. Of course, ideally, we'd have such an assert every time a ref is incremented or decremented.
Attachments
Patch v1 (12.06 KB, patch)
2009-09-14 19:20 PDT, Jeremy Orlow 		abarth: review+
jorlow: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Jeremy Orlow
Comment 1 2009-09-14 19:20:00 PDT
Created attachment 39584 [details] Patch v1
Adam Barth
Comment 2 2009-09-23 22:33:37 PDT
Comment on attachment 39584 [details] Patch v1 I'm not an expert here, but this patch is syntactically correct and I believe that Jeremy understands the lifetimes of the various objects involved.
Jeremy Orlow
Comment 3 2009-09-30 11:53:04 PDT
Committed r48939: <http://trac.webkit.org/changeset/48939>
Note You need to log in before you can comment on or make changes to this bug.