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

[bugzilla-daemon at webkit.org]

Charles Wei <charles.wei at torchmobile.com.cn> has asked	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 Charles Wei <charles.wei at torchmobile.com.cn>
View in context: https://bugs.webkit.org/attachment.cgi?id=173540&action=review


With the help of a colleague who happened to have a QT environment and helped
to verify the patch, no QT regression with this patch.

>> 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.