706088 - IndexedDB: Put and autoIncrement don't get along very well.

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 577586 [details] [diff] [review]
Patch

Review of attachment 577586 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 577586 [details] [diff] [review]
Patch

Review of attachment 577586 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1068,5 @@
>      return rv;
>    }
>  
> +  // Put requires a key, unless this is an autoIncrementing objectStore.
> +  if (aOverwrite && !mAutoIncrement && key.IsUnset()) {

The aOverwrite test here seems wrong?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah I don't really understand why that was there to begin with.  Bent?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We really should have tests for all combinations of:

put/add
keypath/no keypath/keypath but no value at that property
provide explicit key argument/don't provide explicit key argument
autoincrement/no autoincrement

so that's a total of 24 combinations.

Also note that in all instances providing an explicit key argument but setting it to undefined should behave the same as not providing an explicit key argument.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, we need more comprehensive tests.

I'm going to remove aOverwrite from the conditional.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It appears that this patch as-is makes us pass all variants of add/put, so I think we should just land it.

I'm planning on doing some cleanup around add/put which should simplify the code and give us better performance.

Attaching a testcase which shows us passing everything except our quirky autoincrement behavior (objectstores share generator)

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Tests

This tests a lot of combinations. All the ones I could think of.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I landed the fix on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6393012a8cf2

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6393012a8cf2

Comment 10

[Ed Morley [:emorley]]

Comment on attachment 579012 [details] [diff] [review]
Tests

Was landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e479e03eaa71

Then backed out again for M2 permaorange:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e479e03eaa71
https://tbpl.mozilla.org/php/getParsedLog.php?id=7821279&tree=Mozilla-Inbound
{
TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_writer_starvation.html | application timed out after 330 seconds with no output
/test/test_writer_starvation.html, followed by 168504 other errors PROCESS-CRASH | /tests/dom/indexedDB/test/test_writer_starvation.html | application crashed (minidump found)
Thread 0 (crashed)
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/a050fbb24950

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in test

https://hg.mozilla.org/mozilla-central/rev/0414fe2f9d73

I also removed a test which is a subset of the new test. Got r=bent over irc for that.