[webkit-reviews] review denied: [Bug 96614] Need to clear exception in JSDictionary for operations that might have. : [Attachment 173540] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Charles Wei
<charles.wei at torchmobile.com.cn>'s request for review:
Bug 96614: Need to clear exception in JSDictionary for operations that might
have.
https://bugs.webkit.org/show_bug.cgi?id=96614

Attachment 173540: Patch
https://bugs.webkit.org/attachment.cgi?id=173540&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=173540&action=review


>>> Source/WebCore/bindings/js/JSDictionary.cpp:56
>>> +	     m_exec->clearException();
>> 
>> This is just the same change as before. This is not the correct idiom for
use of exceptions in the JavaScriptCore ExecState. What’s really going on here?
We need to get to the heart of the problem and fix that, not just clear an
exception in an ExecState in one particular binding.
> 
> Well, the root cause is crystal clear as described in comment #1, to be more
precise,  if you have 2 consecutive query to the Dictionary, with the first
failure,  which will leave the ExecState of the JSDictionary in Exception
state, that will affect the 2nd query even though the 2nd query will succeed
otherwise.
> 
> IndexedDB supports 2 types of KeyPath, String or StringArray, so it will
first try StringArry type, if fails, it will then try the String type.
> 
> Suppose the Dictionary has {keyPath, "id"},  Dictionary::get("keyPath",
Vector<String> StringArray); fails. Not because getting the attribute, but when
trying to convert the attribute to a Vector<String> at void
JSDictionary::convertValue(ExecState* exec, JSValue value, Vector<String>&
result);  and that leaves the ExecState with Exception.  This exception will be
carried over to the next query:  Dictionary::(keyPath", String value); which
should succeed but the exception carried over from last query makes it fail.
> 
> JSDictionary as an internal data structure, should not affect the ExecState
by leaving it in exception state which will also affect the Javascript run
later.	So clear the exception is necessary.  If you want to get the heart of
the problem and fix that, I believe we need to move the implementation of
Dictionary/Dictionary away from JavaScript JSValue based , which is a big task
and not the scope of this PR.
> 
> Based on above explanation, I hope it's clear that this patch is necessary. 
Currently the problem only shows up for IndexedDB because IndexedDB code is
trying to query the Dictionary for different types, other code doesn't show the
problem because they are not using Dictionary the same way as IndexedDB,  but
if they do later, they will have the same problem.
> 
> I am raising the review flag again.

This still makes no sense to me.  If we had an exception, the right thing is
almost always to propagate it up to the JS environment.  Can you show me a
reduced test case where this is necessary.