[webkit-reviews] review denied: [Bug 60197] IndexedDB result of deleting a record should be true or false : [Attachment 112285] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 24 17:31:41 PDT 2011


David Levin <levin at chromium.org> has denied Joshua Bell <jsbell at chromium.org>'s
request for review:
Bug 60197: IndexedDB result of deleting a record should be true or false
https://bugs.webkit.org/show_bug.cgi?id=60197

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

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


It looks like you are calling a v8 bindings only method "trueValue" from
generic code "Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp".

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1958
> +    DEFINE_STATIC_LOCAL(RefPtr<SerializedScriptValue>, trueValue, (0));

Given that this function may be called from different threads (putting it on
SerializedScriptValue implies that somewhat since it may be used from workers).


This doesn't look like a safe way of doing this.

Does the result really need to be cached? Is there some perf test that
indicates this?

(I see that nullValue does this, but I wonder why and I'm concerned about it as
well.)

If nothing else, we should add asserts in these methods to verify that they are
only used on the main thread, but personally I'd prefer that these
non-threadsafe methods not be in SerializedScriptValue (and if caching is
needed perhaps it should happen in code that is known to be single threaded.)

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1959
> +    if (!trueValue) {

Prefer "fail fast":

if (trueValue)
  return trueValue.get();


More information about the webkit-reviews mailing list