Comment on attachment 577586 [details] [diff] [review] Patch Review of attachment 577586 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

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)

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

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

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

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.