Bug 56259 – StorageTracker constructor shouldn't have initialization code and isMainThread() assertion

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Anton D'Auria]

All initialization code should be in StorageTracker::initializeTracker(). This removes the requirement that StorageTracker constructor must be called on the main thread.

[Anton D'Auria]

Created attachment 85605 [details]
Patch

[Alice Liu]

I have just a couple questions.  Pardon my lack of understanding of when and where StorageTracker is supposed to exist, but it seems that your patch does not address StorageTracker constructor running on non-main threads, but instead moves the assertion that it's running on the main thread to the initialization of StorageTracker. This seems to me that it doesn't prevent all the non-main threadsfrom creating a StorageTracker instance that doesn't do anything.  Maybe i just need a brief overview of StorageTracker ...

[Anton D'Auria]

(In reply to comment #2)
> I have just a couple questions.  Pardon my lack of understanding of when and where StorageTracker is supposed to exist, but it seems that your patch does not address StorageTracker constructor running on non-main threads, but instead moves the assertion that it's running on the main thread to the initialization of StorageTracker. This seems to me that it doesn't prevent all the non-main threadsfrom creating a StorageTracker instance that doesn't do anything.  Maybe i just need a brief overview of StorageTracker ...

The constructor started the LocalStorageTask thread. In LocalStorageTask, thread creation isn't protected by a mutex, so it must happen on the main thread. I moved this code out of the constructor to the initialization function, which now must run on the main thread, and so I removed the assert in this patch.

[Anton D'Auria]

(In reply to comment #3)
> (In reply to comment #2)
> > I have just a couple questions.  Pardon my lack of understanding of when and where StorageTracker is supposed to exist, but it seems that your patch does not address StorageTracker constructor running on non-main threads, but instead moves the assertion that it's running on the main thread to the initialization of StorageTracker. This seems to me that it doesn't prevent all the non-main threadsfrom creating a StorageTracker instance that doesn't do anything.  Maybe i just need a brief overview of StorageTracker ...
> 
> The constructor started the LocalStorageTask thread. In LocalStorageTask, thread creation isn't protected by a mutex, so it must happen on the main thread. I moved this code out of the constructor to the initialization function, which now must run on the main thread, and so I removed the assert in this patch.

So now, it's fine for the constructor to be called on any thread, but the StorageTracker::initializeTracker() must be called on the main thread for the reason stated above.

[Alice Liu]

ok. r=me

[WebKit Commit Bot]

Comment on attachment 85605 [details]
Patch

Clearing flags on attachment: 85605

Committed r80964: <http://trac.webkit.org/changeset/80964>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.