[webkit-reviews] review denied: [Bug 51878] Add WebKit1 API to view and delete local storage : [Attachment 85568] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Anton D'Auria <adauria at apple.com>'s
request for review:
Bug 51878: Add WebKit1 API to view and delete local storage
https://bugs.webkit.org/show_bug.cgi?id=51878

Attachment 85568: Patch
https://bugs.webkit.org/attachment.cgi?id=85568&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=85568&action=review

You may want to consider a new bug for this new patch as it may get hard to
sort out the comments on this patch from the rest (and there have already been
a lot of comments in this bug).

These following comments were not addressed. It is good to do one of several
things: 
1. Address them with changes in the code.
2. Address them with an explanation of why they are incorrect. Surprising, but
I am at times. :)
3. Explain why/that you'll address them later.

> Source/WebCore/storage/StorageTracker.cpp:233
> +    LocalStorageTask* task =
LocalStorageTask::createSetOriginDetails(originIdentifier.threadsafeCopy(),
databaseFile).leakPtr();

Ideally this would be a RefPtr<LocalStorageTask> to get the PassRefPtr from
createSetOriginDetails.

Actually I suspect that the task is getting deleted right now in at least one
code path and the callOnMainThread call below just fails. (It should do a
leakRefPtr and then scheduleTask should deref it.) 

Is there a test to exercise the callOnMainThread code path?

> Source/WebCore/storage/StorageTracker.cpp:63
> +	   storageTracker->importOriginIdentifiers();

This call strikes me as odd since  m_isActive is false and
importOriginIdentifiers does nothing in that case.

> Source/WebCore/storage/StorageTracker.cpp:181
Also what guards your DEFINE_STATIC_LOCAL variables?

Specifically, static initialization isn't threadsafe.

Here are my comments about the changes in this patch:

> Source/WebCore/storage/StorageTracker.cpp:147
> +	   openTrackerDatabase(true);

Why is this change being done? (Ideally, there would be a comment for this
function in the ChangeLog to tell me.)

> Source/WebCore/storage/StorageTracker.cpp:150
> +

Whitespace clean ups like this in general are frowned upon. They make these
diffs harder to read without significant benefit.

> Source/WebCore/storage/StorageTracker.cpp:158
> +	   

Please remove this misc whitespace change.

> Source/WebCore/storage/StorageTracker.cpp:199
> +	   originSetCopy = m_originSet;

This is broken. Note that OriginSet is a  HashSet<String> so the copy will just
copy strings (which are basically RefPtr<StringImpl> and StringImpl is
RefCounted -- which is simple ++/-- ref counting, not threadsafe).

In other words, you'll have ref counted StringImpl escaping out of your lock
and that isn't threadsafe.

> Source/WebCore/storage/StorageTracker.cpp:295
> +	   MutexLocker lockClient(m_clientGuard);

You are doing the null check for m_client outside of the lock. It seems like it
would be good to check for it being null after the mutex is taken as well.

> Source/WebCore/storage/StorageTracker.cpp:365
> +	       MutexLocker lockClient(m_clientGuard);

Ditto.

> Source/WebCore/storage/StorageTracker.cpp:476
> +	   MutexLocker lockClient(m_clientGuard);

Ditto.