1357561 - ConstraintError should mention which constraint is not being satisfied

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect, P3)

Description

[Markus Stange [:mstange]]

This is a really generic error and I'm having a very hard time finding out what's actually happening. It would be really useful to know:
 - Which index is imposing the constraint
 - What the colliding values are

Comment 1

[Markus Stange [:mstange]]

In my case, the problem was that I thought I was modifying an existing record but I was actually adding a new record. And that's because I had an autoIncrement primary key and didn't supply that primary key to the objectStore.put() function.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Let's see if :asuth or :bevis has some ideas to extract more useful info here.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Expanding context: The existing error message is defined at http://searchfox.org/mozilla-central/source/dom/base/domerr.msg#61 and is "A mutation operation in the transaction failed because a constraint was not satisfied. For example, an object such as an object store or index already exists and a new one was being attempted to be created."

The NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR error in :mstage's specific case is just surfacing the underlying NS_ERROR_STORAGE_CONSTRAINT error mozStorage mapped from the SQLITE_CONSTRAINT error SQLite error reported when the primary key constraint on the "unique_index_data" table was violated.

The good news is that we can break this error into multiple errors and improve things immensely.  Specifically, if we check http://searchfox.org/mozilla-central/search?q=NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR we can see that IDBDatabase::{CreateObjectStore,RenameObjectStore,RenameIndex} all manually return NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR when they find the target name already exists.  We could add an additional error or errors in domerr.msg and return those instead in these cases.

For the duplicate index value case that I suspect is coming from DatabaseOperationBase::InsertIndexTableRows, we can create a more specific error that says something like "Index uniqueness constraint violated.  Maybe you are adding a new record when you meant to update an existing record?"

Or if one got reallllly fancy... DatabaseOperationBase::InsertIndexTableRows knows the index experiencing a duplicate value and the duplicate value (encoded as a Key: http://searchfox.org/mozilla-central/source/dom/indexedDB/Key.cpp).  The index name and value could get propagated back to the content process as a new RequestResponse IPDL struct in the union.  Options at that point are:
A) A spec change to expose the information visible to script.
B) Log a rich console message to the devtools console that include the value as an actual inspectable JS Value.
For reasons of laziness and user privacy reasons, I'd recommend going for the console message.  We don't want the content of a user's database accidentally exposed via error messages to reporting systems or potentially adversarial code communicating with the actual origin that owns the database via postMessage(), etc.  It's reasonable to bias the errors to require devtools-level access.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Thanks :asuth for this detailed explanation.

Comment 5

[Diego Pino]

Attachment: Bug-1357561-ConstraintError-should-mention-which-con.patch

Here's an attempt to solving this bug.

What I've done is to refine the constraint messages for failures such as rename index and rename object store. As for other constraint errors, such as duplicated index name or duplicated object store name, what I did was returning the generic ConstraintError, but adding a message providing information about the name and the db's index that collided.

There are two other NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR that I left unchanged. One is caused by NS_ERROR_STORAGE_CONSTRAINT in ActorsParent.cpp:ClampResultCode() and another one when 'autoIncrementNum' overflows in ActorsParent.cpp:ObjectStoreAddOrPutRequestOp::DoDatabaseWork().

I haven't tested the code though, although I've successfully built Firefox. How can I reproduce a test case as the one stated in the bug description?

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9009929 [details] [diff] [review]
Bug-1357561-ConstraintError-should-mention-which-con.patch

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

Thank you very much for your continued efforts to improve our IndexedDB error logging!  Bug 1357636 was a very nice improvement!

Deferring review to Tom.  I suppose this should be a feedback? request for now if tests are planned.

Test-wise, our mochitest test infrastructure has a means of watching console messages.  We have some existing ServiceWorker tests that can be used as a basis.  Specifically, https://searchfox.org/mozilla-central/source/dom/serviceworkers/test/test_error_reporting.html uses the helpers at https://searchfox.org/mozilla-central/source/dom/serviceworkers/test/error_reporting_helpers.js which builds on top of https://searchfox.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#1243.

Mochitest documentation can be found at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest.

Comment 7

[Tom Tung [:tt, :ttung]]

Comment on attachment 9009929 [details] [diff] [review]
Bug-1357561-ConstraintError-should-mention-which-con.patch

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

Looks good!

There are just a few nits for indentation and the way for converting a nsString to nsCString. 
Give an f+ for now since it'd be better to have a mochitest test to verify it. Thank you for the patch!

::: dom/indexedDB/IDBDatabase.cpp
@@ +449,5 @@
>         index < count;
>         index++) {
>      if (aName == objectStores[index].metadata().name()) {
> +      aRv.ThrowDOMException(NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR,
> +          nsPrintfCString(

nits: Two spaces for an indentation [1]. 
Please apply that to here and below.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +451,5 @@
>      if (aName == objectStores[index].metadata().name()) {
> +      aRv.ThrowDOMException(NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR,
> +          nsPrintfCString(
> +              "Object store named '%s' already exists at index '%u':",
> +              ToNewCString(aName), index));

You probably don't want to always copy a whole string while converting it. I would suggest using NS_ConvertUTF16toUTF8(). You can learn more in [2] [3].

[2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings
[3] https://searchfox.org/mozilla-central/source/xpcom/string/nsString.h#96

::: dom/indexedDB/IDBObjectStore.cpp
@@ +2239,5 @@
>         index < count;
>         index++) {
>      if (aName == indexes[index].name()) {
> +      aRv.ThrowDOMException(NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR,
> +          nsPrintfCString("Index named '%s' already exists at index '%u':",

nits: two spaces for an indentation

@@ +2240,5 @@
>         index++) {
>      if (aName == indexes[index].name()) {
> +      aRv.ThrowDOMException(NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR,
> +          nsPrintfCString("Index named '%s' already exists at index '%u':",
> +              ToNewCString(aName), index));

Same as here, you probably don't want to always make an extra copy of whole string while converting it.

Also, please align the first argument in this line with the beginning of the string like [4].

[4] https://searchfox.org/mozilla-central/source/dom/indexedDB/IDBObjectStore.cpp#1689-1691

Comment 8

[Diego Pino]

Attachment: Bug-1357561-ConstraintError-should-mention-which-con.patch

I amended the format errors in the previous patch and the string cast.

Regarding tests, I've added two new test cases to two already existing tests. They test whether an attempt to create a duplicated object store fails, as well as an attempt to create an index.

Regarding the two other errors modified by this patch (rename index and rename object store name), there are already test cases for this. The test cases validate whether a ConstraintError is thrown (https://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/test_rename_index_errors.js#60), and do not check the error message. I think that's the right thing to do and that's why I didn't check the error message in the new test cases.

Comment 9

[Diego Pino]

Attachment: Bug-1357561-ConstraintError-should-mention-which-con.patch

Comment 10

[Tom Tung [:tt, :ttung]]

Comment on attachment 9010601 [details] [diff] [review]
Bug-1357561-ConstraintError-should-mention-which-con.patch

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

Thank you for the patch again! This patch looks great and the test does ensure indexedDB throws ConstraintError in certain conditions, but I'm not really sure about the test is well enough to verify the behavior.

It's rather nice to have a test to verify indexedDB does throw exceptions for ConstraintError while attempting to create a duplicated objectStore and create a duplicated index.
However, because your patch is mainly to make users understand the difference between them, it's better to have another test to verify the content of error reports are different. Given they are good for ensuring reporting the ConstraintError but they are not really related to this change, I'd suggest splitting changes in test_create_index.js and test_create_objectStore.js to another patch.

If you are interested in having a test to verify the behavior, you can refer to [1] [2] as Andrew suggested.

To do that you might want to have an HTML document like the original test [3] which was modified by you in this patch and a javascript file accompanies with it. The HTML will just simply load the js file.
In the js file, you will create a function in dom/indexedDB/test/ with similar logic as expect_console_message() in [2] other than the main testing logic. Please note that you need to add all the new file name into mochitest.ini (https://searchfox.org/mozilla-central/source/dom/indexedDB/test/mochitest.ini) so that they can be recognized.
As for the main testing logic, I guess you probably want to have a test with the following steps:
- Steup environment
- Use indexedDB operations to make an exception (create a duplicated objectStore for one run and create a duplicated index for the other run)
- Verify the error report does match what you added in your patch via expect_console_message()

Drop the review request for now since the test doesn't really verify the changes work as expected.
Please let me know if there is something confuse you or something not really clear. I'd love to help you. Thanks!

[1] https://searchfox.org/mozilla-central/source/dom/serviceworkers/test/test_error_reporting.html
[2] https://searchfox.org/mozilla-central/source/dom/serviceworkers/test/error_reporting_helpers.js
[3] https://searchfox.org/mozilla-central/source/dom/indexedDB/test/test_create_index.html

Comment 11

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #10)
Sorry for not explaining that clear and the typo.

> In the js file, you will create a function in dom/indexedDB/test/ with
Js file should live in dom/indexedDB/test/unit/ folder and you will create a utility function in it with similar logic as expect_console_message(),...

Comment 12

[Diego Pino]

Attachment: Bug-1357561-Test-error-messages.patch

I have coded a test following the ServiceWorker style (using `expect_console_message`, etc). The test is still work in progress (there's only one message covered so far). I'm not totally sure about whether is looking good and would like to ask for feedback.

Some of the things I've doubts about are:

* `expect_console_message` is meant to be used in an async function, so the test is coded in a different manner than the other IndexedDB tests.
* the original `expect_console_message` from ServiceWorkers, expects a msgId which it's used to fetch a localized string from `chrome://global/locale/dom/dom.properties`. I don't know whether it would be possible to fetch an error string from `dom/base/domerr.msg`? As for now, I left the string hard coded in the test.
* If the error string is going to be hard coded, it's more immediate to simply check in the `try/catch` whether error.message equals the expected error message string. That wouldn't require using `expect_console_message` so the test doesn't need to be async and can be coded as the other IndexedDB tests.

Comment 13

[Tom Tung [:tt, :ttung]]

Comment on attachment 9010966 [details] [diff] [review]
Bug-1357561-Test-error-messages.patch

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

I haven't checked if the expect_console_message() works or not, but I think you are in the right direction.
Probably changing the async/await to yield(generator). Also, please add another js file under unit folder. Thanks!

::: dom/indexedDB/test/mochitest.ini
@@ +263,5 @@
>  [test_unique_index_update.html]
>  [test_upgrade_add_index.html]
>  [test_view_put_get_values.html]
>  [test_wasm_put_get_values.html]
> +[test_constraint_error_messages.html]

Please add the test in alphabetical order.

::: dom/indexedDB/test/test_constraint_error_messages.html
@@ +8,5 @@
> +  <meta http-equiv="Content-type" content="text/html;charset=UTF-8">
> +</head>
> +<body>
> +
> +<script type="text/javascript">

Please put the script to another js file in the same name and place the js file under the "dom/indexedDB/test/unit/"

@@ +15,5 @@
> +let messages = {
> +  "ERROR_DOM_INDEXEDDB_DUPLICATED_OBJECT_STORE_ERR": "Object store named 'foo' already exists at index '0':",
> +};
> +
> +function expect_console_message(msgId) {

Maybe rewrite this function to a generator function (function*)

@@ +28,5 @@
> +  SimpleTest.endMonitorConsole();
> +  return expectedPromise;
> +}
> +
> +add_task(async function test() {

Maybe use |function* testSteps() {| rather async function. Currently, all mochitests in indexedDB use that, it's better to follow the same coding style to make it easier to be maintained. 

You can see more about function* in [1]

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function*

@@ +36,5 @@
> +  const objectStoreInfo = [
> +    { name: "1", options: { keyPath: null } },
> +  ];
> +
> +  let db = await new Promise(resolve => {

You can yield a promise to achieve the synchronous stuff.

@@ +40,5 @@
> +  let db = await new Promise(resolve => {
> +    let request = indexedDB.open(name);
> +    request.onupgradeneeded = evt => {
> +      resolve(evt.target.result);
> +    }

Maybe handle other events to capture unexcepted errors.

@@ +45,5 @@
> +  });
> +
> +  try {
> +    db.createObjectStore("foo");
> +    db.createObjectStore("foo");

Maybe have an assertion like |ok(false, error msg)| here to avoid being timeout if there is an unexcepted error occur?

@@ +50,5 @@
> +  } catch (e) {
> +    console.log(e.message);
> +  }
> +
> +  await wait_for_expected_message(expectedMessage);

You might want to have a condition to decide whether the test is completed (error message did pop up correctly) or not.

Comment 14

[Tom Tung [:tt, :ttung]]

(In reply to Diego Pino from comment #12)
> * `expect_console_message` is meant to be used in an async function, so the
> test is coded in a different manner than the other IndexedDB tests.

Actually, the APIs in indexedDB tests are async as well. The tests in indexedDB use yield/generators [1] to make testing logic synchronous while tests in ServiceWorker are using promise or async/await.

I guess you could use "yield" to wait for a promise to achieve the same thing.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function*
[2] https://searchfox.org/mozilla-central/source/dom/promise/tests/test_promise_utils.html#58

> * the original `expect_console_message` from ServiceWorkers, expects a msgId
> which it's used to fetch a localized string from
> `chrome://global/locale/dom/dom.properties`. I don't know whether it would
> be possible to fetch an error string from `dom/base/domerr.msg`? As for now,
> I left the string hard coded in the test.

I'm not really sure about that, but I don't think the same way to do with `dom/base/domerr.msg` will work. You might need to get the file in another way if you want to make it general, but for now, leaving the string hardcoded in the test with a comment to explain that is fine.

> * If the error string is going to be hard coded, it's more immediate to
> simply check in the `try/catch` whether error.message equals the expected
> error message string. That wouldn't require using `expect_console_message`
> so the test doesn't need to be async and can be coded as the other IndexedDB
> tests.

If you can get the whole error message from a try/catch and check the content of that message is the same as what you modified, then it's fair enough to just do that here.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

As an aside: IndexedDB is long overdue for a conversion from the existing "function*" generator usage to "async function" usage.  (Note to mention some tests that aren't even properly using generator functions).  I haven't taken a look at the test yet, but I'd think that as long as the test is correct given how the existing helpers assume generator-functions, it's okay to start making the transition sooner rather than later.

Comment 16

[Tom Tung [:tt, :ttung]]

(In reply to Andrew Sutherland [:asuth] from comment #15)
> As an aside: IndexedDB is long overdue for a conversion from the existing
> "function*" generator usage to "async function" usage.  (Note to mention
> some tests that aren't even properly using generator functions).  I haven't
> taken a look at the test yet, but I'd think that as long as the test is
> correct given how the existing helpers assume generator-functions, it's okay
> to start making the transition sooner rather than later.

That's a good point! From the aspect of making the transition sooner, I'm fine with using async/await.

Diego, sorry for making you confused, but please ignore my suggestions about using generator-functions.

Comment 17

[Diego Pino]

Attachment: Bug-1357561-ConstraintError-should-mention-which-con.patch

Finally I had some time to rework the patch.

I merged both patches into one. I undid the changes in existing tests and create reworked the new constraint error test that validates messages on duplicated object stores and indexes. There are already existing tests for renaming object stores and indexes, so I didn't add those cases again.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9013723 [details] [diff] [review]
Bug-1357561-ConstraintError-should-mention-which-con.patch

(Tom's review is enough.)

Comment 19

[Tom Tung [:tt, :ttung]]

Comment on attachment 9013723 [details] [diff] [review]
Bug-1357561-ConstraintError-should-mention-which-con.patch

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

The testing logic looks correct and it works fine! Thanks! However, there are a few places can be improved so that it could be easier to be debugged.

It's better to handle all the events (e.g. onerror, onupgradeneeded, and onsuccess) for all actions/operations (e.g. db.open(), createObjectStore(), createIndex(), ... etc) so that we can catch the problem quicker if they have some issues. It provides more information when there is something wrong with those operations fail. Besides, having log (e.g. info("xxx")) can also help to accelerate the time for figuring out the state for problems happened.

Based on that, I guess I would like to see it again.

::: dom/indexedDB/test/unit/test_constraint_error_messages.js
@@ +6,5 @@
> +var testGenerator = testSteps();
> +
> +function* testSteps()
> +{
> +  const name = this.window ? window.location.pathname : "Splendid Test";

Nit: Could you add an info here like the other tests [1]? So that it would be easier to debug.
For instance,

info("Opening database");

[1] https://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/test_file_copy_failure.js#14

@@ +11,5 @@
> +  let request = indexedDB.open(name);
> +  request.onerror = errorHandler;
> +  request.onupgradeneeded = grabEventAndContinueHandler;
> +  request.onsuccess = unexpectedSuccessHandler;
> +  let event = yield undefined;

Nit: Add an empty here

@@ +14,5 @@
> +  request.onsuccess = unexpectedSuccessHandler;
> +  let event = yield undefined;
> +  let db = event.target.result;
> +
> +  // Create object store.

Nit: Maybe have an info here like:

info("Creating objectStore");

rather than a comment inside the code?

@@ +15,5 @@
> +  let event = yield undefined;
> +  let db = event.target.result;
> +
> +  // Create object store.
> +  let objectStore = db.createObjectStore('foo');

Perhaps, you can have a const variable for the object store name and put it at the beginning of the function so that it could be a bit more generic.
For instance,

// beginning of function
const objectStoreName = "foo";

// here
let objectStore = db.createObjectStore(objectStoreName);

Also, please handle the error cases of creating an object store.

@@ +17,5 @@
> +
> +  // Create object store.
> +  let objectStore = db.createObjectStore('foo');
> +
> +  // Attempt to create duplicated object store should return an error.

Nit: Maybe have an info:

info("Creating a duplicated object store to get an error");

@@ +19,5 @@
> +  let objectStore = db.createObjectStore('foo');
> +
> +  // Attempt to create duplicated object store should return an error.
> +  try {
> +    db.createObjectStore('foo');

So that here can be:

db.createObjectStore(objectStoreName);

Also, I guess you need to handle the success event of this as well.

@@ +21,5 @@
> +  // Attempt to create duplicated object store should return an error.
> +  try {
> +    db.createObjectStore('foo');
> +    ok(false, "ConstraintError should be thrown if object store already exists");
> +  } catch (e) {

Nit: You can add an assertion here, like:

ok(true, "ConstraintError should be thrown if object store already exists");

@@ +22,5 @@
> +  try {
> +    db.createObjectStore('foo');
> +    ok(false, "ConstraintError should be thrown if object store already exists");
> +  } catch (e) {
> +    is(e.message, "Object store named 'foo' already exists at index '0'");

Nit: I'd prefer having a log here, like:

is(e.message, "Object store named 'foo' already exists at index '0'", "Threw with correct error message");

Also, you can replace the literal string 'foo' with the variable declared above.

@@ +25,5 @@
> +  } catch (e) {
> +    is(e.message, "Object store named 'foo' already exists at index '0'");
> +  }
> +
> +  // Attempt to create duplicated indexes should return an error.

Maybe have a info here:

info("Creating a duplicated indexes to verify the error message");

@@ +27,5 @@
> +  }
> +
> +  // Attempt to create duplicated indexes should return an error.
> +  try {
> +    objectStore.createIndex('bar', 'bar');

So, only the second objectStore should get an error. You probably want to put this out of try-catch. Also, please handle the error cases of creating an index. Furthermore, you can replace the literal string 'bar' with a constant local variable since we want to check the message with that value.

@@ +28,5 @@
> +
> +  // Attempt to create duplicated indexes should return an error.
> +  try {
> +    objectStore.createIndex('bar', 'bar');
> +    objectStore.createIndex('bar', 'bar');

I guess you need to handle events of this action as well.

@@ +30,5 @@
> +  try {
> +    objectStore.createIndex('bar', 'bar');
> +    objectStore.createIndex('bar', 'bar');
> +    ok(false, "ConstraintError should be thrown if index already exists");
> +  } catch (e) {

Nit: Same as above, you can add an assertion here:

ok(true, "ConstraintError should be thrown if index already exists");

@@ +31,5 @@
> +    objectStore.createIndex('bar', 'bar');
> +    objectStore.createIndex('bar', 'bar');
> +    ok(false, "ConstraintError should be thrown if index already exists");
> +  } catch (e) {
> +    is(e.message, "Index named 'bar' already exists at index '0'");

Nit: I'd prefer having a log here, like:
 
is(e.message, "Index named \'" + indexName + "\' already exists at index '0'", "Threw with correct error message");

Comment 20

[Diego Pino]

Attachment: Bug-1357561-ConstraintError-should-mention-which-con.patch

Updated patch. Addressed comments from previous patch.

Comment 21

[Tom Tung [:tt, :ttung]]

Comment on attachment 9015552 [details] [diff] [review]
Bug-1357561-ConstraintError-should-mention-which-con.patch

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

Thanks for the patch and sorry for the confusion!

I'll provide an inter-diff patch for you to refer to. r=me if you refer that and address the nits.

After merging the patch, I think we almost finish it. The next step will be:
- Open a follow-up bug for using async/await syntax to replace with test generator syntax for "test_constraint_error_messages.js". Of course, you can do that here if you want it, but it might need to change some utility js function in dom/indexedDB/test/helper.js [1] like [2].
- Providing more context in the commit message. Please have a few more sentences to describe why do you provide this patch and how do you do that.
- Push the patch to try. Please upload the merged patch, and then I can help you push it to try.

[1] https://searchfox.org/mozilla-central/source/dom/indexedDB/test/helpers.js#213
[2] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=1493908&attachment=9012728

::: dom/indexedDB/test/unit/test_constraint_error_messages.js
@@ +23,5 @@
> +
> +  info("Creating objectStore");
> +
> +  let objectStore = db.createObjectStore(objectStoreName);
> +  request.onerror = errorHandler;

Sorry for the confusion, I thought that creating an objectStore will trigger an event to the IDBRequest, but I was wrong. Therefore, you don't need to handle the events here and below. Will attach an inter-diff patch for you to reference.

@@ +26,5 @@
> +  let objectStore = db.createObjectStore(objectStoreName);
> +  request.onerror = errorHandler;
> +  request.onsuccess = grabEventAndContinueHandler;
> +
> +  info("Creating a duplicated object store to get an error");

nit: one more empty line below.

@@ +30,5 @@
> +  info("Creating a duplicated object store to get an error");
> +  try {
> +    db.createObjectStore(objectStoreName);
> +    request.onsuccess = grabEventAndContinueHandler;
> +    ok(false, "ConstraintError should be thrown if object store already exists");

nit: please don't exceed 80 characters for each line.
I guess you could:
ok(false,
   "ConstraintError should be thrown if object store already exists");

@@ +33,5 @@
> +    request.onsuccess = grabEventAndContinueHandler;
> +    ok(false, "ConstraintError should be thrown if object store already exists");
> +  } catch (e) {
> +    ok(true, "ConstraintError should be thrown if object store already exists");
> +    is(e.message, "Object store named '" + objectStoreName + "' already exists at index '0'", "Threw with correct error message");

nit: please don't exceed 80 characters for each line.
You can do something like:
is(e.message,
   "Object store named '" + objectStoreName + "' already exists at index '0'",
   "Threw with correct error message");
 

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code

@@ +36,5 @@
> +    ok(true, "ConstraintError should be thrown if object store already exists");
> +    is(e.message, "Object store named '" + objectStoreName + "' already exists at index '0'", "Threw with correct error message");
> +  }
> +
> +  info("Creating a duplicated indexes to verify the error message");

nit: one more empty line below.

@@ +48,5 @@
> +    is(e.message, "Index named '" + indexName + "' already exists at index '0'", "Threw with correct error message");
> +  }
> +
> +  request.onsuccess = grabEventAndContinueHandler;
> +  yield undefined;

Maybe close the db here

Comment 22

[Tom Tung [:tt, :ttung]]

Attachment: interdiff.patch

Comment 23

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #21)
Also please change the format of the first line of commit message to align with others, like:
Bug XXX - xxxxxxxx; r=oo

Comment 24

[Tom Tung [:tt, :ttung]]

Andrew, you suggested start using async/await syntax in comment 15, but I reckon it might not be a trivial task for indexedDB. What about doing that in the follow-up bug since this bug is tagged with good-first-bug?

Also, just for the double confirm, although you mentioned that my review would be enough, I am not an IndexedDB peer. Would that be okay to land the final patch in this bug without your or :janv's review?

Comment 25

[Diego Pino]

Attachment: Bug-1357561-ConstraintError-should-mention-which-con.patch

Thanks for the review. I applied the interdiff patch and improved the commit message, as suggested.

Comment 26

[Diego Pino]

Treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc8f986d27f9554b34a13e4e9c70e4af7a19687e

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

Agreed that this is not the bug to transition IndexedDB.  As I understand it, peers/owners are allowed (and encouraged) to defer reviews to other people so that those people can gain more experience in the module/etc.  In comment 6 I deferred review to you which should be good enough, but I appreciate your request for clarification.  You'll soon be an IDB peer if Jan didn't already realize he should add you already! ;)

Comment 28

[Tom Tung [:tt, :ttung]]

(In reply to Diego Pino from comment #26)
> Treeherder:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=dc8f986d27f9554b34a13e4e9c70e4af7a19687e

Oh, thanks for the try push! However, it'd be better if you can just use a specific syntax (You can learn more here [1] [2]) to save resource on try next time. For instance, your patch should only affect indexedDB stuff and it's an FF-only modification, so I guess you could have something like:

./mach try -b do -p all -u mochitests --and dom/indexedDB

[1] https://mozilla-releng.net/trychooser/
[2] https://wiki.mozilla.org/ReleaseEngineering/TryServer

Comment 29

[Tom Tung [:tt, :ttung]]

(In reply to Andrew Sutherland [:asuth] from comment #27)
> Agreed that this is not the bug to transition IndexedDB.  As I understand
> it, peers/owners are allowed (and encouraged) to defer reviews to other
> people so that those people can gain more experience in the module/etc.  In
> comment 6 I deferred review to you which should be good enough, but I
> appreciate your request for clarification.  You'll soon be an IDB peer if
> Jan didn't already realize he should add you already! ;)

I see! Thanks!

Comment 30

[Tom Tung [:tt, :ttung]]

Comment on attachment 9015925 [details] [diff] [review]
Bug-1357561-ConstraintError-should-mention-which-con.patch

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

Looks great! Thanks!

Could you open a follow bug to track changing the current generator/yield to async/await for this test and mark that it depends on this issue?

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0c617e2d351
ConstraintError should mention which constraint is not being satisfied. r=tt

Comment 32

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d0c617e2d351