IndexedDB result of deleting a record should be true or false

• Component: New Bugs
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

Original test: http://mxr.mozilla.org/mozilla2.0/source/dom/indexedDB/test/test_odd_result_order.html?force=1

This test is adapted from a test in Mozilla's IndexedDB test suite. It checks that the result property of a request after an asychronous delete operation is either true (if the record existed and was deleted successfully) or false (if the record did not exist).

WebKit fails this test. The result property is always null, regardless of whether the delete operation succeeded or failed.

Citation: http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#dfn-steps-for-deleting-a-record-from-an-object-store "If no record exists with key key in store, then the result of this algorithm is false... [steps for successful deletion, and then] The result of this algorithm is true."

[Mark Pilgrim (Google)]

Created attachment 92294 [details]
test case

[Mark Pilgrim (Google)]

Created attachment 93700 [details]
Patch

[WebKit Review Bot]

Comment on attachment 93700 [details]
Patch

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

New failing tests:
storage/indexeddb/objectstore-basics.html

[WebKit Review Bot]

Created attachment 93723 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mark Pilgrim (Google)]

Created attachment 93730 [details]
Patch

[Tony Chang]

Comment on attachment 93730 [details]
Patch

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

This looks fine to me, but dgrogan or others should also review.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:300
> -        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::NOT_FOUND_ERR, "Key does not exist in the object store."));
> +      callbacks->onError(SerializedScriptValue::create(v8::False()), IDBDatabaseError::create(IDBDatabaseException::NOT_FOUND_ERR, "Key does not exist in the object store."));

Did you mean to change the indention level here?

[Mark Pilgrim (Google)]

(In reply to comment #6)
> Did you mean to change the indention level here?

Oops, no.

[Mark Pilgrim (Google)]

Created attachment 93984 [details]
Patch

Fixes indentation.

[David Levin]

Comment on attachment 93984 [details]
Patch

Patch doesn't apply but otherwise it looks fine to me.

[David Grogan]

Comment on attachment 93984 [details]
Patch

unofficial r+

Thanks for fixing this.

[Joshua Bell]

The current patches extend the error mechanism to include a value. That would require a two-sided commit to land. It also looks wrong - the IDB spec doesn't specify an error when you try and delete record that doesn't exist, but success with false result.

So... we need a much simpler patch. I'll put it together.

[Joshua Bell]

Created attachment 112285 [details]
Patch

[David Levin]

Comment on attachment 112285 [details]
Patch

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

It looks like you are calling a v8 bindings only method "trueValue" from generic code "Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp".

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1958
> +    DEFINE_STATIC_LOCAL(RefPtr<SerializedScriptValue>, trueValue, (0));

Given that this function may be called from different threads (putting it on SerializedScriptValue implies that somewhat since it may be used from workers).

This doesn't look like a safe way of doing this.

Does the result really need to be cached? Is there some perf test that indicates this?

(I see that nullValue does this, but I wonder why and I'm concerned about it as well.)

If nothing else, we should add asserts in these methods to verify that they are only used on the main thread, but personally I'd prefer that these non-threadsafe methods not be in SerializedScriptValue (and if caching is needed perhaps it should happen in code that is known to be single threaded.)

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1959
> +    if (!trueValue) {

Prefer "fail fast":

if (trueValue)
  return trueValue.get();

[Joshua Bell]

+antonm

I agree with all of levin's feedback. I was following the pattern used for nullValue and undefinedValue which it looks like came from antonm. Any comments?

[David Levin]

(In reply to comment #14)
> +antonm
> 
> I agree with all of levin's feedback. I was following the pattern used for nullValue and undefinedValue which it looks like came from antonm. Any comments?

The first change to do this was http://trac.webkit.org/changeset/66628 If you look at the associated bug, there is a comment explaining the static https://bugs.webkit.org/show_bug.cgi?id=41372#c18 

I wish the static var was local to the history navigation, and there was an assert to verify that this is only called on the main thread. I would gladly r+ clean-ups to this.

I think every other change just copied the pattern because it was already there.

[Joshua Bell]

(In reply to comment #13)
> (From update of attachment 112285 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112285&action=review
> 
> It looks like you are calling a v8 bindings only method "trueValue" from generic code "Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp".

It's worse: undefinedValue() and createFromWire() are already in the WebCore/storage code but only implemented in the v8 version of SerializedScriptValue. Looking to see what it'd take to add those to the JS version.

[David Levin]

(In reply to comment #16)
> (In reply to comment #13)
> > (From update of attachment 112285 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=112285&action=review
> > 
> > It looks like you are calling a v8 bindings only method "trueValue" from generic code "Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp".
> 
> It's worse: undefinedValue() and createFromWire() are already in the WebCore/storage code but only implemented in the v8 version of SerializedScriptValue. Looking to see what it'd take to add those to the JS version.

So jsc must not use this at all?

[Joshua Bell]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #13)
> > > (From update of attachment 112285 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=112285&action=review
> > > 
> > > It looks like you are calling a v8 bindings only method "trueValue" from generic code "Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp".
> > 
> > It's worse: undefinedValue() and createFromWire() are already in the WebCore/storage code but only implemented in the v8 version of SerializedScriptValue. Looking to see what it'd take to add those to the JS version.
> 
> So jsc must not use this at all?

Correct. 

This isn't the first place I've noticed where the code is effectively v8-only. It looks like there's been some effort to keep the INDEXED_DATABASE-wrapped code generic, but since it's not being built things slip through the cracks.

[Joshua Bell]

Filed tracking bug: https://bugs.webkit.org/show_bug.cgi?id=70833

[Joshua Bell]

Created attachment 112370 [details]
Patch

[Joshua Bell]

Comment on attachment 112370 [details]
Patch

New patch - it leaves nullValue() alone for now but adds a FIXME, but eliminates the local statics for the other types.

The JSC binding code is a shot in the dark; I don't have a build environment for it handy, so it's of dubious quality at best. Will give it a whirl tomorrow.

[Early Warning System Bot]

Comment on attachment 112370 [details]
Patch

Attachment 112370 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10210313

[Gyuyoung Kim]

Comment on attachment 112370 [details]
Patch

Attachment 112370 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10209359

[Joshua Bell]

Created attachment 112414 [details]
Patch

[Early Warning System Bot]

Comment on attachment 112414 [details]
Patch

Attachment 112414 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10209403

[Joshua Bell]

Created attachment 112439 [details]
Patch

[Adam Barth]

@jsbell: Ping me when this is ready for review (assuming Dave isn't interested in continuing to review).

[David Levin]

(In reply to comment #27)
> @jsbell: Ping me when this is ready for review (assuming Dave isn't interested in continuing to review).

For the record, I'm fine with abarth reviewing (or doing a review myself. Either way it is ok with me :)

[Early Warning System Bot]

Comment on attachment 112439 [details]
Patch

Attachment 112439 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10214309

[Gyuyoung Kim]

Comment on attachment 112439 [details]
Patch

Attachment 112439 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10212411

[Collabora GTK+ EWS bot]

Comment on attachment 112439 [details]
Patch

Attachment 112439 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10208691

[Joshua Bell]

Created attachment 112911 [details]
Patch

[Joshua Bell]

Created attachment 113517 [details]
Patch

[Joshua Bell]

Comment on attachment 113517 [details]
Patch

Tweaked one more test. I believe this is ready to go, and we have consensus on the list.

[Joshua Bell]

abarth@ or levin@, could you take another peek?

[WebKit Review Bot]

Comment on attachment 113517 [details]
Patch

Clearing flags on attachment: 113517

Committed r99229: <http://trac.webkit.org/changeset/99229>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.