| Summary: | IDB: storage/indexeddb/mozilla/clear.html fails | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||
| Component: | WebKit2 | Assignee: | Brady Eidson <beidson> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | alecflett, commit-queue, jsbell, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Brady Eidson
2014-02-05 16:49:44 PST
The ASSERTs were simply wrong in our multi-process universe, so stopping the crashing was a matter of updating the asserts. The test is outdated. It tests for a success condition no longer allowed for by the spec. In updating the test for the current spec I uncovered another correctness bug (WebKit current gives NULL whereas the spec calls for UNDEFINED) Uploading the current patch in progress so I can grab from a different machine later. Created attachment 223283 [details]
Current patch (still has a layout test bug, so not ready for review/landing)
Created attachment 223313 [details]
Patch v1 - Enable the test, update it to current spec, and make it pass.
Comment on attachment 223313 [details] Patch v1 - Enable the test, update it to current spec, and make it pass. View in context: https://bugs.webkit.org/attachment.cgi?id=223313&action=review r=me, but cq- to consider the question about converting a RefPtr<IDBKey> to a bool, and whether to use "!!" syntax for pointer conversion to bool. > Source/WebCore/ChangeLog:10 > + Update the value deserializer to take into account whether or not there was an IDBKey Nit: Can haz period? > Source/WebCore/ChangeLog:16 > + * Modules/indexeddb/IDBRequest.cpp: > + (WebCore::IDBRequest::onSuccess): Call the new form of deserializeIDBValueBuffer Ditto. > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:290 > - Deprecated::ScriptValue value = deserializeIDBValueBuffer(requestState(), backend->valueBuffer()); > + Deprecated::ScriptValue value = deserializeIDBValueBuffer(requestState(), backend->valueBuffer(), key); Technically, shouldn't this be "key.get()" to match the similar call below? And would "!!key.get()" be clearer that you're converting a pointer into a bool? > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:421 > - Deprecated::ScriptValue value = deserializeIDBValueBuffer(requestState(), buffer); > + Deprecated::ScriptValue value = deserializeIDBValueBuffer(requestState(), buffer, key.get()); Would it be clearer to use "!!key.get()" here instead to show you're converting a pointer into a bool? > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:311 > + // If the key doesn't exist, then the value must be undefined (as opposed to null) Nit: Plz add period. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:313 > + // We either shouldn't have a buffer or it should be of size 0 Nit: Add period. > Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.cpp:104 > + const char* modeString; Should probably initialize this to NULL: const char* modeString = NULL; > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBTransaction.cpp:70 > + // It's okay to not have a SQLite transaction or not have started it yet because its okay for a WebProcess Typo: "its" => "it's" (In reply to comment #6) > (From update of attachment 223313 [details]) > > > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:290 > > - Deprecated::ScriptValue value = deserializeIDBValueBuffer(requestState(), backend->valueBuffer()); > > + Deprecated::ScriptValue value = deserializeIDBValueBuffer(requestState(), backend->valueBuffer(), key); > > Technically, shouldn't this be "key.get()" to match the similar call below? I guess the reason I wrote them differently is because this one is a RefPtr whereas the key.get() below is a PassRefPtr. That said, they both have unspecifiedbooltype operators declared, so I'm not sure why I treat them differently in this context... > And would "!!key.get()" be clearer that you're converting a pointer into a bool? We have no coding style guideline about this, and in general I'm not sure when !! would be preferred with ptr -> bool conversion, the same way I'm not sure when it might be preferred with int -> bool conversion. Searching for !! use in WebCore shows that we use it... but not nearly as much as one might suspect. I don't feel strongly about it one way or the other, but am annoyed that we don't have a standard. |