1404274 - Key Evaluation on the cloned JS objects

Status: RESOLVED FIXED

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

Description

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

This is a follow-up of Bug 1178829 comment 6:
In current implementation, keypath evaluation is done on the passed-in JS object instead of the cloned one.
See step 10 for the reason of evaluating on the cloned object in http://w3c.github.io/IndexedDB/#dom-idbobjectstore-put

Related wpt failures:
- clone-before-keypath-eval.html
- keypath-exceptions.htm
- idb-binary-key-detached.htm

Comment 1

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

Hi Jan,

Is there any reason why we don't follow the spec since IDB spec v1 to create a clone before storing operations?
The only reason I can imagine is to have better the performance for storing/retrieving data.
In our implementation, the object is serialized in content when storing and de-serialized when retrieving.

However, "the created clone" is used as the value for the storing operations, since spec v1:
https://www.w3.org/TR/IndexedDB/#widl-IDBObjectStore-add-IDBRequest-any-value-any-key
"
The steps are run with this IDBObjectStore as source and the steps for storing a record into an object store as operation, using this IDBObjectStore as store, the created clone as value, the key parameter as key, and with the no-overwrite flag flag set to true
"

It makes sense to ensure that the data are structured-cloneable before the key evaluation/extraction as explained in "Why create a copy of the value? in "http://w3c.github.io/IndexedDB/#dom-idbobjectstore-put
That's why we got several failures in the wpt listed in comment 0.

There are 2 options so far to fix these problems:
1. Use/Define more JS APIs for IDB implementation to filter properties unacceptable by structured-clone algorithm.
   Concern: We'll have to update this implementation continuously if structured-clone algorithm is changed.
2. Just do stuctured-clone at the beginning of IDBObjectStore::AddOrPut() to follow the spec.
   Concern: Need to evaluate how bad the performance is dropped.

Do you have more suggestion on fixing this problem?
What option do you prefer?
(IMO, option#1 is preferred if it's doable with the assumption that there will be not much change in the structured-clone alogrithm in the future.)

Comment 2

[Jan Varga [:janv]]

I didn't investigate this very deeply, but is the problem that for example clone-before-keypath-eval.html counts access to object properties and expects that add/put touches them only once ?
But our implementation can end up touching them multiple times () ?

Comment 3

[Jan Varga [:janv]]

(In reply to Bevis Tseng[:bevis][:btseng] from comment #1)
> 2. Just do stuctured-clone at the beginning of IDBObjectStore::AddOrPut() to
> follow the spec.
>    Concern: Need to evaluate how bad the performance is dropped.

You mean to serialize the object first and then deserialize it into a cloned object which is then used for key extraction ?
I think it's fine if we do the extra deserialization, because it would be done only if there's a keypath and auto-increment is false.

Comment 4

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

(In reply to Jan Varga [:janv] from comment #2)
> I didn't investigate this very deeply, but is the problem that for example
> clone-before-keypath-eval.html counts access to object properties and
> expects that add/put touches them only once ?
> But our implementation can end up touching them multiple times () ?
Yes but more exceptions:
1. in keypath-exceptions.htm: no-enumerable getter shall be ignored.
2. in idb-binary-key-detached.htm, a detached ArrayBuffer shall not be structured-clonable (This is not related to key extraction but is related to make a clone before the add/put operations. There might be another bug in our structured clone algorithm to identify this detached ArrayBuffer as invalid data.)

> (In reply to Bevis Tseng[:bevis][:btseng] from comment #1)
> > 2. Just do stuctured-clone at the beginning of IDBObjectStore::AddOrPut() to
> > follow the spec.
> >    Concern: Need to evaluate how bad the performance is dropped.
> 
> You mean to serialize the object first and then deserialize it into a cloned
> object which is then used for key extraction ?
Yes.
> I think it's fine if we do the extra deserialization, because it would be
> done only if there's a keypath and auto-increment is false.
We need this clone object as well if there are index defined in this object store because the key extraction is done in the content process.

I could try option#2 first and do more comparison to the build without option#2 solution and to Chrome to see if there is any performance issue.

Comment 5

[Jan Varga [:janv]]

(In reply to Bevis Tseng[:bevis][:btseng] from comment #4)
> I could try option#2 first and do more comparison to the build without
> option#2 solution and to Chrome to see if there is any performance issue.

Ok.

Comment 6

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

Update the status so far:
Some wpt are fixed but more failures/assertions are discovered after introducing StructuredCloneHolder before key extraction:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1981f674da45df3757ed3f6f96ea80180c87d23

Comment 7

[Marion Daly [:mdaly]]

Janv, assigning to you as you have the most history on this. If you don't have time for it, see if catalin or baku want to take it up instead.

Comment 8

[Jan Varga [:janv]]

Attachment: patch

A test in key_invalid.htm now doesn't pass due to cloning.
The problem is that [1,2,3,,] is converted to [1,2,3] during cloning since array length is not enumerable property, so it is not cloned.
See https://dxr.mozilla.org/mozilla-central/source/js/src/tests/non262/extensions/clone-object.js#57

I don't know why key_invalid.htm insists on getting exception for [1,2,3,,].
IDB suggets to clone before extracting keys and I guess we follow the spec for structured cloning by not enumerating array length property.

Here's a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4b92ca9720f5d610c7d8d482a81afe92f3ab7f3

Comment 9

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

It looks like we're behind the times on implementing the structured clone spec.  I filed bug 1476955 on your discovery, but I believe we can go ahead with landing this without waiting for that to land, so I marked it as see also rather than a dependency.

Comment 10

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

Comment on attachment 8993256 [details] [diff] [review]
patch

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

The IDB logic is good, but I think the structured cloning has some refcounting hazards, which I discuss below.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +710,5 @@
> +      intptr_t ptr;
> +
> +      if (blob->IsFile()) {
> +        RefPtr<File> file = blob->ToFile();
> +        ptr = reinterpret_cast<intptr_t>(file.get());

From a safety/sanity perspective, I don't think it's safe to be writing out a bare pointer to a refcounted object without some guarantee that a refcount will be maintained.  The structured clone process potentially triggers user code via getters which could cause some refcounts to go to zero.  (Although the involvement of JS GC does make it a little bit more complex to accomplish this.)

Unfortunately, just changing the cast to a forget() with a matching already_AddRefed on the read side isn't sufficient because we need to be able to handle cleanup in the event the structured clone aborts partway through.  If we did forget(), we'd potentially leak in those cases.  I believe this is why StructuredCloneHolder maintains multiple parallel nsTArray<RefPtr<foo>> arrays.

On IRC, I pointed :baku at this patch for a skim and he proposed that instead of using the callbacks like you've done here, you instead subclass StructuredCloneHolderBase and implement yours own CustomWriteHandler and CustomReadHandler in order to creating more instances of JSStructuredCloneCallbacks...

...I wonder if whether it makes sense to go a step further and subclass StructuredCloneHolder.  The main problem is that StructuredCloneHolder::CustomWriteHandler supports more than we currently want.  But perhaps :baku would be okay with us modifying StructuredCloneHolder to check for a StructuredCloneScope of  DifferentProcessForIndexedDB so that we can get the behavior IDB wants there without copying-and-pasting-and-modifying the code.  Arguably it's not too bad for complexity because they are orthogonal "is this type okay?" decisions, and we already have a StructuredCloneScope because we can't pretend like the complexity doesn't exist.  Also, this might help us further clean up IDB's structured clone logic.

@@ +1210,5 @@
> +JSObject*
> +CopyingStructuredCloneReadCallback(JSContext* aCx,
> +                                   JSStructuredCloneReader* aReader,
> +                                   uint32_t aTag,
> +                                   uint32_t aData,

If you're going to write the sizeof(ptr), it probably makes sense to at least MOZ_ASSERT on aData matching.

Comment 11

[Jan Varga [:janv]]

Oh, right, I overlooked this line:
https://dxr.mozilla.org/mozilla-central/source/js/src/vm/StructuredClone.cpp#1311
and I somehow assumed that if I hold the original value in a JS::Rooted, the object and its properties can't be released.

Ok, I'll fix it.

Btw, is it necessary to subclass StructuredCloneHolderBase or StructuredCloneHolder ?
What about just passing a closure to JS_StructuredClone() which would contain nsTArray<RefPtr<Blob>> etc ?

We have these special callbacks in IDBObjectStore.cpp already, and the new ones look very similar them, so it's quite easy to figure out what they do.
The new ones are just optimized for cloning a value on the same process and the same thread.

Comment 12

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

(In reply to Jan Varga [:janv] from comment #11)
> Btw, is it necessary to subclass StructuredCloneHolderBase or
> StructuredCloneHolder ?
> What about just passing a closure to JS_StructuredClone() which would
> contain nsTArray<RefPtr<Blob>> etc ?

A closure works.  I just wanted to provide :baku's feedback in there and provide an option for consideration.  There's no clear best option here from my perspective, it's basically a choice between 1 centralized more complex implementation and several simpler implementations.  I agree that the current patch and a closure is definitely more straightforward and if that seems right to you, I'm on board.

Comment 13

[Jan Varga [:janv]]

Attachment: patch

Comment 14

[Jan Varga [:janv]]

Attachment: interdiff

Comment 15

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

Comment on attachment 8994476 [details] [diff] [review]
patch

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

Thanks for the interdiff!

Restating the changes: StructuredCloneInfo is created to hold the strong references.  The serialization now uses indices like how StructuredCloneHolder works.  StructuredCloneInfo is instantiated on the stack, guaranteeing cleanup.  We don't do anything to try and avoid extra refcount pairs like using forget(), but we don't really need to.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +101,5 @@
>      MOZ_COUNT_DTOR(StructuredCloneWriteInfo);
>    }
>  };
>  
> +struct IDBObjectStore::StructuredCloneInfo

Please add a comment here like:
Used by ValueWrapper::Clone to hold strong references to any blob-like objects through the clone process.  This is necessary because:
- The structured clone process may trigger content code via getters/other which can potentially cause existing strong references to be dropped, necessitating the clone to hold its own strong references.
- The structured clone can abort partway through, so it's necessary to track what strong references have been acquired so that they can be freed even if a de-serialization does not occur.

Comment 16

[Jan Varga [:janv]]

I'll add the comment, thanks!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f85380831fe4
Key Evaluation on the cloned JS objects; r=asuth

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 19

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

Attachment: Key evaluation fix for uplift. r=asuth

Note that I am providing an updated patch that folds in a minor correctness fix for uplift.

Approval Request Comment
[Feature/Bug causing the regression]:
Longstanding IndexedDB correctness issue.

[User impact if declined]:
Web developers may be more inclined to fall back to LocalStorage which is all around a worse API for developers and users, or give Firefox a degraded experience.

[Is this code covered by automated tests?]:
Yes, existing web-platform-tests.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
I've folded in the correctness fix for simplicity already.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
We have thorough automated tests.

[String changes made/needed]:
No string changes.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8997127 [details] [diff] [review]
Key evaluation fix for uplift. r=asuth

Fix for IndexedDB, adds test coverage, seems ok on nightly, let's take it for beta 15.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e21cf620b886

Comment 22

[Jan Varga [:janv]]

Comment on attachment 8997127 [details] [diff] [review]
Key evaluation fix for uplift. r=asuth

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Longstanding IndexedDB correctness issue.

User impact if declined: Web developers may be more inclined to fall back to LocalStorage which is all around a worse API for developers and users, or give Firefox a degraded experience.

Fix Landed on Version: 63 and 62

Risk to taking this patch (and alternatives if risky): Low risk, we have thorough automated tests and it's been already verified on nightly and beta.

String or UUID changes made by this patch: None.

Comment 23

[Pascal Chevrel:pascalc (PTO until April 26)]

Ryan, would it make sense to have an uplift for 61 as a ride along for a dot release?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

I don't think this warrants dot release inclusion, but we should take it for ESR 60.2.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/63f65151b538

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out from ESR60 for bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=192644269&repo=mozilla-esr60

https://hg.mozilla.org/releases/mozilla-esr60/rev/4724f75e4822ad568357303ecc787b1c69e9710a

Comment 27

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

It looks like bug 1441141 landed in 61 which added the canTransfer op which is our one too many giving us the error line:
22:04:04 INFO - /builds/worker/workspace/build/src/dom/indexedDB/IDBObjectStore.cpp:3106:3: error: too many initializers for 'const JSStructuredCloneCallbacks' 

https://searchfox.org/mozilla-central/diff/cb4b451b23c3a0f6b73e2245ab1b52d39f5df27d/js/public/StructuredClone.h#313

I'll fix locally and see.

Comment 28

[Jan Varga [:janv]]

Attachment: patch for ESR

This one builds and mochitest/xpshell-test for IndexedDB pass.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/32c012e50283

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Backed out for rooting analysis failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=192793764&repo=mozilla-esr60

https://hg.mozilla.org/releases/mozilla-esr60/rev/e31b504b403eeb3941818c8b6b94134c20714bf6

Comment 31

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

:sfink, can you help us understand this rooting hazard log below and why it's happening on the ESR uplift and not on trunk?  :RyanVM suggested you were the relevant expert / person touching things related to this last ;)

A brief overview of the patch:
- The IDB API lets callers put() an object.  Because IDB allows indexes that involve evaluating "key paths" on the object and the evaluation can involve getters which return different values each time, we're moving to clone the value once and evaluate the paths on the clone IFF we have indices which will necessitate doing more than the single value storage clone.
- Jan implemented a clone-in-place mechanism with its own structured clone callbacks that holds strong references in the closure to the blobs/files/wasm modules during the duration of the clone.  That is, the write pushes the references, the read uses those references, but does not clear them.  The closure is held alive on the stack throughout the entire duration of the call to JS_StructuredClone.
- There is a WasmCompiledModuleStream thing that we use (that's being removed as WASM/IDB support is removed) to know when a WASM module has finished doing its fancy optimized compilation so it can be put in the database.  It's also technically true that the state machine of this thing will drop the WASM module if/when it's closed.  Dynamically speaking, I think what the analysis is concerned about can't happen, but maybe there's something wrong with how the structured clone callbacks are implemented otherwise?

Here's the hazards.txt, there's also some heap write hazards too:
"""
Time: Wed Aug 08 2018 15:07:09 GMT+0000 (UTC)

Function '_ZN7mozilla3dom12_GLOBAL__N_134CopyingStructuredCloneReadCallbackEP9JSContextP23JSStructuredCloneReaderjjPv$IDBObjectStore.cpp:JSObject* mozilla::dom::{anonymous}::CopyingStructuredCloneReadCallback(JSContext*, JSStructuredCloneReader*, uint32, uint32, void*)' has unrooted '<returnvalue>' of type 'JSObject*' live across GC call '_ZN6RefPtrIN2JS10WasmModuleEED1Ev$RefPtr<T>::~RefPtr() [with T = JS::WasmModule]' at dom/indexedDB/IDBObjectStore.cpp:1326
    IDBObjectStore.cpp:1326: Assign(146,147, return := __temp_46**)
    IDBObjectStore.cpp:1326: Call(147,148, wrappedModule.~Rooted())
    IDBObjectStore.cpp:1326: Call(148,149, module.~WasmModule>()) [[GC call]]
    IDBObjectStore.cpp:1326: Call(149,151, result.~Rooted())
    IDBObjectStore.cpp:1331:  [[end of function]]
GC Function: _ZN6RefPtrIN2JS10WasmModuleEED1Ev$RefPtr<T>::~RefPtr() [with T = JS::WasmModule]
    RefPtr<T>::~RefPtr() [with T = JS::WasmModule]
    static void RefPtr<T>::ConstRemovingRefPtrTraits<U>::Release(U*) [with U = JS::WasmModule; T = JS::WasmModule]
    static void mozilla::RefPtrTraits<U>::Release(U*) [with U = JS::WasmModule]
    void js::AtomicRefCounted<T>::Release() const [with T = JS::WasmModule]
    Utility.h:void js_delete(JS::WasmModule*) [with T = JS::WasmModule]
    void js::wasm::Module::~Module(int32)
    void js::wasm::Module::~Module(int32)
    js::ExclusiveWaitableData<js::wasm::Tiering>::~ExclusiveWaitableData()
    js::ExclusiveWaitableData<js::wasm::Tiering>::~ExclusiveWaitableData()
    js::ExclusiveData<T>::~ExclusiveData() [with T = js::wasm::Tiering]
    js::ExclusiveData<T>::~ExclusiveData() [with T = js::wasm::Tiering]
    void js::wasm::Tiering::~Tiering(int32)
    void js::wasm::Tiering::~Tiering(int32)
    mozilla::Vector<T, N, AllocPolicy>::~Vector() [with T = RefPtr<JS::WasmModuleListener>; long unsigned int MinInlineCapacity = 0ul; AllocPolicy = js::SystemAllocPolicy]
    mozilla::Vector<T, N, AllocPolicy>::~Vector() [with T = RefPtr<JS::WasmModuleListener>; long unsigned int MinInlineCapacity = 0ul; AllocPolicy = js::SystemAllocPolicy]
    static void mozilla::detail::VectorImpl<T, N, AP, IsPod>::destroy(T*, T*) [with T = RefPtr<JS::WasmModuleListener>; long unsigned int N = 0ul; AP = js::SystemAllocPolicy; bool IsPod = false]
    RefPtr<T>::~RefPtr() [with T = JS::WasmModuleListener]
    RefPtr<T>::~RefPtr() [with T = JS::WasmModuleListener]
    static void RefPtr<T>::ConstRemovingRefPtrTraits<U>::Release(U*) [with U = JS::WasmModuleListener; T = JS::WasmModuleListener]
    static void mozilla::RefPtrTraits<U>::Release(U*) [with U = JS::WasmModuleListener]
    IDBObjectStore.cpp:uint32 mozilla::dom::{anonymous}::WasmCompiledModuleStream::Release()
    IDBObjectStore.cpp:void mozilla::dom::{anonymous}::WasmCompiledModuleStream::~WasmCompiledModuleStream()
    IDBObjectStore.cpp:void mozilla::dom::{anonymous}::WasmCompiledModuleStream::~WasmCompiledModuleStream(int32)
    IDBObjectStore.cpp:void mozilla::dom::{anonymous}::WasmCompiledModuleStream::~WasmCompiledModuleStream(int32)
    IDBObjectStore.cpp:uint32 mozilla::dom::{anonymous}::WasmCompiledModuleStream::Close()
    IDBObjectStore.cpp:uint32 mozilla::dom::{anonymous}::WasmCompiledModuleStream::CloseWithStatus(uint32)
    FieldCall: nsIInputStream.Close
"""

Comment 32

[Steve Fink [:sfink] [:s:]]

Oops, just saw this needinfo. I will look a bit later, but it appears to be a real hazard that I uncovered recently while fixing bugs in the analysis, and fixed but have not yet landed over in bug 1480640. So most likely, some code changed that allowed to see through the part where the analysis was getting confused, and the same fix will be needed.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Try confirms that this is green on ESR60 with bug 1480640 grafted on top. Steve, can you proceed with an uplift request please?

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/8578421ae142