Bug 83526 – [V8] IndexedDB: Cursor value modifications should be preserved until cursor iterates

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joshua Bell]

The IndexedDB spec http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html sayeth, re: IDBCursorWithValue

"Returns the cursor's current value. Note that if this property returns an object, it returns the same object instance every time it is inspected, until the cursor's value is changed. This means that if the object is modified, those modifications will be seen by anyone inspecting the value of the cursor. However modifying such an object does not modify the contents of the database."

The WebKit implementation does not currently support the "same object instance every time" semantics, meaning the following test would fail:

// assume cursor.value = {a: 1, b: 2}
cursor.value.a = 3;
delete cursor.value.b;
cursor.value.foo = "bar";

assert(cursor.value.a === 3);
assert(cursor.value.b === undefined);
assert(cursor.value.foo === "bar");

It may be possible to do this via [CachedAttribute] or, since IDBAny already has a custom V8 binding, with custom caching code. Or possible via an IDL attribute on IDBCursorWithValue that ensure the same IDBAny is returned each time?

[Joshua Bell]

Created attachment 136345 [details]
Layout test for spec behavior

[Joshua Bell]

This is pesky. I thought it might be as simple as changing IDBCursorWithValue.idl

-        readonly attribute IDBAny value;
+        readonly attribute [CachedAttribute] SerializedScriptValue value;

(plus appropriate .h/.cpp updates) but this is insufficient since the same IDBCursor object is returned across continue() calls and so the cached value gets re-used incorrectly.

FWIW, if the above is done the autogenerated code (for V8) looks like:


static v8::Handle<v8::Value> valueAttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
{
    INC_STATS("DOM.IDBCursorWithValue.value._get");
    v8::Handle<v8::String> propertyName = v8::String::NewSymbol("value");
    v8::Handle<v8::Value> value = info.Holder()->GetHiddenValue(propertyName);
    if (!value.IsEmpty())
        return value;
    IDBCursorWithValue* imp = V8IDBCursorWithValue::toNative(info.Holder());
    SerializedScriptValue* serialized = imp->value();
    value = serialized ? serialized->deserialize() : v8::Handle<v8::Value>(v8::Null(info.GetIsolate()));
    info.Holder()->SetHiddenValue(propertyName, value);
    return value;
}

There's no check that the SSV hasn't changed, nor a way to invalidate this cache outside the binding layer. Bleah.

The V8IDBAnyCustom.cpp implementation of toV8 unfortunately doesn't get passed the AccessorInfo object so there's no place for it to stash a value. We may need to do custom binding for IDBCursorWithValue

[David Grogan]

(In reply to comment #0)
> "Returns the cursor's current value. Note that if this property returns an object, it returns the same object instance every time it is inspected, until the cursor's value is changed. This means that if the object is modified, those modifications will be seen by anyone inspecting the value of the cursor. However modifying such an object does not modify the contents of the database."
> 
> The WebKit implementation does not currently support the "same object instance every time" semantics

I thought Hans added this for prefetching and that cursor-inconsistency.html tested it?

[Joshua Bell]

(In reply to comment #3)
> (In reply to comment #0)
> > "Returns the cursor's current value. Note that if this property returns an object, it returns the same object instance every time it is inspected, until the cursor's value is changed. This means that if the object is modified, those modifications will be seen by anyone inspecting the value of the cursor. However modifying such an object does not modify the contents of the database."
> > 
> > The WebKit implementation does not currently support the "same object instance every time" semantics
> 
> I thought Hans added this for prefetching and that cursor-inconsistency.html tested it?

Similar but subtly different: the same IDBCursor is indeed returned each time (i.e. request.result), but the cursor's value (e.g. request.result.value) is deserialized fresh from the SSV on each property access.

[Alec Flett]

I think I have a fix, I'll use the test here.

[Alec Flett]

Created attachment 146900 [details]
Patch

[Alec Flett]

jsbell@ / dgrogan@ - quick review?

[Joshua Bell]

Comment on attachment 146900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146900&action=review

> Source/WebCore/ChangeLog:8
> +        Cache the 'value' attribute of IDBCursorWithValue with policy

Should mention in the ChangeLog that this is explicit in the spec.

> Source/WebCore/Modules/indexeddb/IDBCursor.h:86
> +    bool valueIsDirty() { return m_valueIsDirty; }

I think this method (or the field) merits a comment, since it's not obvious why this would exist and subtle in the spec.

> Source/WebCore/Modules/indexeddb/IDBCursorWithValue.idl:31
> +        readonly attribute [V8CustomGetter] IDBAny value;

Would it simplify the code to change IDBAny here to SerializedScriptValue?

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:28
> +    LOG_ERROR(".value => is dirty? %d", imp->valueIsDirty());

Remove LOG_ERROR calls here and below.

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:37
> +    // either value is dirty, or the hidden value is empty

Can remove this comment, it's clear from the code.

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:39
> +    v8::Handle<v8::Value> wrapper = result.get() ? getDOMObjectMap(info.GetIsolate()).get(result.get()) : v8::Handle<v8::Object>();

Definitely get a V8/bindings expert to review this bit.

[Alec Flett]

Created attachment 146933 [details]
Patch

[Joshua Bell]

Comment on attachment 146933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146933&action=review

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:1
> +

Copyright header? (Some but not all of the files in bindings/v8 appear to have them, I note.)

[Alec Flett]

* I don't think it simplifies much to use SerializedScriptValue - just changes the transformation slightly, possibly making IDBCursor.cpp more complex in the process.

haraken@ - I wrote this custom binding as a sort of merge of the standard binding code and the CachedAttribute code. I think I likely have one extra layer of caching in here but I don't understand this completely.. can you take a look at IDBCustomBindings.cpp in this patch? Do I need all the goop calling getDOMObjectMap() and such?
  
(And the final patch will need to update the other build systems too, I will get to that before webkit-review)

[Alec Flett]

Created attachment 146941 [details]
Patch

[Alec Flett]

Created attachment 146942 [details]
Patch

[Kentaro Hara]

Comment on attachment 146942 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146942&action=review

At a high level question: Does the cache really improve performance?

We need to care that GetHiddenValue() and SetHiddenValue() are not so light. In case of SSV, since the serialization overhead is large, the cache management cost will pay. On the other hand, in case of 'value', I am not sure if the cache management cost pays or not. Did you confirm any performance gain in some benchmark (artificial one is OK)?

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:14
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY

Please use the correct copyright of Google.

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:41
> +#include "ContextEnabledFeatures.h"
> +#include "RuntimeEnabledFeatures.h"
> +#include "V8Binding.h"
> +#include "V8BindingState.h"
> +#include "V8DOMWrapper.h"
> +#include "V8HiddenPropertyName.h"
> +#include "V8IDBAny.h"
> +#include "V8IDBCursor.h"
> +#include "V8IsolatedContext.h"
> +#include "V8Proxy.h"
> +#include <wtf/UnusedParam.h>

It seems that unused headers are included. e.g. "ContextEnabledFeatures.h", "RuntimeEnabledFeatures.h", etc. Please check each of them if it is needed or not.

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:46
> +                                                                const v8::AccessorInfo& info)

Nit: you do not need to wrap the code.

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:65
> +    RefPtr<IDBAny> result = imp->value();
> +    v8::Handle<v8::Value> wrapper = result.get() ? getDOMObjectMap(info.GetIsolate()).get(result.get()) : v8::Handle<v8::Object>();
> +    if (wrapper.IsEmpty()) {
> +        wrapper = toV8(result.get(), info.GetIsolate());
> +        if (!wrapper.IsEmpty())
> +            V8DOMWrapper::setNamedHiddenReference(info.Holder(), "value", wrapper);
> +    }
> +    info.Holder()->SetHiddenValue(propertyName, wrapper);

getDOMObjectMap() and setNamedHiddenReference() are used for keeping the lifetime of 'wrapper' over GC, which is sometimes required by semantics. But in this case, you just want to cache the value for performance (i.e. cache miss won't violate semantics), and you do not need to care the lifetime management. So I think that this code can be:

RefPtr<IDBAny> result = imp->value();
v8::Handle<v8::Value> wrapper = toV8(result.get(), info.GetIsolate());
info.Holder()->SetHiddenValue(propertyName, wrapper);
return wrapper;

[Alec Flett]

Created attachment 146974 [details]
Patch

[Alec Flett]

Comment on attachment 146974 [details]
Patch

kentaro@ - thanks - this isn't for performance, this is for spec compliance - like you I suspect this will have a negligible effect on performance. We actually need the JS object to survive mutations during the lifetime of the attribute value. specifically imagine code like this:

cursor.value.foo = "bar";
...
if (cursor.value.foo == "bar") { ....}

cursor.advance() // changes the value of cursor.value here

if (cursor.value.foo == "bar") { ... }

New patch is up. r? cq?

[Alec Flett]

[I just realized I still need to do XCode project files.. ]

[Kentaro Hara]

Comment on attachment 146974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146974&action=review

> this isn't for performance, this is for spec compliance

I am sorry! I should have read your ChangeLog carefully...

For now let me mark r- due to missing necessary tests to confirm the change.

> Source/WebCore/ChangeLog:9
> +        determined by IDBCursor.cpp, to follow spec behavior of keeping a

Link to the spec please.

> Source/WebCore/ChangeLog:12
> +        Test: storage/indexeddb/cursor-value.html

Would you add the following test cases?

(Case 1)
cursor.value.foo = "bar";
shouldBe('cursor.value.foo', ...);
cursor.advance();
shouldBe('cursor.value.foo', ...);

(Case 2)
cursor.value.foo = "bar";
shouldBe('cursor.value.foo', ...);
gc();
shouldBe('cursor.value.foo', ...);

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:2
> + * Copyright (C) 2011, Google Inc. All rights reserved.

2011 => 2012

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY GOOGLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND

Please do not change the line manually. Please just copy the whole copyright from bindings/v8/custom/V8TrackEventCustom.cpp, and just change "2011" to "2012".

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:50
> +    IDBCursorWithValue* imp = V8IDBCursorWithValue::toNative(info.Holder());
> +    v8::Handle<v8::String> propertyName = v8::String::NewSymbol("value");
> +    if (!imp->valueIsDirty()) {
> +        v8::Handle<v8::Value> value = info.Holder()->GetHiddenValue(propertyName);
> +        if (!value.IsEmpty())
> +            return value;
> +    }
> +
> +    RefPtr<IDBAny> result = imp->value();
> +    v8::Handle<v8::Value> wrapper = toV8(result.get(), info.GetIsolate());
> +    info.Holder()->SetHiddenValue(propertyName, wrapper);
> +    return wrapper;

Confirmation: The semantics you need is:

- If the value is not yet invalidated, return the same wrapper object keeping JavaScript properties.
- If the value is already invalidated, create a new wrapper object.

Is my understanding correct? If so, the code looks correct.

[Alec Flett]

Created attachment 147113 [details]
Patch

[Alec Flett]

Comment on attachment 147113 [details]
Patch

kentaro@ - all issues addressed - cursor advancing/continuing is async, so the tests can be awkward, but I handled both cases, and rolled gc'ing into each case

(to be fair I copied that copyright notice in whole from another file, sounds like I should fix that separately)

in any case, r? cq?

[Kentaro Hara]

Comment on attachment 147113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147113&action=review

Looks OK except for nits.

> Source/WebCore/WebCore.gypi:1884
> +            'bindings/v8/IDBCustomBindings.cpp',

Is this correct? IDBCustomBindings.cpp is added twice.

> LayoutTests/storage/indexeddb/resources/cursor-value.js:56
> +            index += 2;
> +        } else if (index == 3) {

Nit: why are you using index+=2 instead of index++?

[Alec Flett]

Created attachment 147350 [details]
Patch for landing

[Alec Flett]

jsbell / dgrogan - cq?

[Alec Flett]

haraken: the += 2 was just to keep the index in sync with the actual index of the items we're iterating, since we're calling advance(2) - it's in sync mainly for test readability/maintenance

[WebKit Review Bot]

Comment on attachment 147350 [details]
Patch for landing

Clearing flags on attachment: 147350

Committed r120241: <http://trac.webkit.org/changeset/120241>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.