Closed Bug 1118028 Opened 9 years ago Closed 8 years ago

Allow objectStores and indexes to be renamed

Categories

(Core :: Storage: IndexedDB, defect)

Product:
Core
Component:
Storage: IndexedDB
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: bent.mozilla, Assigned: bevis, Mentored)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [tw-dom] btpp-active)

Attachments

(1 file, 2 obsolete files)

v(3) Patch: Allow objectStores and indexes to be renamed. r=khuey
58.54 KB, patch
bevis
: review+
Details | Diff | Splinter Review 
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.
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.
Whiteboard: [tw-dom]
I'd like to take this bug to be more familiar with IndexedDB-related implementation.
Assignee: nobody → btseng
Whiteboard: [tw-dom] → [tw-dom] btpp-active
r? after addressing the failure in web platform test found in comment 3.
Attachment #8741101 - Flags: review?(khuey)
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
Attachment #8741101 - Flags: review?(khuey)
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.
Status: NEW → ASSIGNED
(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.
(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
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.
(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()|. :)
1. Address the comments in the implementation.
2. Add test coverage for the edge cases.
Attachment #8741101 - Attachment is obsolete: true
Attachment #8746413 - Flags: review?(khuey)
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?
Attachment #8746413 - Flags: review?(khuey) → review+
Sorry for not explaining this clearly while providing the patch. :p
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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
(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] (khuey@mozilla.com) 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.
address the improper use of nsAutoString at comment 18.
Attachment #8746413 - Attachment is obsolete: true
Attachment #8748467 - Flags: review+
treeherder result in comment 19 looks good!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/949543040bb6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1271497
I just noticed I forgot to include the exception lists for these; working on that now.
Depends on: 1309527
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: