Bug 113091 - [V8] IndexedDB: Exceptions thrown inconsistently for non-cloneable values
Summary: [V8] IndexedDB: Exceptions thrown inconsistently for non-cloneable values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks: 113689
  Show dependency treegraph
 
Reported: 2013-03-22 13:25 PDT by Joshua Bell
Modified: 2013-04-01 12:37 PDT (History)
6 users (show)

See Also:


Attachments
Repro case (458 bytes, text/html)
2013-03-22 13:25 PDT, Joshua Bell
no flags Details
Second repro case (292 bytes, text/html)
2013-03-22 13:26 PDT, Joshua Bell
no flags Details
layout test (1.04 KB, text/html)
2013-03-22 14:37 PDT, Joshua Bell
no flags Details
Patch (5.48 KB, patch)
2013-03-22 15:53 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (5.77 KB, patch)
2013-03-29 11:52 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2013-03-22 13:25:13 PDT
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!
Comment 1 Joshua Bell 2013-03-22 13:26:55 PDT
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!
Comment 2 Joshua Bell 2013-03-22 14:01:10 PDT
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.
Comment 3 Joshua Bell 2013-03-22 14:37:13 PDT
Created attachment 194633 [details]
layout test

A layout test version of the "first exception not thrown". Assumes running from LayoutTests/storage/indexeddb
Comment 4 Joshua Bell 2013-03-22 15:19:11 PDT
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()) {
Comment 5 Joshua Bell 2013-03-22 15:27:18 PDT
(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().
Comment 6 Joshua Bell 2013-03-22 15:39:06 PDT
Second case spews this in a debug build:

#
# Fatal error in ../../v8/src/execution.cc, line 215
# CHECK(isolate->external_caught_exception()) failed
#
Comment 7 Joshua Bell 2013-03-22 15:53:07 PDT
Created attachment 194648 [details]
Patch
Comment 8 Joshua Bell 2013-03-22 15:55:11 PDT
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.
Comment 9 Kentaro Hara 2013-03-22 16:02:44 PDT
(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.
Comment 10 Joshua Bell 2013-03-22 16:21:05 PDT
(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.
Comment 11 Kentaro Hara 2013-03-23 00:48:17 PDT
(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.
Comment 12 Joshua Bell 2013-03-26 13:19:06 PDT
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.
Comment 13 Joshua Bell 2013-03-29 11:52:36 PDT
Created attachment 195771 [details]
Patch
Comment 14 Joshua Bell 2013-03-29 11:53:05 PDT
haraken@ - I added a FIXME, but I think we should land this now and tackle SSV/ScriptState refactoring separately.
Comment 15 Joshua Bell 2013-03-29 11:53:35 PDT
(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?
Comment 16 Kentaro Hara 2013-03-29 12:10:45 PDT
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.
Comment 17 WebKit Review Bot 2013-03-29 12:34:55 PDT
Comment on attachment 195771 [details]
Patch

Clearing flags on attachment: 195771

Committed r147241: <http://trac.webkit.org/changeset/147241>
Comment 18 WebKit Review Bot 2013-03-29 12:34:59 PDT
All reviewed patches have been landed.  Closing bug.