Bug 96614 – Need to clear exception in JSDictionary for operations that might have.

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Charles Wei]

https://bugs.webkit.org/show_bug.cgi?id=84207 introduced array-type keyPath support.  So when creating a indexeddb, the IDBDatabase::createObjectStore will first try if the keyPath is an array, if not, it then tries it as a JSValue of String type.

Code below:

@@ -95,18 +95,26 @@ PassRefPtr<IDBObjectStore> IDBDatabase::createObjectStore(const String& name, co
     IDBKeyPath keyPath;
     if (!options.isUndefinedOrNull()) {
         String keyPathString;
-        if (options.getWithUndefinedOrNullCheck("keyPath", keyPathString))
+        Vector<String> keyPathArray;
+        if (options.get("keyPath", keyPathArray))
+            keyPath = IDBKeyPath(keyPathArray);
+        else if (options.getWithUndefinedOrNullCheck("keyPath", keyPathString))


But if the Javascript tries to create the IDB with a non-array type key type below:

          db.createObjectStore("people", {keyPath: "id"});

the first try as a keyPathArray in above native code  will fail and leaves an exception in current ExecState,  and that will be carried over to the following check as a keyPathString with "else if (options.getWithUndefinedOrNullCheck("keyPath", keyPathString))",  in which it will check if there's any exception in current ExecState, and fail if there is exception.  The exception carried over will fail it.

So we need to clear any exception in getWithUndefinedOrNullCheck() to make sure no exception is carried over.

[Charles Wei]

Created attachment 163809 [details]
Patch

[Yong Li]

This patch does not only affect blackberry port, does it?

[Yong Li]

Comment on attachment 163809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163809&action=review

> Source/WebCore/ChangeLog:16
> +        IndexedDB now supports both array-type keyPath and string type keyPath,
> +        it will first try array type, if that fails, it will try the string type
> +        by calling JSDictionary::getWithUndefinedOrNullCheck, but the fail as 
> +        array type will generate an execption in ExecState and will carried over.
> +        We need to clear the exception to avoid the impact to the check as a string.
> +

I'm not an expert here. But according to this description, it should be IDBDatabase's responsibility to clear exception. I see webkit usually clears exception immediately after calling into JSC, rather than before.

[Kentaro Hara]

Comment on attachment 163809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163809&action=review

>> Source/WebCore/ChangeLog:16
>> +
> 
> I'm not an expert here. But according to this description, it should be IDBDatabase's responsibility to clear exception. I see webkit usually clears exception immediately after calling into JSC, rather than before.

Yong is right. Would you update the patch?

[Charles Wei]

(In reply to comment #3)
> (From update of attachment 163809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163809&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        IndexedDB now supports both array-type keyPath and string type keyPath,
> > +        it will first try array type, if that fails, it will try the string type
> > +        by calling JSDictionary::getWithUndefinedOrNullCheck, but the fail as 
> > +        array type will generate an execption in ExecState and will carried over.
> > +        We need to clear the exception to avoid the impact to the check as a string.
> > +
> 
> I'm not an expert here. But according to this description, it should be IDBDatabase's responsibility to clear exception. I see webkit usually clears exception immediately after calling into JSC, rather than before.

Thanks, Yong, for the review, and the suggestion.

No, it couldn't be the IDBDatabase's responsibility,  the IDBDatabase doesn't know how the Dictionary is implemented, and doesn't know that Dictionary is actually storing the original ExecState and JSValue inside. 

The Dictionary (and internal JSDictionary) is responsible to clear the exception.  The Dictionary is an internal data and should not contaminate the ExecState by leaving an exception there.

I moved the clearException() right after where it happens in the new patch, following your suggestions,  but still (and have to be) inside JSDictionary.

[Charles Wei]

Created attachment 164065 [details]
Patch

[Kentaro Hara]

Comment on attachment 164065 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164065&action=review

OK.

> Source/WebCore/bindings/js/JSDictionary.h:150
> +            m_exec->clearException();
>              return ExceptionThrown;

At first I thought that this will make it impossible to retrieve the exception later. But the fact that we return 'ExceptionThrown' means that we're no longer interested in what exception was thrown. In that sense, clearing the exception here would make sense.

[Charles Wei]

Comment on attachment 164065 [details]
Patch

Thanks, Kentaro.

[WebKit Review Bot]

Comment on attachment 164065 [details]
Patch

Clearing flags on attachment: 164065

Committed r128568: <http://trac.webkit.org/changeset/128568>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 96739

[Csaba Osztrogonác]

(In reply to comment #11)
> Re-opened since this is blocked by 96739

Rolled out by https://trac.webkit.org/changeset/128574 , because it broke
3 tests everywhere (at leaset on GTK, Qt, EFL)

- fast/dom/Geolocation/argument-types.html started to crash:
-------------------------------------------------------------
crash log for DumpRenderTree (pid 17744):
STDOUT: <empty>
STDERR: 1   0xf6a0dbbc /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/WebKitBuild/Release/lib/libQtWebKit.so.5(+0x1786bbc) [0xf6a0dbbc]
STDERR: 2   0xf77e0400 [0xf77e0400]
...

- fast/events/constructors/message-event-constructor.html started to fail:
---------------------------------------------------------------------------
--- /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/events/constructors/message-event-constructor-expected.txt 
+++ /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/events/constructors/message-event-constructor-actual.txt 
@@ -80,19 +80,19 @@
 PASS new MessageEvent('eventType', { ports: [] }).ports is []
 PASS new MessageEvent('eventType', { ports: undefined }).ports is []
 PASS new MessageEvent('eventType', { ports: null }).ports is []
-PASS new MessageEvent('eventType', { ports: [1, 2, 3] }).ports[2] threw exception TypeError: Type error.
-PASS new MessageEvent('eventType', { ports: test_object }).ports threw exception TypeError: Type error.
-PASS new MessageEvent('eventType', { ports: document }).ports threw exception TypeError: Type error.
-PASS new MessageEvent('eventType', { ports: false }).ports threw exception TypeError: Type error.
-PASS new MessageEvent('eventType', { ports: true }).ports threw exception TypeError: Type error.
-PASS new MessageEvent('eventType', { ports: '' }).ports threw exception TypeError: Type error.
-PASS new MessageEvent('eventType', { ports: 'chocolate' }).ports threw exception TypeError: Type error.
-PASS new MessageEvent('eventType', { ports: 12345 }).ports threw exception TypeError: Type error.
-PASS new MessageEvent('eventType', { ports: 18446744073709551615 }).ports threw exception TypeError: Type error.
-PASS new MessageEvent('eventType', { ports: NaN }).ports threw exception TypeError: Type error.
-PASS new MessageEvent('eventType', { get ports() { return 123; } }).ports threw exception TypeError: Type error.
+PASS new MessageEvent('eventType', { ports: [1, 2, 3] }).ports[2] threw exception TypeError: 'undefined' is not an object (evaluating 'new MessageEvent('eventType', { ports: [1, 2, 3] }).ports').
+PASS new MessageEvent('eventType', { ports: test_object }).ports threw exception TypeError: 'undefined' is not an object (evaluating 'new MessageEvent('eventType', { ports: test_object }).ports').
+PASS new MessageEvent('eventType', { ports: document }).ports threw exception TypeError: 'undefined' is not an object (evaluating 'new MessageEvent('eventType', { ports: document }).ports').
+PASS new MessageEvent('eventType', { ports: false }).ports threw exception TypeError: 'undefined' is not an object (evaluating 'new MessageEvent('eventType', { ports: false }).ports').
+PASS new MessageEvent('eventType', { ports: true }).ports threw exception TypeError: 'undefined' is not an object (evaluating 'new MessageEvent('eventType', { ports: true }).ports').
+PASS new MessageEvent('eventType', { ports: '' }).ports threw exception TypeError: 'undefined' is not an object (evaluating 'new MessageEvent('eventType', { ports: '' }).ports').
+PASS new MessageEvent('eventType', { ports: 'chocolate' }).ports threw exception TypeError: 'undefined' is not an object (evaluating 'new MessageEvent('eventType', { ports: 'chocolate' }).ports').
+PASS new MessageEvent('eventType', { ports: 12345 }).ports threw exception TypeError: 'undefined' is not an object (evaluating 'new MessageEvent('eventType', { ports: 12345 }).ports').
+PASS new MessageEvent('eventType', { ports: 18446744073709551615 }).ports threw exception TypeError: 'undefined' is not an object (evaluating 'new MessageEvent('eventType', { ports: 18446744073709551615 }).ports').
+PASS new MessageEvent('eventType', { ports: NaN }).ports threw exception TypeError: 'undefined' is not an object (evaluating 'new MessageEvent('eventType', { ports: NaN }).ports').
+PASS new MessageEvent('eventType', { get ports() { return 123; } }).ports threw exception TypeError: 'undefined' is not an object (evaluating 'new MessageEvent('eventType', { get ports() { return 123; } }).ports').
 PASS new MessageEvent('eventType', { get ports() { throw 'MessageEvent Error'; } }) threw exception MessageEvent Error.
-PASS new MessageEvent('eventType', { ports: {valueOf: function () { return [channel.port1, channel.port2, channel.port2]; } } }).ports[0] threw exception TypeError: Type error.
+PASS new MessageEvent('eventType', { ports: {valueOf: function () { return [channel.port1, channel.port2, channel.port2]; } } }).ports[0] threw exception TypeError: 'undefined' is not an object (evaluating 'new MessageEvent('eventType', { ports: {valueOf: function () { return [channel.port1, channel.port2, channel.port2]; } } }).ports').
 PASS new MessageEvent('eventType', { bubbles: true, cancelable: true, data: test_object, origin: 'wonderful', lastEventId: 'excellent', source: window, ports: [channel.port1, channel.port2, channel2.port1] }).bubbles is true
 PASS new MessageEvent('eventType', { bubbles: true, cancelable: true, data: test_object, origin: 'wonderful', lastEventId: 'excellent', source: window, ports: [channel.port1, channel.port2, channel2.port1] }).cancelable is true
 PASS new MessageEvent('eventType', { bubbles: true, cancelable: true, data: test_object, origin: 'wonderful', lastEventId: 'excellent', source: window, ports: [channel.port1, channel.port2, channel2.port1] }).data is test_object


- fast/files/blob-constructor.html started to fail:
----------------------------------------------------
--- /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/files/blob-constructor-expected.txt 
+++ /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/files/blob-constructor-actual.txt 
@@ -25,11 +25,11 @@
 PASS new Blob([throwingObj]) threw exception Error.
 PASS (new Blob([], {unknownKey:'value'})) instanceof window.Blob is true
 PASS new Blob([], {endings:'illegalValue'}) threw exception TypeError: The endings property must be either "transparent" or "native".
-PASS new Blob([], {endings:throwingObj}) threw exception Error.
-PASS new Blob([], {type:throwingObj}) threw exception Error.
+FAIL new Blob([], {endings:throwingObj}) should throw Error. Was [object Blob].
+FAIL new Blob([], {type:throwingObj}) should throw Error. Was [object Blob].
 PASS new Blob([], {type:'helloĂŽ'}) threw exception SyntaxError: type must consist of ASCII characters.
-PASS new Blob([], {endings:throwingObj1, type:throwingObj2}) threw exception Error 1.
-PASS new Blob([], {type:throwingObj2, endings:throwingObj1}) threw exception Error 1.
+FAIL new Blob([], {endings:throwingObj1, type:throwingObj2}) should throw Error 1. Was [object Blob].
+FAIL new Blob([], {type:throwingObj2, endings:throwingObj1}) should throw Error 1. Was [object Blob].
 PASS new Blob([], {type:throwingObj2, endings:'illegal'}) threw exception TypeError: The endings property must be either "transparent" or "native".
 PASS (new Blob([], null)) instanceof window.Blob threw exception TypeError: Second argument of the constructor is not of type Object.
 PASS (new Blob([], undefined)) instanceof window.Blob threw exception TypeError: Second argument of the constructor is not of type Object.

[Charles Wei]

hmmm, will look into the test cases broken.

[Charles Wei]

Created attachment 173253 [details]
Patch

[WebKit Review Bot]

Comment on attachment 173253 [details]
Patch

Attachment 173253 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14777385

[Charles Wei]

Created attachment 173261 [details]
Patch

[Charles Wei]

Comment on attachment 173261 [details]
Patch

Tried to introduce an API Dictionary::clearException() with last patch so that the IndexedDB can call that to clear the exceptions between 2 calls of Dictionary::get() and Dictionary::getWithUndefinedOrNullCheck(), that doesn't seem good because 1) Needs to have both API for both V8 and JSC porting, even though V8 porting doesn't need that at all; 2) exposes the implementation details of Dictionary to external world; 3) violates the const syntax of the dictionary object.

So I decided to clear the exception inside JSDictionary::getWithUndefinedOrNullCheck(), which is affected by last query failure of JSDictionary::get().   JSDictionary::getWithUndefinedOrNullCheck() is only referenced by indexeddb, and the problem only shows for indexeddb, so we clear the exception in it to minimize the impact to other parts.

[Kentaro Hara]

How about calling clearException() at the head of JSDictionary::tryGetProperty()? What we really need to do is to clear exceptions before JSDictionary::tryGetProperty() starts checking exceptions, doesn't it?

[Charles Wei]

(In reply to comment #18)
> How about calling clearException() at the head of JSDictionary::tryGetProperty()? What we really need to do is to clear exceptions before JSDictionary::tryGetProperty() starts checking exceptions, doesn't it?

That seems breaks some test cases of QT and EFL porting, which seems are expecting the exceptions.   I don't have an environment to test these portings.  This patch has minimal impact and won't break anything while works for our JSC binding for IndexedDB.

[Kentaro Hara]

(In reply to comment #19)
> (In reply to comment #18)
> > How about calling clearException() at the head of JSDictionary::tryGetProperty()? What we really need to do is to clear exceptions before JSDictionary::tryGetProperty() starts checking exceptions, doesn't it?
> 
> That seems breaks some test cases of QT and EFL porting, which seems are expecting the exceptions.   I don't have an environment to test these portings.  This patch has minimal impact and won't break anything while works for our JSC binding for IndexedDB.

It would have minimal impact, but it looks like you're making a wrong change.

> That seems breaks some test cases of QT and EFL porting, which seems are expecting the exceptions.

I would guess you need to figure out the cause and update the test results if needed. It doesn't make sense to me not to clear exceptions at the head of tryGetProperty().

[Charles Wei]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > How about calling clearException() at the head of JSDictionary::tryGetProperty()? What we really need to do is to clear exceptions before JSDictionary::tryGetProperty() starts checking exceptions, doesn't it?
> > 
> > That seems breaks some test cases of QT and EFL porting, which seems are expecting the exceptions.   I don't have an environment to test these portings.  This patch has minimal impact and won't break anything while works for our JSC binding for IndexedDB.
> 
> It would have minimal impact, but it looks like you're making a wrong change.
> 

I don't agree that it's wrong, it might not address all the problems , but I don't think it's wrong.

> > That seems breaks some test cases of QT and EFL porting, which seems are expecting the exceptions.
> 
> I would guess you need to figure out the cause and update the test results if needed. It doesn't make sense to me not to clear exceptions at the head of tryGetProperty().

[George Staikos]

Comment on attachment 173261 [details]
Patch

Er - no reviewed by line?  I don't think you can use commit queue then?

[Kentaro Hara]

(In reply to comment #21)
> I don't agree that it's wrong, it might not address all the problems , but I don't think it's wrong.

I don't think the change itself is wrong but the change is inappropriate in that the change hides a potential bug. For example, it doesn't make sense to add clearException() to JSDictionary::getWithUndefinedOrNullCheck() but not add JSDictionary::get(). You don't want to add it just because it is not necessary for your IDB change and it will break other port tests for some reason --- which wouldn't be a right way to fix the problem.

[Charles Wei]

(In reply to comment #23)
> (In reply to comment #21)
> > I don't agree that it's wrong, it might not address all the problems , but I don't think it's wrong.
> 
> I don't think the change itself is wrong but the change is inappropriate in that the change hides a potential bug. For example, it doesn't make sense to add clearException() to JSDictionary::getWithUndefinedOrNullCheck() but not add JSDictionary::get(). You don't want to add it just because it is not necessary for your IDB change and it will break other port tests for some reason --- which wouldn't be a right way to fix the problem.

You can't expect me to solve all the problems even for those that I don't have an environment to test and verify :-)

[Kentaro Hara]

(In reply to comment #24)
> > I don't think the change itself is wrong but the change is inappropriate in that the change hides a potential bug. For example, it doesn't make sense to add clearException() to JSDictionary::getWithUndefinedOrNullCheck() but not add JSDictionary::get(). You don't want to add it just because it is not necessary for your IDB change and it will break other port tests for some reason --- which wouldn't be a right way to fix the problem.
> 
> You can't expect me to solve all the problems even for those that I don't have an environment to test and verify :-)

How much is it critical to make the fix? If you are in a hurry, you can land the patch as is and should ask Qt/EFL folks for help after that. Otherwise, let's ask them for help first. Either way I think you should fix the problem at some point.

(BTW, building the Qt port would not be so difficult if you have Linux.)

[Charles Wei]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #21)
> > > I don't agree that it's wrong, it might not address all the problems , but I don't think it's wrong.
> > 
> > I don't think the change itself is wrong but the change is inappropriate in that the change hides a potential bug. For example, it doesn't make sense to add clearException() to JSDictionary::getWithUndefinedOrNullCheck() but not add JSDictionary::get(). You don't want to add it just because it is not necessary for your IDB change and it will break other port tests for some reason --- which wouldn't be a right way to fix the problem.
> 
> You can't expect me to solve all the problems even for those that I don't have an environment to test and verify :-)

Actually I kind of agree with you ,  Dictionary should clear exception before each new query,  because otherwise, the last query will be carried over to the next query. 

But seems some portings do expect this exception with their test cases, which is wrong.    I don't want to submit a patch that will be reverted just because it breaks some test cases that I don't have access to and can't verify because I don't have corresponding porting environment.  So I would like to submit a patch that won't break them while works for us also.

[Charles Wei]

(In reply to comment #25)
> (In reply to comment #24)
> > > I don't think the change itself is wrong but the change is inappropriate in that the change hides a potential bug. For example, it doesn't make sense to add clearException() to JSDictionary::getWithUndefinedOrNullCheck() but not add JSDictionary::get(). You don't want to add it just because it is not necessary for your IDB change and it will break other port tests for some reason --- which wouldn't be a right way to fix the problem.
> > 
> > You can't expect me to solve all the problems even for those that I don't have an environment to test and verify :-)
> 
> How much is it critical to make the fix? If you are in a hurry, you can land the patch as is and should ask Qt/EFL folks for help after that. Otherwise, let's ask them for help first. Either way I think you should fix the problem at some point.
> 
> (BTW, building the Qt port would not be so difficult if you have Linux.)

The fix is critical for our product,  and I also tried to setup a QT environement. But because of the poor network environement, I already spent 2 days to grab the QT source code to build, still didn't finish yet and it fails many times and I have to start over again everytime (poor network in my office and home in Beijing)

[Kentaro Hara]

(In reply to comment #27)
> The fix is critical for our product,  and I also tried to setup a QT environement. But because of the poor network environement, I already spent 2 days to grab the QT source code to build, still didn't finish yet and it fails many times and I have to start over again everytime (poor network in my office and home in Beijing)

Then I don't object to landing the patch as is, although the change is inappropriate. But please do fix the problem soon anyway. If it is difficult for you to fix Qt/EFL problems by yourself, you should ask Qt/EFL experts for help (here or in IRC).

[Charles Wei]

(In reply to comment #28)
> (In reply to comment #27)
> > The fix is critical for our product,  and I also tried to setup a QT environement. But because of the poor network environement, I already spent 2 days to grab the QT source code to build, still didn't finish yet and it fails many times and I have to start over again everytime (poor network in my office and home in Beijing)
> 
> Then I don't object to landing the patch as is, although the change is inappropriate. But please do fix the problem soon anyway. If it is difficult for you to fix Qt/EFL problems by yourself, you should ask Qt/EFL experts for help (here or in IRC).

Thanks for your understanding,  will talk to Qt/EFL guys later with a more appropritate fix that won't break their test case (or they have to fix their test case).

[Kentaro Hara]

Comment on attachment 173261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173261&action=review

> Source/WebCore/bindings/js/JSDictionary.cpp:220
> +    if (m_exec->hadException()) // Clear exceptions that might have been caused by previous query.

Please add a comment about the current situation (and fix it sooner or later):

// FIXME: This check should be moved to the head of JSDictionary::tryGetProperty().
// Currently we cannot move the check there because it breaks some Qt/EFL tests for some reason.

[Kentaro Hara]

I'm leaving today. Feel free to cq+ it after adding the FIXME.

[Charles Wei]

Created attachment 173304 [details]
Patch

[Charles Wei]

Created attachment 173306 [details]
Patch

[Charles Wei]

Comment on attachment 173306 [details]
Patch

Commit it after review by George and Kentaro Hara.

[WebKit Review Bot]

Comment on attachment 173306 [details]
Patch

Clearing flags on attachment: 173306

Committed r134069: <http://trac.webkit.org/changeset/134069>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

This change looks completely wrong to me. When can we do a real fix?

[Sam Weinig]

(In reply to comment #37)
> This change looks completely wrong to me. When can we do a real fix?

I agree.  Changes like this should not be landed.

[Geoffrey Garen]

Kentaro, it is your responsibility as a reviewer to maintain a high standard of coding quality in the WebKit project. You have failed in that responsibility here.

[Kentaro Hara]

Let me roll it out in hours.

[WebKit Review Bot]

Re-opened since this is blocked by bug 101852

[George Staikos]

(In reply to comment #30)
> (From update of attachment 173261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173261&action=review
> 
> > Source/WebCore/bindings/js/JSDictionary.cpp:220
> > +    if (m_exec->hadException()) // Clear exceptions that might have been caused by previous query.
> 
> Please add a comment about the current situation (and fix it sooner or later):
> 
> // FIXME: This check should be moved to the head of JSDictionary::tryGetProperty().
> // Currently we cannot move the check there because it breaks some Qt/EFL tests for some reason.

So why don't we just break Qt/EFL tests if their tests are wrong and they are not fixing it?  There's a boatload of time being wasted here for no net gain to anyone.

Simon can someone fix Qt?

[Charles Wei]

(In reply to comment #42)
> (In reply to comment #30)
> > (From update of attachment 173261 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=173261&action=review
> > 
> > > Source/WebCore/bindings/js/JSDictionary.cpp:220
> > > +    if (m_exec->hadException()) // Clear exceptions that might have been caused by previous query.
> > 
> > Please add a comment about the current situation (and fix it sooner or later):
> > 
> > // FIXME: This check should be moved to the head of JSDictionary::tryGetProperty().
> > // Currently we cannot move the check there because it breaks some Qt/EFL tests for some reason.
> 
> So why don't we just break Qt/EFL tests if their tests are wrong and they are not fixing it?  There's a boatload of time being wasted here for no net gain to anyone.
> 
> Simon can someone fix Qt?

Ok, let me have a real fix which might break Qt/EFL. If it does, Qt/EFL should fix their test case.

[Kentaro Hara]

(In reply to comment #43)
> > Simon can someone fix Qt?
> 
> Ok, let me have a real fix which might break Qt/EFL. If it does, Qt/EFL should fix their test case.

I would recommend you should contact Qt/EFL folks first and investigate the problem. It is not a good idea to update test cases just because "my change would be correct in theory" before understanding the problem.

[Charles Wei]

Created attachment 173540 [details]
Patch

[Charles Wei]

Comment on attachment 173540 [details]
Patch

I am now contacting Qt/EFL guys to ask for their help to verify this patch to make sure it doesn't break their test cases.

[Charles Wei]

Comment on attachment 173540 [details]
Patch

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.

[Kentaro Hara]

(In reply to comment #47)
> (From update of attachment 173540 [details])
> 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.

What was the cause of the Qt failure we observed before?

[Charles Wei]

(In reply to comment #48)
> (In reply to comment #47)
> > (From update of attachment 173540 [details] [details])
> > 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.
> 
> What was the cause of the Qt failure we observed before?

Don't know . Look at comment #12.

[Darin Adler]

Comment on attachment 173540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173540&action=review

> Source/WebCore/bindings/js/JSDictionary.cpp:56
> +    if (m_exec->hadException()) // Clear exceptions, if any, left by last query.
> +        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.

[Charles Wei]

Comment on attachment 173540 [details]
Patch

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.

[Joshua Bell]

*** Bug 92828 has been marked as a duplicate of this bug. ***

[Charles Wei]

Any comments to this ?

[Charles Wei]

Any comments to this ?(In reply to comment #52)
> *** Bug 92828 has been marked as a duplicate of this bug. ***

I was not aware this is already reported with 92828.

[Jiyeon Kim]

(In reply to comment #54)
> Any comments to this ?(In reply to comment #52)
> > *** Bug 92828 has been marked as a duplicate of this bug. ***
> 
> I was not aware this is already reported with 92828.
It's ok. I think your patch is what I want. 
I try to enable indexed database for efl port. And then, I found this issue also.

[Yong Li]

It seems JSDictionary::tryGetProperty is also used by JSGeolocationCustom. I still think it should be IDBDatabase's responsibility to clear the exception or tell getWithUndefinedOrNullCheck to clear it. How about adding a new argument "bool clearException" to getWithUndefinedOrNullCheck?

[Joshua Bell]

(In reply to comment #56)
> It seems JSDictionary::tryGetProperty is also used by JSGeolocationCustom. I still think it should be IDBDatabase's responsibility to clear the exception or tell getWithUndefinedOrNullCheck to clear it. How about adding a new argument "bool clearException" to getWithUndefinedOrNullCheck?

We want Dictionary to behave the same between v8 and js implementations. The v8 implementation never sets a persistent exception state; the js implementation does, and that's the root of the problem. Hiding the JS behavior behind a flag won't solve the problem unless similar behavior is added on the v8 side.

Setting a persistent exception state from behind WebCore code seems strange and fragile to me - the code using it can't determine if should terminate or continue processing. I'd much prefer having the Dictionary methods return an ExceptionCode which could be ignored or thrown.

It looks like several methods have been added to the both implementations to return errors (e.g. MediaKeyError). These look like type-specific errors (which is strange...), but I think this is a better model. The caller can decide how to proceed.

...

Getting back to the original suggestion:

If JS-specific code is relying on this behavior (i.e. JSGeolocationCustom) then it should be a JS-specific API. So perhaps the getWithUndefinedOrNullCheck() should clear the exception by default, and there's an optional raiseException argument only in the JS version that JSGeolocationCustom calls?

[Charles Wei]

(In reply to comment #56)
> It seems JSDictionary::tryGetProperty is also used by JSGeolocationCustom. I still think it should be IDBDatabase's responsibility to clear the exception or tell getWithUndefinedOrNullCheck to clear it. How about adding a new argument "bool clearException" to getWithUndefinedOrNullCheck?

As pointed out in comment #57 by Joshua,  this is a JSC porting specific problem, V8 doesn't have the same problem (leaving the ExecState of JSDictionary in exception state).  If we add a new argument, we are changing the API  for both JSC binding and V8 binding, and it makes no sense to V8 binding at all.  While it's OK for me, but I don't think it will get approved for V8 binding.

[Yong Li]

(In reply to comment #57)
> (In reply to comment #56)
> 
> We want Dictionary to behave the same between v8 and js implementations. The v8 implementation never sets a persistent exception state; the js implementation does, and that's the root of the problem.
> 

This means we will get more and more similar troubles in the future until the root issue is solved. Can V8 binding and JSC binding gurus sit down and get this settled? Probably there could be an abstract layer on top of both bindings?

[Charles Wei]

I ran webkit-run-tests on Mac to test if any regression with this patch.  It turned out that this patch brings no regression,  instead , if fixes some test failures, with details below:

Tests that failed text/pixel/audio diff (13) : same as without the patch.
Tests that had stderr output (6):                  it was 8 without the patch.
Flaky tests (failed the first run and passed on retry) (12):  it was 17 without the patch. 
Tests expected to fail but passed (5):     same as without the patch.

[Sam Weinig]

Comment on attachment 173540 [details]
Patch

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.

[Charles Wei]

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

As said in comment #1, this happens for IndexedDB. Any test cases in LayoutTest/storage/indexeddb are test cases for this. Please try (So far we are the only JSC-based webkit porting, you guys don't have the problem so far because you are not enabling the indexedDB yet). Look at bug 92828 from Samsung who had the same problem trying to enable indexedDB on their porting.

If you don't agree with my patch, please have a proposal to solve this problem.

[Charles Wei]

Comment on attachment 173540 [details]
Patch

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.

[Sam Weinig]

(In reply to comment #62)
> > 
> > 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.
> 
> As said in comment #1, this happens for IndexedDB. Any test cases in LayoutTest/storage/indexeddb are test cases for this. Please try (So far we are the only JSC-based webkit porting, you guys don't have the problem so far because you are not enabling the indexedDB yet). Look at bug 92828 from Samsung who had the same problem trying to enable indexedDB on their porting.
> 
> If you don't agree with my patch, please have a proposal to solve this problem.

I asked for a reduced test case.  It is very hard to consider this in the abstract.

[Alec Flett]

Comment on attachment 173540 [details]
Patch

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.

(from an IDB perspective, not knowing a huge amount about JSC)
I agree that clearing the exception seems totally wrong here... at least in the context of JSDictionary itself. The issue here is the way the JSC-based IDB code is calling into the dictionary, not the dictionary itself, and this will almost certainly break other consumers of JSDictionary.

Not really knowing the JSC Dictionary API, IDB should be probing the dictionary looking for keys, and if it needs to "catch" an exception, then it needs to look something like this, in the IDB bindings code:

// assuming 'exec' holds the same ExecState:

JSValue val;
if (!dictionary->tryGetProperty("keyPath", val))
    return false;

// now we know the key is present and valid
Vector<String> stringArray;
if (!dictionary->tryGetProperty("keyPath", stringArray) {
    exec->clearException();
    String stringVal;
    if (!dictionary->tryGetProperty("keyPath", stringVal))
         return false;
     .. deal with stringVal here
    return true;
}
.. deal with stringArray here

I'm making assumptions here that it's cheap to keep calling tryGetProperty() - if its not, then the original value from the first tryGetProperty should be manually converted into each appropriate type.

[Sam Weinig]

r-'ing, as this still seems wrong.

[Darin Adler]

Comment on attachment 173540 [details]
Patch

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.
> 
> (from an IDB perspective, not knowing a huge amount about JSC)
> I agree that clearing the exception seems totally wrong here... at least in the context of JSDictionary itself. The issue here is the way the JSC-based IDB code is calling into the dictionary, not the dictionary itself, and this will almost certainly break other consumers of JSDictionary.
> 
> Not really knowing the JSC Dictionary API, IDB should be probing the dictionary looking for keys, and if it needs to "catch" an exception, then it needs to look something like this, in the IDB bindings code:
> 
> // assuming 'exec' holds the same ExecState:
> 
> JSValue val;
> if (!dictionary->tryGetProperty("keyPath", val))
>     return false;
> 
> // now we know the key is present and valid
> Vector<String> stringArray;
> if (!dictionary->tryGetProperty("keyPath", stringArray) {
>     exec->clearException();
>     String stringVal;
>     if (!dictionary->tryGetProperty("keyPath", stringVal))
>          return false;
>      .. deal with stringVal here
>     return true;
> }
> .. deal with stringArray here
> 
> I'm making assumptions here that it's cheap to keep calling tryGetProperty() - if its not, then the original value from the first tryGetProperty should be manually converted into each appropriate type.

Alec’s comments seem correct. The problem here is in IndexedDB code using JavaScriptCore functions incorrectly, not a bug in the JSDictionary wrapper class. That’s where it needs to be fixed.

[Kentaro Hara]

*** Bug 95563 has been marked as a duplicate of this bug. ***

[Darin Adler]

I’m getting really mixed up about this. What is the JSDictionary class? It seems to be an interface to help people work with JavaScript objects as a dictionary, hiding implementation details like exceptions. If that is so, then maybe I was wrong all along and the exception clearing does belong in JSDictionary.

Who created JSDictionary and can explain its purpose?

[Kentaro Hara]

(In reply to comment #69)
> It seems to be an interface to help people work with JavaScript objects as a dictionary, hiding implementation details like exceptions.

In my understanding, this is correct.

One confusion would come from that JSC has JSDictionary and Dictionary. Previously JSC has JSDictionary only and V8 has Dictionary only. Then, in order to make interfaces consistent between JSC and V8, we introduced js/Dictionary, which has the same interfaces as v8/Dictionary. Although we wanted to kill JSDictionary at some point, we haven't yet done it because we haven't reach a consensus on the interfaces of js/Dictionary.

[Charles Wei]

(In reply to comment #69)
> I’m getting really mixed up about this. What is the JSDictionary class? It seems to be an interface to help people work with JavaScript objects as a dictionary, hiding implementation details like exceptions. If that is so, then maybe I was wrong all along and the exception clearing does belong in JSDictionary.
> 
> Who created JSDictionary and can explain its purpose?

Yes, JSDictionary is a native helper class to work with JavaScript objects as a the backend of Dictionary,  it should hide it's implementation to external world,  but it's now implemented with JSValue,  and querying the JSDictionary will leave the JSC in exception state, which is really not what we want.

Somebody say(like comment #65) we should clear the exception outside of JSDictionary, like in where it's used (IndexedDB),  but the external world really should not know how Dictionary is implemented, and they shouldn't know that query the dictionary will impact the JSC state, which is also wrong.  Further, V8 doesn't have the same problem because they have a different way of implement the Dictionary.  we shouldn't clear the unexpected exception in the common code for both V8 and JSC.


The patch doesn't bring any regression, in the contrast, it fixes some DRT issues for Safari/Mac and for us (see comment #60).

[Sam Weinig]

(In reply to comment #69)
> I’m getting really mixed up about this. What is the JSDictionary class? It seems to be an interface to help people work with JavaScript objects as a dictionary, hiding implementation details like exceptions. If that is so, then maybe I was wrong all along and the exception clearing does belong in JSDictionary.
> 
> Who created JSDictionary and can explain its purpose?

It was I! If memory serves, I added it to make adding Event constructors easier to write, and to clean up other code that took vanilla JS objects and treated them like dictionaries, for instance the Geolocation PositionOptions.  The basic usage of the object was intended to look like this:

JSDictionary dictionary(exec, object);

double timeoutValue;
if (!dictionary.tryGetProperty("timeout", timeoutValue)) {
    // An exception occurred!
    return;
}

....

If a user wants to eat the exception, they can do so if tryGetProperty returns false, but the object should not do it for you, since in the most common case, you want it to propagate up.

[Sam Weinig]

Note: I am not clear on the history behind the similarly named, yet different Dictionary class, that seems to wrap a JSDictionary.  Its existence worries me.

[Yong Li]

(In reply to comment #70)
> (In reply to comment #69)
> > It seems to be an interface to help people work with JavaScript objects as a dictionary, hiding implementation details like exceptions.
> 
> In my understanding, this is correct.
> 
> One confusion would come from that JSC has JSDictionary and Dictionary. Previously JSC has JSDictionary only and V8 has Dictionary only. Then, in order to make interfaces consistent between JSC and V8, we introduced js/Dictionary, which has the same interfaces as v8/Dictionary. Although we wanted to kill JSDictionary at some point, we haven't yet done it because we haven't reach a consensus on the interfaces of js/Dictionary.

So it sounds the exception should be cleared by js/Dictionary, if its existence is to "make interfaces consistent between JSC and V8".

[Sam Weinig]

(In reply to comment #74)
> (In reply to comment #70)
> > (In reply to comment #69)
> > > It seems to be an interface to help people work with JavaScript objects as a dictionary, hiding implementation details like exceptions.
> > 
> > In my understanding, this is correct.
> > 
> > One confusion would come from that JSC has JSDictionary and Dictionary. Previously JSC has JSDictionary only and V8 has Dictionary only. Then, in order to make interfaces consistent between JSC and V8, we introduced js/Dictionary, which has the same interfaces as v8/Dictionary. Although we wanted to kill JSDictionary at some point, we haven't yet done it because we haven't reach a consensus on the interfaces of js/Dictionary.
> 
> So it sounds the exception should be cleared by js/Dictionary, if its existence is to "make interfaces consistent between JSC and V8".

I'm still having trouble understanding this in abstract.  Can someone give me a small test case, ~5 lines of JavaScript, that demonstrates this code path?  I find it hard to understand when an exception should not be propagated out to the JS caller.

[Joshua Bell]

(In reply to comment #75)
> I'm still having trouble understanding this in abstract.  Can someone give me a small test case, ~5 lines of JavaScript, that demonstrates this code path?  I find it hard to understand when an exception should not be propagated out to the JS caller.

var req = indexedDB.open('db'+Date.now());
req.onupgradeneeded = function() {
  var db = req.result;
  var funky = {toString: function() { return "hello"; }};
  db.createObjectStore('s1', {keyPath: 'string'}); // Shouldn't throw - string is used
  db.createObjectStore('s2', {keyPath: ['string']}); // Shouldn't throw - Array of string is used
  db.createObjectStore('s3', {keyPath: funky); // Shouldn't throw - converted to string "hello"
};

Spec:
http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBDatabase-createObjectStore-IDBObjectStore-DOMString-name-IDBObjectStoreParameters-optionalParameters

"If the optionalParameters argument is specified and has a keyPath property which is not undefined or null, then set keyPath to the value of this property. If keyPath is an Array, then each item in the array is converted to a string. If keyPath is not an Array, it is converted to a string."

[Geoffrey Garen]

<snip>
> var req = indexedDB.open('db'+Date.now());
</snip>

I don't see any code here that would throw an exception, which should then be swallowed. What am I missing?

> "If the optionalParameters argument is specified and has a keyPath property which is not undefined or null, then set keyPath to the value of this property. If keyPath is an Array, then each item in the array is converted to a string. If keyPath is not an Array, it is converted to a string."

I don't see any specification here that an exception should be swallowed. What am I missing?

[Joshua Bell]

(In reply to comment #77)
> I don't see any code here that would throw an exception, which should then be swallowed. What am I missing?

I don't think you're missing anything. My code was demonstrating the reverse, where we would not expect exceptions to be thrown when passing options with different value types to a method using the WebIDL Dictionary type.

The V8 implementation of Dictionary allows the method to be implemented e.g. by calling get("keyPath", Vector<String>) then (on failure) get("keyPath", String) with no need to clear exceptions.

The JSC implementation of Dictionary sets an exception when get("keyPath", Vector<String>) is called, and requires the method calling into Dictionary to clear the exception.

For this scenario, V8's Dictionary API makes more sense to me as the API for Dictionary, and callers that are interacting at a lower level would use JSDictionary.

...

This case also comes to mind:

var req = indexedDB.open('db'+Date.now());
req.onupgradeneeded = function() {
  var db = req.result;
  var bad = {toString: function() { throw new Error('Oops!'); }};
  try { db.createObjectStore('s3', {keyPath: bad}); } catch (ex) { alert(ex); }
};

I guess I'd expect an exception to be thrown here c/o the finer points of DOMString conversion as defined in WebIDL and ECMA-262's definition of ToString. And.... it alerts w/ V8.

[Sam Weinig]

(In reply to comment #78)
> (In reply to comment #77)
> > I don't see any code here that would throw an exception, which should then be swallowed. What am I missing?
> 
> I don't think you're missing anything. My code was demonstrating the reverse, where we would not expect exceptions to be thrown when passing options with different value types to a method using the WebIDL Dictionary type.
> 
> The V8 implementation of Dictionary allows the method to be implemented e.g. by calling get("keyPath", Vector<String>) then (on failure) get("keyPath", String) with no need to clear exceptions.
> 
> The JSC implementation of Dictionary sets an exception when get("keyPath", Vector<String>) is called, and requires the method calling into Dictionary to clear the exception.
> 
> For this scenario, V8's Dictionary API makes more sense to me as the API for Dictionary, and callers that are interacting at a lower level would use JSDictionary.

I finally understand (I hope).  The codes in question, IDBDatabase::createObjectStore, is attempting to use exceptions to influence control flow.  e.g. if the options.get("keyPath", keyPathArray) fails (due to an exception) try something else, options.getWithUndefinedOrNullCheck("keyPath", keyPathString).  This is a fragile idiom, and not one I think that makes sense to use.  Instead, we should be explicit in our assumption.  Something like:

Vector<String> keyPathArray;
String keyPathString;

if (options.isJSSequence("keyPath")) {
    if (options.get("keyPath", keyPathArray) {
        // An exception occurred, propagate it up.
        return;
    }
}

if (!options.getWithUndefinedOrNullCheck("keyPath", keyPathString))
....

This idiom could cleaned up and be put into a function that did the type checking for you, but it would have to be named explicitly.

[Joshua Bell]

(In reply to comment #79)
> 
> I finally understand (I hope).  The codes in question, IDBDatabase::createObjectStore, is attempting to use exceptions to influence control flow.  e.g. if the options.get("keyPath", keyPathArray) fails (due to an exception) try something else, options.getWithUndefinedOrNullCheck("keyPath", keyPathString). 

Where are exceptions coming into play? The code in v8/Dictionary.cpp is:

bool Dictionary::get(const String& key, Vector<String>& value) const
{
    v8::Local<v8::Value> v8Value;
    if (!getKey(key, v8Value))
        return false;
    if (!v8Value->IsArray())
        return false;
    v8::Local<v8::Array> v8Array = v8::Local<v8::Array>::Cast(v8Value);
    /* stringify and copy members over */
    return true;
}

> This is a fragile idiom, and not one I think that makes sense to use.  Instead, we should be explicit in our assumption.  Something like:
> 
> Vector<String> keyPathArray;
> String keyPathString;
> 
> if (options.isJSSequence("keyPath")) {
>     if (options.get("keyPath", keyPathArray) {
>         // An exception occurred, propagate it up.
>         return;
>     }
> }
> 
> if (!options.getWithUndefinedOrNullCheck("keyPath", keyPathString))
> ....
> 
> This idiom could cleaned up and be put into a function that did the type checking for you, but it would have to be named explicitly.

So the v8 version of Dictionary::get(String, Vector<String>) is providing this "isJSSequence?" functionality implicitly.

That said, this approach - adding an explicit Dictionary::isSequence(propertyName) - seems reasonable to me, although it would require changes to v8/Dictionary's get(String, Vector<String>) implementation to raise a TypeError if it's not a sequence.

[Alec Flett]

*** Bug 88287 has been marked as a duplicate of this bug. ***

[Michael Pruett]

Created attachment 177806 [details]
Patch

Here's a patch which checks whether the property being accessed is an array before attempting to convert the value to an array. The patch doesn't clear exceptions and requires no changes to existing IDB code.

With this patch and another fixing odd-length data in SerializedScriptValue, WebKitGtk+ passes 157 out of 196 layout tests in storage/indexeddb.

[Kentaro Hara]

Comment on attachment 177806 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177806&action=review

> Source/WebCore/ChangeLog:13
> +        JSDictionary should check whether the property being accessed in get()
> +        is an array before attempting to convert the value to an array.
> +
> +        Previously calling get() with a result type of Vector<String> when
> +        the named property could not be converted to an array would generate
> +        an exception.

I guess you can fix JSDictionary::convertValue(ExecState* exec, JSValue value, Vector<String>& result), instead of implementing yet another get().

[Michael Pruett]

Created attachment 177865 [details]
Patch

[Kentaro Hara]

Comment on attachment 177865 [details]
Patch

The change looks reasonable. Let's hear thoughts of sam, darin, ggaren before landing.

[Kentaro Hara]

Comment on attachment 177865 [details]
Patch

sam: any comments on this patch?

[Geoffrey Garen]

Comment on attachment 177865 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177865&action=review

This patch looks good to me, and I think it resolves the problem of swallowing exceptions.

> Source/WebCore/bindings/js/JSDictionary.h:159
>          if (m_exec->hadException())
>              return ExceptionThrown;
>  
> +        if (!success)
> +            return ConversionFailed;

Might be nice to move the test for exception inside the !success test. This is slightly more efficient, and it helps show the reader that we always check the return value before proceeding.

[Michael Pruett]

Created attachment 177906 [details]
Patch

I've cleaned up the flow of control in tryGetPropertyAndResult().

[Kentaro Hara]

Comment on attachment 177906 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177906&action=review

> Source/WebCore/bindings/js/JSDictionary.h:163
> +    case ConversionFailed:

As mentioned above, I'd like to add ASSERT_NOT_REACHED() to ConversionFailed.

[Michael Pruett]

Created attachment 177907 [details]
Patch

I've added ASSERT_NOT_REACHED() to verify that tryGetProperty() never returns ConversionFailed.

[Kentaro Hara]

Comment on attachment 177907 [details]
Patch

ok, thanks!

[Charles Wei]

Comment on attachment 177907 [details]
Patch

wohoo, eventually get it resolved, the other way.

[WebKit Review Bot]

Comment on attachment 177907 [details]
Patch

Clearing flags on attachment: 177907

Committed r136817: <http://trac.webkit.org/changeset/136817>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

I don't think this was the right way to go.  My biggest gripe is that JSDictionary used to cleanly map to the WebIDL dictionary type and now doesn't.  If this is really needed, I would rather it be in the IndexDB code, rather than polluting the general case.

[Sam Weinig]

Does anyone have a link to where IndexDB actually asks for an sequence or a String here?  The spec at http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#options-object-concept seems to only call for a keyPath of type DOMString?.  Is that different in another version of the spec? Is there a more up-to-date version I should be referencing?

[Michael Pruett]

(In reply to comment #96)
> Does anyone have a link to where IndexDB actually asks for an sequence or a String here?  The spec at http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#options-object-concept seems to only call for a keyPath of type DOMString?.  Is that different in another version of the spec? Is there a more up-to-date version I should be referencing?

Section 3.2.4 of the specification describes the handling of the keyPath property in the optionalParameters argument to createObjectStore():

http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#database-interface

"If the optionalParameters argument is specified and has a keyPath property which is not undefined or null, then set keyPath to the value of this property. If keyPath is an Array, then each item in the array is converted to a string. If keyPath is not an Array, it is converted to a string."

[Sam Weinig]

(In reply to comment #97)
> (In reply to comment #96)
> > Does anyone have a link to where IndexDB actually asks for an sequence or a String here?  The spec at http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#options-object-concept seems to only call for a keyPath of type DOMString?.  Is that different in another version of the spec? Is there a more up-to-date version I should be referencing?
> 
> Section 3.2.4 of the specification describes the handling of the keyPath property in the optionalParameters argument to createObjectStore():
> 
> http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#database-interface
> 
> "If the optionalParameters argument is specified and has a keyPath property which is not undefined or null, then set keyPath to the value of this property. If keyPath is an Array, then each item in the array is converted to a string. If keyPath is not an Array, it is converted to a string."

Thanks, that is very helpful.  It is odd/unfortunate that the IDL does not match the prose.  Given this, I think it is more reasonable that this be implemented as a quirk in the IndexDB bindings and not in JSDictionary.  Please revert.

[Kentaro Hara]

(In reply to comment #95)
> My biggest gripe is that JSDictionary used to cleanly map to the WebIDL dictionary type and now doesn't.

Would you elaborate on this? I don't figure out how this change violates the spec:

Spec: http://dev.w3.org/2006/webapi/WebIDL/#es-dictionary

This patch just changes a return value of JSDictionary::tryGetPropertyAndResult() so that callers of (JS)Dictionary can correctly judge if an exception was thrown or not. This information is useful for callers to handle exceptions (e.g. Indexed DB code). Also I don't think this behavior violates the WebIDL spec.

[Sam Weinig]

(In reply to comment #99)
> (In reply to comment #95)
> > My biggest gripe is that JSDictionary used to cleanly map to the WebIDL dictionary type and now doesn't.
> 
> Would you elaborate on this? I don't figure out how this change violates the spec:
> 
> Spec: http://dev.w3.org/2006/webapi/WebIDL/#es-dictionary
> 
> This patch just changes a return value of JSDictionary::tryGetPropertyAndResult() so that callers of (JS)Dictionary can correctly judge if an exception was thrown or not. This information is useful for callers to handle exceptions (e.g. Indexed DB code). Also I don't think this behavior violates the WebIDL spec.

Its not a question of violation, its the mapping of concepts.  As far as I can tell, in WebIDL there is no concept of converting failing due to not being the correct type.

Also, since I had issue with the original patch, it would have been polite to email me to ask for a final review here. I realize you asked in the bug, but I am cc'd on many bugs and don't get to them immediately all the time.

[Joshua Bell]

Re: the IDL - yeah, there are outstanding W3C-tracker bugs on updating the IDLs in the spec. I'll make sure there is one for this dictionary.

The WebIDL for this specific Dictionary in the spec should be written to match the prose, i.e.:

dictionary IDBObjectStoreParameters {
  (sequence<DOMString> or DOMString)? keyPath = null;
  boolean autoIncrement = false;
};

... of course, WebKitIDL doesn't support unions yet. Presumably that would allow the generated binding code to throw if necessary (anything that can't convert to DOMString), and createObjectStore callback would use e.g. 

  IDLUnion<Vector<String>, String> keyPath; // Just made this up
  options.get(keyPath); // If we got this far, shouldn't throw
  if (keyPath.isFirstType()) { ... } // Or something, don't design this here

[Michael Pruett]

Created attachment 178728 [details]
Patch

[Sam Weinig]

Comment on attachment 178728 [details]
Patch

This is still not right.  The special casing should be in the IndexDB code.  In fact, the IndexDB code is the odd duck here, the conversion from js object to c++ object should be happening in the bindings layer, not in the c++ layer.  See the Event constructors for the correct way to do this.

[Michael Pruett]

Created attachment 179040 [details]
Patch

With this patch I've implemented custom JSC bindings for parsing options in IDBDatabase.createObjectStore() and IDBObjectStore.createIndex(). This patch makes no changes to Dictionary or JSDictionary.

[Alec Flett]

Comment on attachment 179040 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179040&action=review

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:154
> +    bool autoIncrement = false;

This seems like an odd factoring of this code - perhaps move the autoIncrement above the first (!options.isUndefinedOrNull()) and put the options.get("autoIncrement",..) there too?

Other than that, the non-JSC code looks good to me, I like the way this is factored.

[Michael Pruett]

Created attachment 179158 [details]
Patch

I've updated IDBDatabase::createObjectStore() to move parsing of the autoIncrement property to the same block as parsing of the keyPath property as Alec suggested.

[Darin Adler]

Comment on attachment 179158 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179158&action=review

This is OK if we can’t fix the Dictionary class. But I think it would have been OK to fix Dictionary too. It’s just that it was wrong to change JSDictionary.

review- because the bindings have multiple small mistakes in them

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:52
> +    ExceptionCode ec = 0;

This should be moved down to the end of the function where it’s used rather than declaring it way up here.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:54
> +    JSValue nameValue = exec->argument(0);
> +    String name = nameValue.toString(exec)->value(exec);

There's no reason to put nameValue into a local variable. This can all be done on one line.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:58
> +    JSValue optionsValue = MAYBE_MISSING_PARAMETER(exec, 1, DefaultIsUndefined);

This is overkill. The argument function already does this without requiring any macro help. The way to write this is:

    JSValue optionsValue = exec->argument(1);

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:60
> +    if (!optionsValue.isUndefinedOrNull() && !optionsValue.isObject())
> +        return throwTypeError(exec, "Not an object.");

Is this type check really needed? Are there tests that cover this? Does the specification ask for it? Normally in JavaScript you can try to get the properties of anything, and there's no need to require that a JavaScript value be an object just to get its properties.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:62
> +    JSDictionary options(exec, optionsValue.getObject());

It's not idiomatic to use JSDictionary here. Why not just use JavaScriptCore functions directly on optionsValue?

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:67
> +    if (options.isValid() && options.get("keyPath", keyPathScriptValue)) {
> +        JSValue keyPathValue = keyPathScriptValue.jsValue();

This should just be:

    JSValue keyPathValue = optionsValue.get(exec, PropertyName("keyPath"));

Or something along those lines. JSDictionary does not add value here.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:68
> +        if (isJSArray(keyPathValue))

It’s surprising that a JavaScript array behaves differently than a non-array JavaScript object with the same properties as the array. That’s not a conventional distinction made in JavaScript in most cases. I guess IDB is unusual in that respect? Is that really what’s specified?

Do we have test cases that pass a non-array object that has array-like properties to test this?

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:82
> +    bool autoIncrement = false;
> +    if (options.isValid())
> +        options.get("autoIncrement", autoIncrement);
> +
> +    if (exec->hadException())
> +        return jsUndefined();

This should just be:

    JSValue autoIncrementValue = optionsValue.get(exec, PropertyName("autoIncrement"));
    if (exec->hadException())
        return jsUndefined();

    bool autoIncrement = autoIncrementValue.toBoolean(exec);
    if (exec->hadException())
        return jsUndefined();

Again, JSDictionary does not help us.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:84
> +    JSValue result = toJS(exec, globalObject(), WTF::getPtr(impl()->createObjectStore(name, keyPath, autoIncrement, ec)));

Please use get() instead of WTF::getPtr. WTF::getPtr just exists so the generator can make code that works correctly without knowing if the return value from createObjectStore is a PassRefPtr or a raw pointer.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:51
> +    ExceptionCode ec = 0;

Same comment about moving the ec definition down to where it's used.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:54
> +    ScriptExecutionContext* context = jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject())->scriptExecutionContext();
> +    if (!context)
> +        return jsUndefined();

We don’t want to throw an exception or log to the console in this case? Just fail silently?

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:57
> +    JSValue nameValue = exec->argument(0);
> +    String name = nameValue.toString(exec)->value(exec);

There's no reason to put nameValue into a local variable. This can all be done on one line.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:62
> +    IDBKeyPath keyPath;

Conversion of a JSValue into an IDBKeyPath is complex enough that I’d like these two classes to share a helper function that does it.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:63
> +    if (keyPathValue.isObject() && isJSArray(keyPathValue))

The isObject() check here is not needed or helpful. The isJSArray function will return false for any non-object value.

It’s surprising that a JavaScript array behaves differently than a non-array JavaScript object with the same properties as the array. That’s not a conventional distinction made in JavaScript in most cases. I guess IDB is unusual in that respect? Is that really what’s specified?

Do we have test cases that pass a non-array object that has array-like properties to test this?

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:70
> +    JSValue optionsValue = MAYBE_MISSING_PARAMETER(exec, 2, DefaultIsUndefined);

As in a similar case above, this should just be:

    JSValue optionsValue = exec->argument(2);

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:72
> +    if (!optionsValue.isUndefinedOrNull() && !optionsValue.isObject())
> +        return throwTypeError(exec, "Not an object.");

Is this type check really needed? Are there tests that cover this? Does the specification ask for it? Normally in JavaScript you can try to get the properties of anything, and there's no need to require that a JavaScript value be an object just to get its properties.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:74
> +    JSDictionary options(exec, optionsValue.getObject());

No need to use JSDictionary here.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:78
> +    bool unique = false;
> +    if (options.isValid())
> +        options.get("unique", unique);

This should just be:

    JSValue uniqueValue = unique.get(exec, PropertyName("unique"));
    if (exec->hadException())
        return jsUndefined();

    bool unique = uniqueValue.toBoolean(exec);
    if (exec->hadException())
        return jsUndefined();

JSDictionary does not help us.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:85
> +    bool multiEntry = false;
> +    if (options.isValid())
> +        options.get("multiEntry", multiEntry);
> +
> +    if (exec->hadException())
> +        return jsUndefined();

This should just be:

    JSValue multiEntryValue = multiEntry.get(exec, PropertyName("multiEntry"));
    if (exec->hadException())
        return jsUndefined();

    bool multiEntry = multiEntryValue.toBoolean(exec);
    if (exec->hadException())
        return jsUndefined();

JSDictionary does not help us.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:87
> +    JSValue result = toJS(exec, globalObject(), WTF::getPtr(impl()->createIndex(context, name, keyPath, unique, multiEntry, ec)));

Please use get() instead of WTF::getPtr. WTF::getPtr just exists so the generator can make code that works correctly without knowing if the return value from createObjectStore is a PassRefPtr or a raw pointer.

[Darin Adler]

(In reply to comment #107)
> This is OK if we can’t fix the Dictionary class. But I think it would have been OK to fix Dictionary too. It’s just that it was wrong to change JSDictionary.

No, I changed my mind. This idea of the bindings passing a Dictionary to a DOM class is not right. It’s not how we want to do JavaScript binding. The order that we evaluate things from the dictionary is observable in JavaScript. It’s not OK to have that mixed in with the job of the DOM function at an arbitrary time. It needs to be part of the binding. The relationship of JavaScript and DOM here was done wrong.

So this latest patch is the right direction.

[Michael Pruett]

(In reply to comment #107)
> > Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:60
> > +    if (!optionsValue.isUndefinedOrNull() && !optionsValue.isObject())
> > +        return throwTypeError(exec, "Not an object.");
> 
> Is this type check really needed? Are there tests that cover this? Does the specification ask for it? Normally in JavaScript you can try to get the properties of anything, and there's no need to require that a JavaScript value be an object just to get its properties.

This check is required by the Web IDL specification:

http://www.w3.org/TR/2011/WD-WebIDL-20110927/#es-dictionary

"IDL dictionary type values are represented by ECMAScript Object values."

This check matches that on line 1827 of CodeGeneratorV8.pm.

This check is tested in storage/indexeddb/create-object-store-options.html.

> > Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:68
> > +        if (isJSArray(keyPathValue))
> 
> It’s surprising that a JavaScript array behaves differently than a non-array JavaScript object with the same properties as the array. That’s not a conventional distinction made in JavaScript in most cases. I guess IDB is unusual in that respect? Is that really what’s specified?
> 
> Do we have test cases that pass a non-array object that has array-like properties to test this?

The IndexedDB specification does make such a distinction:

http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#database-interface

"If keyPath is an Array, then each item in the array is converted to a string. If keyPath is not an Array, it is converted to a string."

Converting an object to a string results in the string "[object Object]", which is not a valid key path since this string is not a valid JavaScript identifier:

http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#key-path-construct

Key paths consisting of array-like objects are prohibited by the IndexedDB specification and are rejected with a syntax error exception by WebKit/Chromium and Firefox.

[Michael Pruett]

Created attachment 179326 [details]
Patch

I've removed the use of JSDictionary, simplified accessing arguments, and changed WTF::getPtr() to .get(). I've also added a utility function for converting a JSValue into an IDBKeyPath and updated the check for a script execution context to throw an exception if no context is present.

[Joshua Bell]

(In reply to comment #109)
> "If keyPath is an Array, then each item in the array is converted to a string. If keyPath is not an Array, it is converted to a string."
...
> Key paths consisting of array-like objects are prohibited by the IndexedDB specification and are rejected with a syntax error exception by WebKit/Chromium and Firefox.

It's unfortunate that this does not match the WebIDL/ECMAScript binding for sequence<T> (which allows Array-like w/ |length| property etc). Fortunately, moving to that would be a backwards compatible change for the IDB spec in the sense that no currently valid key paths would be rejected if we started accepting {'length': 2, '0': 'identifier1', '1': 'identifier2'. I'll raise this on public-webapps soon.

[Darin Adler]

Comment on attachment 179326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179326&action=review

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:54
> +    if (exec->argumentCount() < 1)
> +        return throwError(exec, createNotEnoughArgumentsError(exec));
> +    String name = exec->argument(0).toString(exec)->value(exec);
> +    if (exec->hadException())
> +        return jsUndefined();

I suggest breaking this into two paragraphs. The argument count check seems separate from extracting the first argument.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:62
> +    if (optionsValue.isObject()) {

Why do we have this check? I believe we’ll get the same behavior without it since the get calls will return undefined values, and I would prefer that we omit it to streamline the code, and move the definitions of the variables closer to the code that sets them up.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:71
> +        if (!keyPathValue.isUndefinedOrNull())
> +            keyPath = idbKeyPathFromValue(exec, keyPathValue);
> +
> +        if (exec->hadException())
> +            return jsUndefined();

I suggest nesting the second if statement inside the first to make it clear the relationship between the function that might have created an exception and the need for the if statement for the early return.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:74
> +        JSValue autoIncrementValue = optionsValue.get(exec, Identifier(exec, "autoIncrement"));
> +        autoIncrement = autoIncrementValue.toBoolean(exec);

This would read better without the autoIncrementValue local variable.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:60
> +    IDBKeyPath keyPath = idbKeyPathFromValue(exec, exec->argument(1));

This will convert undefined to the string "undefined" and null to the string "null". I assume that’s the behavior we want.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:70
> +    if (optionsValue.isObject()) {

Why do we have this check? I believe we’ll get the same behavior without it since the get calls will return undefined values, and I would prefer that we omit it to streamline the code.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:72
> +        JSValue uniqueValue = optionsValue.get(exec, Identifier(exec, "unique"));
> +        unique = uniqueValue.toBoolean(exec);

This would read better without the uniqueValue local variable.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:77
> +        JSValue multiEntryValue = optionsValue.get(exec, Identifier(exec, "multiEntry"));
> +        multiEntry = multiEntryValue.toBoolean(exec);

This would read better without the multiEntryValue local variable.

[Darin Adler]

Comment on attachment 179326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179326&action=review

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:42
> +#include "IDBBindingUtilities.h"
> +#include "IDBDatabase.h"
> +#include "IDBKeyPath.h"
> +#include "IDBObjectStore.h"
> +#include "JSIDBObjectStore.h"
> +
> +#include <runtime/Error.h>
> +#include <runtime/JSArray.h>
> +#include <runtime/JSString.h>

WebKit coding style puts these into a single paragraph, not two.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:48
> +JSValue JSIDBDatabase::createObjectStore(JSC::ExecState* exec)

Given the "using namespace JSC" above, this can just be ExecState rather than JSC::ExecState.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:41
> +#include "IDBBindingUtilities.h"
> +#include "IDBKeyPath.h"
> +#include "IDBObjectStore.h"
> +#include "JSIDBIndex.h"
> +
> +#include <runtime/Error.h>
> +#include <runtime/JSArray.h>
> +#include <runtime/JSString.h>

WebKit coding style puts these into a single paragraph, not two.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:47
> +JSValue JSIDBObjectStore::createIndex(JSC::ExecState* exec)

Given the "using namespace JSC" above, this can just be ExecState rather than JSC::ExecState.

[Michael Pruett]

(In reply to comment #112)
> > Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:62
> > +    if (optionsValue.isObject()) {
> 
> Why do we have this check? I believe we’ll get the same behavior without it since the get calls will return undefined values, and I would prefer that we omit it to streamline the code, and move the definitions of the variables closer to the code that sets them up.

This check ensures that accessing properties on optionsValue does not result in an exception being thrown, as will be the case if optionsValue is null or undefined, which are its only other possible values. Perhaps changing this check from isObject() to !isUndefinedOrNull() would make this more clear.

> > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:60
> > +    IDBKeyPath keyPath = idbKeyPathFromValue(exec, exec->argument(1));
> 
> This will convert undefined to the string "undefined" and null to the string "null". I assume that’s the behavior we want.

Yes that is the specified behavior:

http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBObjectStoreSync-createIndex-IDBIndexSync-DOMString-name-any-keyPath-IDBIndexParameters-optionalParameters

"If the keyPath argument is an Array, then each item in the array is converted to a DOMString. If keyPath is not an Array, it is converted to a DOMString."

This behavior is tested in storage/indexeddb/keypath-basics.html.

[Michael Pruett]

Created attachment 179456 [details]
Patch

I've updated formatting and elided temporary variables in value conversion.

[WebKit Review Bot]

Comment on attachment 179456 [details]
Patch

Clearing flags on attachment: 179456

Committed r138081: <http://trac.webkit.org/changeset/138081>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.