1404276 - Support index key extraction if compounded with |autoIncrement| primary key. [Indexedb does not fill indices with auto-incremented fields / keys] WPT failure: idbobjectstore_createIndex15-autoincrement.htm

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, enhancement, P2)

Description

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

This is a follow-up of bug 1178829 comment 6:
Index key is extracted in the content processes but the auto-incremented primary key is generated later in the parent side, so the object can be stored in the IDBObjectStore but is ignored in the IDBIndex:
http://searchfox.org/mozilla-central/rev/20e41d4a61a8f5e34c9cf357304b78b3e9bced8a/dom/indexedDB/IDBObjectStore.cpp#1325-1328

Related wpt failure:
- idbobjectstore_createIndex15-autoincrement.htm

Comment 1

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

Attachment: (v1) Patch: Support index key extraction if the |autoIncrement| primary key is part of this index key.

As analyzed in comment 0,
This is to fix a defect in our design:
1. The key extraction and encoding are done in child processes.
2. However, the autoIncrement key can only be generated later in parent.
3. Hence, the value of a compound indexed key with |autoIncrement| primary key included is not possible to be extracted from an input JS object if the primary key is undefined in this object.

The solution in this patch is to 
1. Make a reservation in the encoded buffer for the |autoIncrement| primary key if included in the index key.
2. Fill the real key in parent when |autoIncrementNum| is acquired.

Comment 2

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

Treeherder result looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2f97f70df6f5dbbe0db73ef7abe9eaa00448e8d

Comment 3

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

Comment on attachment 8915537 [details] [diff] [review]
(v1) Patch: Support index key extraction if the |autoIncrement| primary key is part of this index key.

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

Jan's queue is a little bit long.

:baku, would you mind reviewing this change?

Thanks!

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8915537 [details] [diff] [review]
(v1) Patch: Support index key extraction if the |autoIncrement| primary key is part of this index key.

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

It looks good to me, but I prefer janv to take a look.

Comment 5

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

Hi Jan, this is the only test that is failed in all browser which might be a concern for the v2 spec recommendation, although it's not a new feature specified in v2:
https://wpt.fyi/IndexedDB/idbobjectstore_createIndex15-autoincrement.htm

Would you minding reviewing it in higher priority? Thanks!

Comment 6

[Jan Varga [:janv]]

Comment on attachment 8915537 [details] [diff] [review]
(v1) Patch: Support index key extraction if the |autoIncrement| primary key is part of this index key.

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

::: dom/indexedDB/Key.h
@@ +29,5 @@
>    friend struct IPC::ParamTraits<Key>;
>  
>    nsCString mBuffer;
> +  nsTArray<uint32_t> mAutoIncrementKeyOffsets;
> +  bool mHasAutoIncrementKeyReserved;

I'm not very happy that we have to "pollute" the Key class with these additional members just to handle the special case.
Well, I will just say ok if there's no other way to fix the bug.
Let me dig into it a bit more...

Comment 7

[Jan Varga [:janv]]

Comment on attachment 8915537 [details] [diff] [review]
(v1) Patch: Support index key extraction if the |autoIncrement| primary key is part of this index key.

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

::: dom/indexedDB/ActorsParent.cpp
@@ +26350,5 @@
> +      // Update index keys if primary key is preserved in child.
> +      nsTArray<IndexUpdateInfo>& indexUpdateInfos = mParams.indexUpdateInfos();
> +      uint32_t count = indexUpdateInfos.Length();
> +      for (uint32_t i = 0; i < count; i++) {
> +        indexUpdateInfos[i].value().MaybeUpdateAutoIncrementKey(autoIncrementNum);

Here we could do something like this:
Key& key = indexUpdateInfos[i].value();
if (key.HasUnsetAutoIncrementNums()) {
  key.SetAutoIncrementNums(autoIncrementNum);
}

It lines with the way how we do that for primary keys a bit better.
Other methods can also assert that there are no unset keys.

::: dom/indexedDB/Key.cpp
@@ +859,5 @@
> +  mAutoIncrementKeyOffsets.AppendElement(oldLen + 1);
> +
> +  // Fill the type.
> +  buffer += oldLen;
> +  *(buffer++) = (aFirstOfArray ? eMaxType : 0) + eFloat;;

Hm, so for example EncodeNumber() takes aTypeOffset and then sets the type to |eFloat + aTypeOffset|.
aTypeOffset is used to handle/optimize arrays AFAIK. Are you sure that |(aFirstOfArray ? eMaxType : 0) + eFloat;| is ok here ?

::: dom/indexedDB/Key.h
@@ +29,5 @@
>    friend struct IPC::ParamTraits<Key>;
>  
>    nsCString mBuffer;
> +  nsTArray<uint32_t> mAutoIncrementKeyOffsets;
> +  bool mHasAutoIncrementKeyReserved;

We could try to eliminate mHasAutoIncrementKeyReserved at least.

::: dom/indexedDB/KeyPath.cpp
@@ +326,3 @@
>                                              DoNotCreateProperties, nullptr,
>                                              nullptr);
>      if (NS_FAILED(rv)) {

It's a bit weird that we do this when we "fail". Can you try to incorporate this into GetJSValFromKeyPathString() ?

Comment 8

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

(In reply to Jan Varga [:janv] from comment #7)
> Hm, so for example EncodeNumber() takes aTypeOffset and then sets the type
> to |eFloat + aTypeOffset|.
> aTypeOffset is used to handle/optimize arrays AFAIK. Are you sure that
> |(aFirstOfArray ? eMaxType : 0) + eFloat;| is ok here ?
If I understand correctly from the spec:
http://w3c.github.io/IndexedDB/#key-path-construct
It shall be ok to add an offset of eMaxType at most once because:
1. the type of the AutoIncrement Key itself is a 'Number'.
2. The keypath could be a list of identifier, so the max array level of this key in the key path could only be 1.

I'll try to optimize the rest according to your suggestions.
Thanks for the comment! Happy holiday & happy new year!!!

Comment 9

[Jan Varga [:janv]]

(In reply to Bevis Tseng[:bevis][:btseng] from comment #8)
> If I understand correctly from the spec:
> http://w3c.github.io/IndexedDB/#key-path-construct
> It shall be ok to add an offset of eMaxType at most once because:
> 1. the type of the AutoIncrement Key itself is a 'Number'.
> 2. The keypath could be a list of identifier, so the max array level of this
> key in the key path could only be 1.

Maybe, I have to double check, key encoding is highly optimized, thus it's quite complex.
And if we do a mistake then we will have to write a new migration.

> 
> I'll try to optimize the rest according to your suggestions.
> Thanks for the comment! Happy holiday & happy new year!!!

Sure, happy holidays to you too!

Comment 10

[Marion Daly [:mdaly]]

Sorry to keep sending things your way :janv, can you take a look at this?

Comment 11

[Jan Varga [:janv]]

Yeah, I'll take a look at these bugs one by one.

Comment 12

[Marion Daly [:mdaly]]

removing NI adding to backlog

Comment 15

[Thomas Wisniewski [:twisniewski]]

I think this is also causing the failures of the various reading-autoincrement-indexes interop2024 iDB tests.

Comment 16

[Jari Jalkanen]

Attachment: Bug 1404276 - Support IDB key extraction with autoincremented primary key. r=#dom-storage

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f56bb751bd0
Support IDB key extraction with autoincremented primary key. r=dom-storage-reviewers,janv

Comment 18

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/9f56bb751bd0