Summary: | IndexedDB result of deleting a record should be true or false | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Pilgrim (Google) <pilgrim> | ||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Joshua Bell <jsbell> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, antonm, dglazkov, dgrogan, fishd, gustavo.noronha, gustavo, hans, japhet, jsbell, levin, pilgrim, tony, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Attachments: |
|
Description
Mark Pilgrim (Google)
2011-05-04 12:09:14 PDT
Created attachment 92294 [details]
test case
Created attachment 93700 [details]
Patch
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 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
Created attachment 93730 [details]
Patch
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? (In reply to comment #6) > Did you mean to change the indention level here? Oops, no. Created attachment 93984 [details]
Patch
Fixes indentation.
Comment on attachment 93984 [details]
Patch
Patch doesn't apply but otherwise it looks fine to me.
Comment on attachment 93984 [details]
Patch
unofficial r+
Thanks for fixing this.
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. Created attachment 112285 [details]
Patch
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(); +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? (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. (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. (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? (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. Filed tracking bug: https://bugs.webkit.org/show_bug.cgi?id=70833 Created attachment 112370 [details]
Patch
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.
Comment on attachment 112370 [details] Patch Attachment 112370 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10210313 Comment on attachment 112370 [details] Patch Attachment 112370 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10209359 Created attachment 112414 [details]
Patch
Comment on attachment 112414 [details] Patch Attachment 112414 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10209403 Created attachment 112439 [details]
Patch
@jsbell: Ping me when this is ready for review (assuming Dave isn't interested in continuing to review). (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 :) Comment on attachment 112439 [details] Patch Attachment 112439 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10214309 Comment on attachment 112439 [details] Patch Attachment 112439 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10212411 Comment on attachment 112439 [details] Patch Attachment 112439 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10208691 Created attachment 112911 [details]
Patch
Created attachment 113517 [details]
Patch
Comment on attachment 113517 [details]
Patch
Tweaked one more test. I believe this is ready to go, and we have consensus on the list.
abarth@ or levin@, could you take another peek? Comment on attachment 113517 [details] Patch Clearing flags on attachment: 113517 Committed r99229: <http://trac.webkit.org/changeset/99229> All reviewed patches have been landed. Closing bug. |