1118028 - Allow objectStores and indexes to be renamed

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

IDBObjectStore and IDBIndex need a rename function. They should only be available during versionchange transactions and should throw if the given name is already in use.

Comment 1

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

This was specced as https://w3c.github.io/IndexedDB/#dom-idbobjectstore-name and https://w3c.github.io/IndexedDB/#dom-idbindex-name.  .name is now has a setter that renames the object store/index during a version change transaction, and throws at all other times.

Comment 2

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

I'd like to take this bug to be more familiar with IndexedDB-related implementation.

Comment 3

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

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

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=228d72c5c89b

Comment 5

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

Attachment: (v1) Patch: Allow objectStores and indexes to be renamed.

r? after addressing the failure in web platform test found in comment 3.

Comment 6

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

Comment on attachment 8741101 [details] [diff] [review]
(v1) Patch: Allow objectStores and indexes to be renamed.

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

This is pretty awesome.  Mostly minor comments.

A few more edge cases you should test:

1. If I have two object stores/indexes, A and B, and I delete B, I should then be able to rename A to B.
2. If we rename an object store/index, and then the version change transaction is aborted, things should behave correctly.

For #2, something like

let req = indexedDB.open(dbName, N);
let index;
req.onupgradeneeded = function(evt) {
  let db = evt.target.result;
  let txn = evt.target.transaction;

  let os = db.objectStore(objectStoreName);
  index = os.index(indexName);
  index.name = indexName2;

  let req2 = os.put(42);
  req2.onsuccess = function() { txn.abort(); };
};
req.onerror = function() {
  req = indexedDB.open(dbName, N);
  req.onupgradeneeded = function(evt) {
    let db = evt.target.result;
    let txn = evt.target.transaction;

    let os2 = db.objectStore(objectStoreName);
    let index2 = os2.index(indexName);
    // index2.name should be indexName
    // os2.index(indexName2) should not exist
    // What does the spec say index.name should be?
  }  
};

::: dom/indexedDB/ActorsParent.cpp
@@ +15657,5 @@
> +  }
> +
> +  foundMetadata->mCommonMetadata.name() = aName;
> +
> +  RefPtr<RenameObjectStoreOp> op =

name this renameOp

@@ +15881,5 @@
> +
> +  foundIndexMetadata->mCommonMetadata.name() = aName;
> +
> +  RefPtr<RenameIndexOp> op =
> +    new RenameIndexOp(this, foundIndexMetadata, aObjectStoreId);

here too

::: dom/indexedDB/IDBDatabase.cpp
@@ +1360,5 @@
> +       objIndex < objCount;
> +       objIndex++) {
> +    const ObjectStoreSpec& objSpec = objectStores[objIndex];
> +    if (objSpec.metadata().id() == aObjectStoreId) {
> +      foundObjectStoreSpec = &objectStores[objIndex];

MOZ_ASSERT(!foundObjectStoreSpec)?  We shouldn't find two, after all.

@@ +1370,5 @@
> +  }
> +
> +  if (!foundObjectStoreSpec) {
> +    return NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR;
> +  }

Hmm, should this actually be able to happen?  Under what circumstances?

I would think the only way this could happen is if the objectStore was deleted, which you already checked for.

@@ +1373,5 @@
> +    return NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR;
> +  }
> +
> +  // Update the name of the matched object store.
> +  foundObjectStoreSpec->metadata().name() = nsAutoString(aName);

Can you pass in an nsString& to avoid this?

@@ +1402,5 @@
> +  }
> +
> +  if (!foundObjectStoreSpec) {
> +    return NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR;
> +  }

Again, can this actually happen?

@@ +1411,5 @@
> +       idxIndex < idxCount;
> +       idxIndex++) {
> +    const IndexMetadata& metadata = indexes[idxIndex];
> +    if (metadata.id() == aIndexId) {
> +      foundIndexMetadata = &indexes[idxIndex];

Similarly, MOZ_ASSERT(!foundIndexMetadata)?

@@ +1421,5 @@
> +  }
> +
> +  if (!foundIndexMetadata) {
> +    return NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR;
> +  }

And how does that happen?

@@ +1424,5 @@
> +    return NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR;
> +  }
> +
> +  // Update the name of the matched object store.
> +  foundIndexMetadata->name() = nsAutoString(aName);

here too

::: dom/indexedDB/test/unit/test_rename_index.js
@@ +16,5 @@
> +  // Rename in v1.
> +  let request = indexedDB.open(name, 1);
> +  request.onerror = errorHandler;
> +  request.onupgradeneeded = grabEventAndContinueHandler;
> +  request.onsuccess = continueToNextStep;

the typical pattern in our tests is to set this to unexpectedSuccessHandler.

@@ +36,5 @@
> +
> +  txn.oncomplete = continueToNextStep;
> +  yield undefined;
> +
> +  // Wait for onsuccess

And then here set it to continueToNextStep

@@ +51,5 @@
> +  db = event.target.result;
> +  txn = event.target.transaction;
> +
> +  objectStore = txn.objectStore(storeName);
> +  index = objectStore.index(indexName_v1);

You should also verify that objectStore.index(indexName_v0) doesn't work.

::: dom/webidl/IDBIndex.webidl
@@ +20,5 @@
>  
>  [Exposed=(Window,Worker,System)]
>  interface IDBIndex {
> +    [Throws]
> +    attribute DOMString name;

[SetterThrows] please

::: dom/webidl/IDBObjectStore.webidl
@@ +14,5 @@
>  
>  [Exposed=(Window,Worker,System)]
>  interface IDBObjectStore {
> +    [Throws]
> +    attribute DOMString name;

ibid

Comment 7

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

Comment on attachment 8741101 [details] [diff] [review]
(v1) Patch: Allow objectStores and indexes to be renamed.

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

Thanks for the suggestion of covering the edge cases.

::: dom/indexedDB/IDBDatabase.cpp
@@ +1370,5 @@
> +  }
> +
> +  if (!foundObjectStoreSpec) {
> +    return NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR;
> +  }

No, only if the aOBjectStoreId is illegal.
We should put MOZ_ASSERT(!foundObjectStoreSpec) in the for loop instead.

@@ +1402,5 @@
> +  }
> +
> +  if (!foundObjectStoreSpec) {
> +    return NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR;
> +  }

ditto.

@@ +1421,5 @@
> +  }
> +
> +  if (!foundIndexMetadata) {
> +    return NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR;
> +  }

No, only if the aIndexed is illegal.
We should put MOZ_ASSERT(!foundIndexMetadata) in the for loop instead.

::: dom/webidl/IDBIndex.webidl
@@ +20,5 @@
>  
>  [Exposed=(Window,Worker,System)]
>  interface IDBIndex {
> +    [Throws]
> +    attribute DOMString name;

Will do.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=079cd36f8663

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c111b42836d

Comment 10

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

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #9)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c111b42836d

Something is tricky if we want to test the abortion in version change transaction without being verdict FAIL:
> var request;
> request = indexedDB.open(name, 1);
> request.onupgradeneeded = function (event) {
>     db = event.target.result;
>     txn = event.target.transaction;
>     txn.abort();
> }
> request.onerror = function (event) {
>     console.log("onerror");
> }
we'll got AbortError at window.onerror() or worker.onerror() after txn.abort() is executed which is expected according to the IDB spec and our implementation but it is annoying when running the test case.
To resolve this problem, I've wrapped expectUncaughtException() from SimpleTest to helper.js to suppress this uncaught exception when needed.

Comment 11

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

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #10)
> we'll got AbortError at window.onerror() or worker.onerror() after
> txn.abort() is executed which is expected according to the IDB spec and our
> implementation but it is annoying when running the test case.
Note: The error to the global object is dispatched at IndexedDatabaseManager::CommonPostHandleEvent()[1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/IndexedDatabaseManager.cpp#497-534

Comment 12

[Jan Varga [:janv]]

I think you can just these calls to the error handler add:
event.preventDefault();
event.stopPropagation();

or use the expectedErrorHandler() or new ExpectError() defined in helpers.js
Take a look at other tests how it's done.

Comment 13

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

(In reply to Jan Varga [:janv] from comment #12)
> I think you can just these calls to the error handler add:
> event.preventDefault();
> event.stopPropagation();
> 
> or use the expectedErrorHandler() or new ExpectError() defined in helpers.js
> Take a look at other tests how it's done.

Thanks for pointing out this!
I made a mistake by setting onerror to |continueToNextStep| in previous patch which causes this error raised at globalObject.onerror().

After replacing with |expectedErrorHandler("AbortError")|, the test case works now without specifying |expectUncaughtException()|. :)

Comment 14

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34f1462f3bdc

Comment 15

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

Attachment: (v2) Patch: Allow objectStores and indexes to be renamed.

1. Address the comments in the implementation.
2. Add test coverage for the edge cases.

Comment 16

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

Comment on attachment 8746413 [details] [diff] [review]
(v2) Patch: Allow objectStores and indexes to be renamed.

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

::: dom/indexedDB/IDBIndex.cpp
@@ +175,5 @@
> +  const LoggingString loggingOldIndex(this);
> +
> +  const int64_t indexId = Id();
> +
> +  const nsAutoString name = nsAutoString(aName);

If this is really necessary, you ought to be able to get away with nsString instead of nsAutoString.

But why can't we just send const nsAString& through everything?

Comment 17

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

Sorry for not explaining this clearly while providing the patch. :p
(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #16)
> If this is really necessary, you ought to be able to get away with nsString
> instead of nsAutoString.
"nsAutoString" is recommended in MDN with stack-based string allocation(64 char-buff) if we need a nsString instance as a local variable[1].
Please correct me if it's not proper here. :p
> But why can't we just send const nsAString& through everything?
Unfortunately, all the names of the IPDL {Index|ObjectStore}Metadata structures and the arguments we are going to invoke requires "nsString" as input.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#Local_variables

Comment 18

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

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #17)
> Sorry for not explaining this clearly while providing the patch. :p
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #16)
> > If this is really necessary, you ought to be able to get away with nsString
> > instead of nsAutoString.
> "nsAutoString" is recommended in MDN with stack-based string allocation(64
> char-buff) if we need a nsString instance as a local variable[1].
> Please correct me if it's not proper here. :p

Ah.  So, the distinction is this:

Class::GetFoo(nsAString& string) {
  string.AssignLiteral("myString");
}

Class::GetBar(nsAString& string) {
  string = mMyVariableOfTypensStringThatHasMyStringInIt;
}

Class::Test() {
  nsAutoString autoString;
  nsString regularString;

  GetFoo(autoString);    // Does not malloc an nsStringBuffer, because nsAutoString provides stack space.
  GetFoo(regularString); // Does malloc an nsStringBuffer, because we have no place to store the result otherwise.

  GetBar(autoString);    // Increments the reference count on the existing nsStringBuffer inside mBlahBlahBlah, wastes the stack space.
  GetBar(regularString); // Increments the reference count on the existing nsStringBuffer inside mBlahBlahBlah.
}

In other words, nsAutoString is more efficient if we'd otherwise have to *create* a string buffer.  But if we already have a string that stores the text we care about and we're just copying it then it doesn't matter.

> > But why can't we just send const nsAString& through everything?
> Unfortunately, all the names of the IPDL {Index|ObjectStore}Metadata
> structures and the arguments we are going to invoke requires "nsString" as
> input.

Ok.

Comment 19

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57409de8690e

Comment 20

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

Attachment: v(3) Patch: Allow objectStores and indexes to be renamed. r=khuey

address the improper use of nsAutoString at comment 18.

Comment 21

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

treeherder result in comment 19 looks good!

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/949543040bb6

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/949543040bb6

Comment 24

[Eric Shepherd [:sheppy]]

Updated pages:

https://developer.mozilla.org/en-US/docs/Web/API/IDBObjectStore
https://developer.mozilla.org/en-US/docs/Web/API/IDBObjectStore/name
https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex
https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex/name

And updated Firefox 49 for developers.

Comment 25

[Eric Shepherd [:sheppy]]

I just noticed I forgot to include the exception lists for these; working on that now.