Bug 96614

Created attachment 163809 [details] Patch

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

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.

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?

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

Created attachment 164065 [details] Patch

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.

Comment on attachment 164065 [details] Patch Thanks, Kentaro.

Comment on attachment 164065 [details] Patch Clearing flags on attachment: 164065 Committed r128568: <http://trac.webkit.org/changeset/128568>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 96739

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

hmmm, will look into the test cases broken.

Created attachment 173253 [details] Patch

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

Created attachment 173261 [details] Patch

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.

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?

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

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

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

Comment on attachment 173261 [details] Patch Er - no reviewed by line? I don't think you can use commit queue then?

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

(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 :-)

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

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

(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)

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

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

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.

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

Created attachment 173304 [details] Patch

Created attachment 173306 [details] Patch

Comment on attachment 173306 [details] Patch Commit it after review by George and Kentaro Hara.

Comment on attachment 173306 [details] Patch Clearing flags on attachment: 173306 Committed r134069: <http://trac.webkit.org/changeset/134069>

All reviewed patches have been landed. Closing bug.

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

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

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.

Let me roll it out in hours.

Re-opened since this is blocked by bug 101852

(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?

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

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

Created attachment 173540 [details] Patch

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.

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.

(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?

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

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.

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.

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

Any comments to this ?

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.

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

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?

(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?

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

(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?

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.

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.

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

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.

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

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.

r-'ing, as this still seems wrong.

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.

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

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?

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

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

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

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.

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

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

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

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

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

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

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

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

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.

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

Created attachment 177865 [details] Patch

Comment on attachment 177865 [details] Patch The change looks reasonable. Let's hear thoughts of sam, darin, ggaren before landing.

Comment on attachment 177865 [details] Patch sam: any comments on this patch?

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.

Created attachment 177906 [details] Patch I've cleaned up the flow of control in tryGetPropertyAndResult().

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.

Created attachment 177907 [details] Patch I've added ASSERT_NOT_REACHED() to verify that tryGetProperty() never returns ConversionFailed.

Comment on attachment 177907 [details] Patch ok, thanks!

Comment on attachment 177907 [details] Patch wohoo, eventually get it resolved, the other way.

Comment on attachment 177907 [details] Patch Clearing flags on attachment: 177907 Committed r136817: <http://trac.webkit.org/changeset/136817>

All reviewed patches have been landed. Closing bug.

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.

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?

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

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

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

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

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

Created attachment 178728 [details] Patch

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.

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.

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.

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.

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.

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

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

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.

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

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.

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.

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

Created attachment 179456 [details] Patch I've updated formatting and elided temporary variables in value conversion.

Comment on attachment 179456 [details] Patch Clearing flags on attachment: 179456 Committed r138081: <http://trac.webkit.org/changeset/138081>

All reviewed patches have been landed. Closing bug.