Bug 29265 - DOM Storage needs to be more careful about where "ThreadSafe" objects are destroyed.
Summary: DOM Storage needs to be more careful about where "ThreadSafe" objects are des...
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jeremy Orlow
Depends on:
Reported: 2009-09-14 19:16 PDT by Jeremy Orlow
Modified: 2009-09-30 11:53 PDT (History)
2 users (show)

See Also:

Patch v1 (12.06 KB, patch)
2009-09-14 19:20 PDT, Jeremy Orlow
abarth: review+
jorlow: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 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.
Comment 1 Jeremy Orlow 2009-09-14 19:20:00 PDT
Created attachment 39584 [details]
Patch v1
Comment 2 Adam Barth 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.
Comment 3 Jeremy Orlow 2009-09-30 11:53:04 PDT
Committed r48939: <http://trac.webkit.org/changeset/48939>