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!
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!
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.
Created attachment 194633 [details] layout test A layout test version of the "first exception not thrown". Assumes running from LayoutTests/storage/indexeddb
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()) {
(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().
Second case spews this in a debug build: # # Fatal error in ../../v8/src/execution.cc, line 215 # CHECK(isolate->external_caught_exception()) failed #
Created attachment 194648 [details] Patch
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.
(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.
(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.
(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.
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.
Created attachment 195771 [details] Patch
haraken@ - I added a FIXME, but I think we should land this now and tackle SSV/ScriptState refactoring separately.
(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 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 on attachment 195771 [details] Patch Clearing flags on attachment: 195771 Committed r147241: <http://trac.webkit.org/changeset/147241>
All reviewed patches have been landed. Closing bug.