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

RESOLVED FIXED

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 577586 [details] [diff] [review]
Patch
Attachment #577586 - Flags: review?(bent.mozilla)
Comment on attachment 577586 [details] [diff] [review]
Patch

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

Looks good!
Attachment #577586 - Flags: review?(bent.mozilla) → review+
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?
Yeah I don't really understand why that was there to begin with.  Bent?
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.
Yeah, we need more comprehensive tests.

I'm going to remove aOverwrite from the conditional.
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)
Created attachment 579012 [details] [diff] [review]
Tests

This tests a lot of combinations. All the ones I could think of.
Attachment #579012 - Flags: review?(khuey)
I landed the fix on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6393012a8cf2

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/6393012a8cf2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Attachment #579012 - Flags: review?(khuey) → review+
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
Attachment #579012 - Flags: checkin-
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.
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla11 → ---
You need to log in before you can comment on or make changes to this bug.