Bug 113091 – [V8] IndexedDB: Exceptions thrown inconsistently for non-cloneable values

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

[Joshua Bell]

Created attachment 194623 [details]
Repro case

Attached repro "clone_no_exception.html" does the following twice:

* Creates a new database and object store
* Attempts to put a non-cloneable value into it

Watch the console for output. For some reason, the first attempt doesn't throw!

[Joshua Bell]

Created attachment 194624 [details]
Second repro case

For more fun, try the attachment "clone_double_throw.html". Again, watch the console.

This tries to add with both an invalid key (DataError) and an invalid value (DataCloneError) in the same call. The DataError is thrown and caught as expected.

Unexpectedly, the *console* reports an uncaught DataCloneError. It's like the call is generating two exceptions simultaneously!

[Joshua Bell]

I should clarify that the above steps repro in Chrome. I'm assuming there's a bug in the bindings code somewhere and/or the IDB usage of it.

[Joshua Bell]

Created attachment 194633 [details]
layout test

A layout test version of the "first exception not thrown". Assumes running from LayoutTests/storage/indexeddb

[Joshua Bell]

Applying this patch to IDBObjectStore resolves the first issue, but it seems like it should not be necessary.

-    RefPtr<SerializedScriptValue> serializedValue = value.serialize(state);
-    if (state->hadException()) {
+    bool didThrow = false;
+    RefPtr<SerializedScriptValue> serializedValue = value.serialize(state, 0, 0, didThrow);
+    if (didThrow || state->hadException()) {

[Joshua Bell]

(In reply to comment #4)
> Applying this patch to IDBObjectStore resolves the first issue, but it seems like it should not be necessary.
> 
> -    RefPtr<SerializedScriptValue> serializedValue = value.serialize(state);
> -    if (state->hadException()) {
> +    bool didThrow = false;
> +    RefPtr<SerializedScriptValue> serializedValue = value.serialize(state, 0, 0, didThrow);
> +    if (didThrow || state->hadException()) {

Actually, prior to wkrev.com/128379 the conversion to SerializedScriptValue took place in the generated binding code rather than via ScriptValue. That code only looked at the "didThrow" out param from serialize().

[Joshua Bell]

Second case spews this in a debug build:

#
# Fatal error in ../../v8/src/execution.cc, line 215
# CHECK(isolate->external_caught_exception()) failed
#

[Joshua Bell]

Created attachment 194648 [details]
Patch

[Joshua Bell]

Okay, here's a correct patch.

It looks uglier and less intuitive than the previous code, which means (unsurprisingly) that the SSV API isn't as clear as it should be.

[Kentaro Hara]

(In reply to comment #8)
> Okay, here's a correct patch.
> 
> It looks uglier and less intuitive than the previous code, which means (unsurprisingly) that the SSV API isn't as clear as it should be.

The fact that we have both didThrow and state->hadException() is confusing. Would it be hard to fix the confusing API now? If it's hard, I'm willing to go with your current patch as an immediate fix.

[Joshua Bell]

(In reply to comment #9)
> (In reply to comment #8)
> > Okay, here's a correct patch.
> > 
> > It looks uglier and less intuitive than the previous code, which means (unsurprisingly) that the SSV API isn't as clear as it should be.
> 
> The fact that we have both didThrow and state->hadException() is confusing. Would it be hard to fix the confusing API now? If it's hard, I'm willing to go with your current patch as an immediate fix.

I was hoping you could tell me. :) I'll dig in a bit more. 

V8ThrowException::setDOMException() (called when didThrow is set to true in SSV) is actually calling into v8::ThrowException, whereas state->hadException() is merely keeping an exception around so it can later be thrown by the generated binding code in a handful of cases.

So... one possibility for unifying them is to pass a ScriptState into SerializedScriptValue and ensure it only registers an exception there. (That would require reworking V8ThrowException since that's where the DOMException mapping happens.) That seems... non-trivial.

Another is making ScriptState more general so it's notion of "was there an exception?" handles both pending exceptions (like it has now) and exceptions already thrown w/in V8. The latter seems like it would require getting access to a wrapping tryCatch block and rethrowing on exit. Also a fair bit of work.

Any advice? I think I'd need to better understand the intent of ScriptState to know how to proceed.

[Kentaro Hara]

(In reply to comment #10)
> Another is making ScriptState more general so it's notion of "was there an exception?" handles both pending exceptions (like it has now) and exceptions already thrown w/in V8. The latter seems like it would require getting access to a wrapping tryCatch block and rethrowing on exit. Also a fair bit of work.

This approach sounds good to me (although I've not studied how much work will be needed to accomplish the work).


> Any advice? I think I'd need to better understand the intent of ScriptState to know how to proceed.

Ideally, we should have ScriptState piggyback all information about a script state, including an exception thrown inside WebKit. Thus, making ScriptState more general sounds like a reasonable approach to me.

[Joshua Bell]

I'm probably not going to get to a refactor any time soon. It looks like ScriptState's use is quite limited at the moment and I don't see ways for it to introspect the engine state without making quite intrusive changes into the generated binding code; passing it in to deserialize() is probably less impactful, but still quite a big change to SSV.

[Joshua Bell]

Created attachment 195771 [details]
Patch

[Joshua Bell]

haraken@ - I added a FIXME, but I think we should land this now and tackle SSV/ScriptState refactoring separately.

[Joshua Bell]

(In reply to comment #14)
> haraken@ - I added a FIXME, but I think we should land this now and tackle SSV/ScriptState refactoring separately.

Oh, and: r?

[Kentaro Hara]

Comment on attachment 195771 [details]
Patch

> I added a FIXME, but I think we should land this now and tackle SSV/ScriptState refactoring separately.

Sounds reasonable to me. Let's fix a bug first.

[WebKit Review Bot]

Comment on attachment 195771 [details]
Patch

Clearing flags on attachment: 195771

Committed r147241: <http://trac.webkit.org/changeset/147241>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.