Bug 83492 – IndexedDB: Attributes for a cursor "run past the end" should be undefined.

• Component: New Bugs
• Status: RESOLVED FIXED

[Alec Flett]

IndexedDB: Attributes for a cursor "run past the end" should be undefined.

[Alec Flett]

Created attachment 136281 [details]
Patch

[Alec Flett]

Created attachment 136282 [details]
Patch

[Alec Flett]

jsbell@ - mind taking a look? in particular I want to make sure I have the RefPtr stuff right for m_currentValue, since I'm storing a SerializedScriptValue() and converting it to an IDBAny on demand in IDBCursor::value()

[Joshua Bell]

Comment on attachment 136282 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        update and delete can be called before continue, but not afterwards.

Nit: For consistency, should be update() and delete() and reword to start the sentence with a capital.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:87
> +    return IDBAny::create(m_currentValue.get());

As an aside, the spec includes "if this property returns an object, it returns the same object instance every time it is inspected, until the cursor's value is changed. This means that if the object is modified, those modifications will be seen by anyone inspecting the value of the cursor." - do we implement that? Would returning the same IDBAny instance on each call affect that behavior? My guess is no and no. Implementing that part of the spec may be problematic. (This change doesn't affect it either way.)

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:100
> +        ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR;

Spec says this should be InvalidStateError (DOM4-style) - can you make it INVALID_STATE_ERR (not part of the IDBDatabaseException enum)

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:123
> +        ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR;

ditto

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:150
> +        ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR;

ditto

> Source/WebCore/Modules/indexeddb/IDBCursor.h:87
> +    RefPtr<IDBKey> m_currentKey;

Does "current" add anything here? Can it just be m_key etc?

> Source/WebCore/Modules/indexeddb/IDBCursor.h:89
> +    RefPtr<SerializedScriptValue> m_currentValue;

Although I believe this code is correct, I would have this be a RefPtr<IDBAny> and re-use the IDBAny across calls so that we minimize the number of places IDB objects hold SSV pointers. It shouldn't make any practical difference, though.

> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:52
> +    result = evalAndLog("objectStore.add({'x': " + nextToAdd + " }, " + nextToAdd + ")");

Multiple add calls can be done w/o waiting for the success callback of the previous one. Just use a for a loop here.

> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:148
> +        // behavior against pre-caching. Make sure to use prime

Nit: "pre-fetching" (so readers know what to search on)

> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:173
> +        onTransactionComplete();

The transaction is not actually complete here, the cursor has just run to the end...

> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:179
> +    evalAndExpectException("cursor.continue()", "");

I'd move this inline since it's part of the consistency checks...

> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:180
> +    finishJSTest();

And either call this directly, or hang it off he actual transaction oncomplete callback.

[Joshua Bell]

Comment on attachment 136282 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:88
>  }

Since m_currentValue is a RefPtr<SerializedScriptValue> this should just be: return IDBAny::create(m_currentValue)

[Alec Flett]

Created attachment 136319 [details]
Patch

[Alec Flett]

Created attachment 136325 [details]
Patch

[Alec Flett]

Comment on attachment 136282 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:87
>> +    return IDBAny::create(m_currentValue.get());
> 
> As an aside, the spec includes "if this property returns an object, it returns the same object instance every time it is inspected, until the cursor's value is changed. This means that if the object is modified, those modifications will be seen by anyone inspecting the value of the cursor." - do we implement that? Would returning the same IDBAny instance on each call affect that behavior? My guess is no and no. Implementing that part of the spec may be problematic. (This change doesn't affect it either way.)

changed to be an IDBAny, and sadly you're correct: the obj does change each time.

>> Source/WebCore/Modules/indexeddb/IDBCursor.h:87
>> +    RefPtr<IDBKey> m_currentKey;
> 
> Does "current" add anything here? Can it just be m_key etc?

I personally think it does because m_backend can get out of sync with m_current*

>> Source/WebCore/Modules/indexeddb/IDBCursor.h:89
>> +    RefPtr<SerializedScriptValue> m_currentValue;
> 
> Although I believe this code is correct, I would have this be a RefPtr<IDBAny> and re-use the IDBAny across calls so that we minimize the number of places IDB objects hold SSV pointers. It shouldn't make any practical difference, though.

Done.

>> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:52
>> +    result = evalAndLog("objectStore.add({'x': " + nextToAdd + " }, " + nextToAdd + ")");
> 
> Multiple add calls can be done w/o waiting for the success callback of the previous one. Just use a for a loop here.

this was copied from another test, and I thought the same until I realized that you don't have a final onsuccess to fire if you do it like that - you could attach to the last-added value, but there's no guarantee that the other values have been written by then. (at least that I could see...)

[Alec Flett]

jsbell@ - comments addressed...

[WebKit Review Bot]

Comment on attachment 136325 [details]
Patch

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

New failing tests:
storage/indexeddb/cursor-continue.html
storage/indexeddb/mozilla/cursors.html

[WebKit Review Bot]

Created attachment 136379 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alec Flett]

Created attachment 136498 [details]
Patch

[Alec Flett]

jsbell@ - ok, one more try, the other tests pass this time! (one mozilla test had to be altered because the new behavior is correct)

[Joshua Bell]

(In reply to comment #8)
> changed to be an IDBAny, and sadly you're correct: the obj does change each time.

I filed https://bugs.webkit.org/show_bug.cgi?id=83526 to track this.

> this was copied from another test, and I thought the same until I realized that you don't have a final onsuccess to fire if you do it like that - you could attach to the last-added value, but there's no guarantee that the other values have been written by then. (at least that I could see...)

Within a transaction, tasks a guaranteed to execute in the order they are requested, so if you run:

req1 = store.put(1, "key");
req2 = store.get("key");

then req2.result will eventually be 1, even though you're not waiting for req1 to succeed before you issue the get() call.

[Joshua Bell]

Comment on attachment 136498 [details]
Patch

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

The cursor-continue.html test failed (exception code changed) - did its expected file need to be updated as well?

> LayoutTests/ChangeLog:11
> +        * storage/indexeddb/resources/shared.js:

Should add storage/indexeddb/mozilla/cursors-expected.txt to the list

(Sadly, the list is not automagically updated if you revise a patch)

[Alec Flett]

Created attachment 136538 [details]
Patch

[Joshua Bell]

Comment on attachment 136538 [details]
Patch

lgtm

[Alec Flett]

ojan@ - one more for today - r? cq?

[Ojan Vafai]

Comment on attachment 136538 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:175
> +void IDBCursor::setGotValue()

This does more than just set m_gotValue. Maybe this should be called something like syncToBackendValues?

> LayoutTests/storage/indexeddb/cursor-continue-validity.html:1
> +<html>

New tests should all be in standards mode unless they're explicitly testing quirks behavior. So, this should start with:
<!DOCTYPE html>

> LayoutTests/storage/indexeddb/cursor-continue-validity.html:7
> +<script src="resources/cursor-continue-validity.js"></script>

We've been moving away from using a separate script file for tests that are not pure JS tests (i.e. test that can be run without a browser context). Instead, just inline cursor-continue-validity.js. It makes maintaining the tests much easier when the whole test is contained in one file (with the test harness code still in external files of course).

> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:132
> +function testModifyContinueOrder() {

Here and below, opening curly brace should be on the next line.

[Joshua Bell]

(In reply to comment #19)
> 
> > LayoutTests/storage/indexeddb/cursor-continue-validity.html:7
> > +<script src="resources/cursor-continue-validity.js"></script>
> 
> We've been moving away from using a separate script file for tests that are not pure JS tests (i.e. test that can be run without a browser context). Instead, just inline cursor-continue-validity.js. It makes maintaining the tests much easier when the whole test is contained in one file (with the test harness code still in external files of course).

dgrogan@ just moved most of the IDB test logic from .html into .js files so we can run them both in a Window context and in a Worker (or SharedWorker) context. The only exceptions are tests than span Window+Worker or places where the APIs differ.

[Ojan Vafai]

(In reply to comment #20)
> (In reply to comment #19)
> > 
> > > LayoutTests/storage/indexeddb/cursor-continue-validity.html:7
> > > +<script src="resources/cursor-continue-validity.js"></script>
> > 
> > We've been moving away from using a separate script file for tests that are not pure JS tests (i.e. test that can be run without a browser context). Instead, just inline cursor-continue-validity.js. It makes maintaining the tests much easier when the whole test is contained in one file (with the test harness code still in external files of course).
> 
> dgrogan@ just moved most of the IDB test logic from .html into .js files so we can run them both in a Window context and in a Worker (or SharedWorker) context. The only exceptions are tests than span Window+Worker or places where the APIs differ.

oic. That makes sense then.

[Alec Flett]

Created attachment 136941 [details]
Patch

[Alec Flett]

Comment on attachment 136538 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:175
>> +void IDBCursor::setGotValue()
> 
> This does more than just set m_gotValue. Maybe this should be called something like syncToBackendValues?

renamed to setValueReady

All other comments addressed.

[Ojan Vafai]

Comment on attachment 136941 [details]
Patch

FYI, I believe if you get an r+ then the next patch you upload you can do with "webkit-patch land-safely" and it should add it to the commit queue and use the previous r+. Not 100% sure if that works for people without commit access though.

[WebKit Review Bot]

Comment on attachment 136941 [details]
Patch

Clearing flags on attachment: 136941

Committed r114022: <http://trac.webkit.org/changeset/114022>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.