Bug 102552 – IndexedDB: Obtain ScriptState from IDL binding generator

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Michael Pruett]

Currently when serializing a ScriptValue in IDBObjectStore::put(), the ScriptValue is serialized using the ScriptState provided by ScriptState::current(). In JSC there is no ScriptState::current(), and it will be necessary to obtain an ExecState from DOMRequestState to serialize the ScriptValue.

[Michael Pruett]

Created attachment 174760 [details]
Patch

[Alec Flett]

Comment on attachment 174760 [details]
Patch

This looks ok to me. I'd be curious what abarth thinks. I've always found it a little odd that SerializedScriptValue::deserialize takes a ScriptState and not something better. (In fact I wonder now if there's a way we can fix SerializedScriptValue rather than fix this here)

[Michael Pruett]

Another possible approach would be to update DOMRequestState to add a method to return the ScriptState* (presumably with ScriptState::forContext()); we could then use requestState.scriptState() for both V8 and JSC and skip adding the new serializeIDBValue() function.

[Adam Barth]

Can't you just add a CallWith=ScriptState to the IDL to get the script state from the bindings layer?

[Michael Pruett]

(In reply to comment #4)
> Can't you just add a CallWith=ScriptState to the IDL to get the script state from the bindings layer?

That solution would be ideal. However it seems that adding CallWith=ScriptState to a function causes the V8 binding generator to invoke the function with an EmptyScriptState rather than with ScriptState::current(). And at the beginning of ScriptValue::serialize(), a ScriptScope is created with this EmptyScriptState. Since the EmptyScriptState has a null v8::Context, the v8::Context::Scope in ScriptScope enters the null context, triggering an assertion failure in V8.

[Adam Barth]

> However it seems that adding CallWith=ScriptState to a function causes the V8 binding generator to invoke the function with an EmptyScriptState rather than with ScriptState::current().

That sounds like a bug in the code generator that we should fix.

[Michael Pruett]

Created attachment 175501 [details]
Patch

[Michael Pruett]

Now that the V8 binding generator has been updated in <https://bugs.webkit.org/show_bug.cgi?id=102739>, I've created a new patch which obtains the ScriptState from the IDL binding generator. Creating a new serializeIDBValue() utility function is no longer necessary.

[Joshua Bell]

Comment on attachment 175501 [details]
Patch

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

IDB changes LGTM but would be nice to start off with consistent naming, i.e....

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:136
> +    return put(IDBObjectStoreBackendInterface::AddOrUpdate, IDBAny::create(this), scriptState, value, key, ec);

^^^ "scriptState" vs.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:139
> +PassRefPtr<IDBRequest> IDBObjectStore::put(IDBObjectStoreBackendInterface::PutMode putMode, PassRefPtr<IDBAny> source, ScriptState* state, ScriptValue& value, PassRefPtr<IDBKey> prpKey, ExceptionCode& ec)

^^^ "state"

[Kentaro Hara]

Comment on attachment 175501 [details]
Patch

LGTM. Please address jsbell's comments before landing.

[Michael Pruett]

Created attachment 175550 [details]
Patch

I've changed variables previously named 'scriptState' to 'state' for the sake of consistency.

[WebKit Review Bot]

Comment on attachment 175550 [details]
Patch

Clearing flags on attachment: 175550

Committed r135471: <http://trac.webkit.org/changeset/135471>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Comment on attachment 175550 [details]
Patch

Thanks for improving the code generator.  I think this patch came out very nicely this way.