Closed Bug 706088 Opened 12 years ago Closed 12 years ago

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


(Core :: Storage: IndexedDB, defect)

Not set





(Reporter: khuey, Assigned: khuey)




(2 files)

Attached patch PatchSplinter Review
      No description provided.
Attachment #577586 - Flags: review?(bent.mozilla)
Comment on attachment 577586 [details] [diff] [review]

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

Looks good!
Attachment #577586 - Flags: review?(bent.mozilla) → review+
Comment on attachment 577586 [details] [diff] [review]

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:

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)
Attached patch TestsSplinter Review
This tests a lot of combinations. All the ones I could think of.
Attachment #579012 - Flags: review?(khuey)
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Attachment #579012 - Flags: review?(khuey) → review+
Comment on attachment 579012 [details] [diff] [review]

Was landed on inbound:

Then backed out again for M2 permaorange:
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)
Attachment #579012 - Flags: checkin-
Checked in test

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.