[webkit-reviews] review denied: [Bug 55919] Yet another multi-threading bug in WebSQLDatabase. : [Attachment 85244] wrapper

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Michael Nordman
<michaeln at google.com>'s request for review:
Bug 55919: Yet another multi-threading bug in WebSQLDatabase.
https://bugs.webkit.org/show_bug.cgi?id=55919

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

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

Just a few comments, so r- for now (but perhaps after a bit of back and forth I
could be convinced to do r+ with some changes on check in).

> Source/WebCore/storage/SQLCallbackWrapper.h:72
> +	   context->postTask(createCallbackTask(&safeRelease,
m_callback.release().leakRef()));

Personally, I would prefer if postTask were done outside of the mutex (since it
can be as long as m_callback.release().leakRef() is stored in a local).

> Source/WebCore/storage/SQLCallbackWrapper.h:77
> +	   ASSERT(!m_callback || m_scriptExecutionContext->isContextThread());

Doesn't the assert need to be inside of the locker?

> Source/WebCore/storage/SQLCallbackWrapper.h:83
> +    bool hasCallback() const { return m_callback; }

I don't understand how this method could be useful unless there is some implied
locking mechanism at a higher level which ensure that it isn't called if clear
or unwrap may be called on another thread.

Given that clear and unwrap both have mutex guards in them, it implies that
they may be called on another thread so this seems concerning.

btw, I'm not arguing for a mutex here as that doesn't fix the problem that I'm
talking about. It feels like this needs a comment in the code because something
tricky is happening here (and in general this method feels like it is prone to
leading people to write broken code).