Bug 196128 – Blob type cannot be stored correctly in IDB when IDBObjectStore has autoIncrement and keyPath options

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

In network process, we deserialize value from IDBValue to JSValue, insert key into the value, and then serialize it again and put it in database.
Because network process does not have JSDOMGlobal object, deserialization would fail for types including File, Blob, etc.

[Sihui Liu]

Created attachment 365992 [details]
Patch

[EWS Watchlist]

Attachment 365992 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1853:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 365992 [details]
Patch

Attachment 365992 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11675656

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/nested-cloning-large-multiple.html

[EWS Watchlist]

Created attachment 366003 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[youenn fablet]

Comment on attachment 365992 [details]
Patch

Looks to go in right direction but I am not familiar enough in IDB for doing proper review.
Some suggestions below.

View in context: https://bugs.webkit.org/attachment.cgi?id=365992&action=review

> Source/WebCore/ChangeLog:106
> +        store the IDBValue into IDBRequest result after injectKeyIntoResult, and then create JSValue at the time 

Could it be a preliminary patch? Or could we find another way to not have all this Blob specific handling?

> Source/WebCore/Modules/indexeddb/IDBGetAllResult.cpp:36
> +template<typename T> Vector<T> isolatedCopyOfVector(const Vector<T> original)

Probably no longer needed, CrossThreadCopier has it.

> Source/WebCore/Modules/indexeddb/IDBGetAllResult.cpp:55
>  void IDBGetAllResult::isolatedCopy(const IDBGetAllResult& source, IDBGetAllResult& destination)

Can we return an IDBGetAllResult and make it a function member?

> Source/WebCore/Modules/indexeddb/IDBGetAllResult.cpp:60
> +    destination.m_keyPath = WebCore::isolatedCopy(source.m_keyPath);

Maybe use a constructor?

> Source/WebCore/Modules/indexeddb/IDBGetResult.h:75
> +        , m_keyPath(keyPath)

These 3 constructors could use r-values.
Not to say to do this in this patch though.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:886
> +            if (injectKeyIntoResult(result.primaryKeyData(), result.value(), result.keyPath().value(), injectedValue, addedBlobURLPaths)) {

Can injectKeyIntoResult return a pair of ThreadSafeDataBuffer + Vector<std::pair<String, String>> or an Optional pair?
I find it easier to read to have Vector<Struct> with Struct having two named members.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:889
> +                for (auto urlPath : addedBlobURLPaths) {

auto&

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:891
> +                    paths.append(urlPath.second);

Could WTFMove these.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:893
> +                finalResultData.getResultPtr()->setValue({ injectedValue, urls, result.value().sessionID(), paths });

We could probably move urls and paths here.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:947
> +    }

This seems code that is very similar to above. Use a helper routine?

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:367
> +        IDBKeyData key = objectStore->lowestKeyWithRecordInRange(range);

auto

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:369
> +            outValue = {key, ThreadSafeDataBuffer(), objectStore->info().keyPath()};

space {}

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:371
> +            outValue = {key, objectStore->valueForKey(key), objectStore->info().keyPath()};

Can we do outValue = { key, key.isNull() ? : , ... };
Or move key?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1750
> +    auto* objectStoreInfo = infoForObjectStore(info.objectStoreIdentifier());

Why are we sure to have an objectStoreInfo? applies to here and below.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1752
> +    generateIndexKeyForValue(*m_globalObject->globalExec(), info, jsValue, indexKey, objectStoreInfo->keyPath(), key);

Maybe it is not possible in this patch, but can we remove the need for generateIndex... to take an ExecState?

> Source/WebCore/Modules/indexeddb/shared/IDBResultData.cpp:238
> +    RELEASE_ASSERT(m_getResult);

Why release_assert here.
Should probably return a IDBGetResult&

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3070
> +    Vector<std::pair<String, String>>* m_addedBlobURLPaths;

Why a pointer and not a reference?
Also, this makes it difficult to see how m_addedBlobURLPaths is used.
For instance, if m_addedBlobURLPaths is empty before the deserialization, maybe it would be better replaced by a getter to the vector used after the deserialization.

[Sihui Liu]

Created attachment 366036 [details]
Patch

[Sihui Liu]

(In reply to youenn fablet from comment #5)
> Comment on attachment 365992 [details]
> Patch
> 
> Looks to go in right direction but I am not familiar enough in IDB for doing
> proper review.
> Some suggestions below.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365992&action=review
> 
> > Source/WebCore/ChangeLog:106
> > +        store the IDBValue into IDBRequest result after injectKeyIntoResult, and then create JSValue at the time 
> 
> Could it be a preliminary patch? Or could we find another way to not have
> all this Blob specific handling?
> 

The tricky part is the Blob object goes away but its url is recorded in SerializedScriptValue / IDBValue, so network process does not keep information about that Blob.

I haven't figured out a better way to do this. Another option is to keep JSValue instead of SerializedScriptValue as request result, but that would require to use Strong<>, which sounds like a reference cycle.

> > Source/WebCore/Modules/indexeddb/IDBGetAllResult.cpp:36
> > +template<typename T> Vector<T> isolatedCopyOfVector(const Vector<T> original)
> 
> Probably no longer needed, CrossThreadCopier has it.
> 

Didn't know that. Removed.

> > Source/WebCore/Modules/indexeddb/IDBGetAllResult.cpp:55
> >  void IDBGetAllResult::isolatedCopy(const IDBGetAllResult& source, IDBGetAllResult& destination)
> 
> Can we return an IDBGetAllResult and make it a function member?
> 

You mean like IDBGetAllResult IDBGetAllResult::isolatedCopy() const above?

> > Source/WebCore/Modules/indexeddb/IDBGetAllResult.cpp:60
> > +    destination.m_keyPath = WebCore::isolatedCopy(source.m_keyPath);
> 
> Maybe use a constructor?
> 

Constructor for IDBKeyPath?

> > Source/WebCore/Modules/indexeddb/IDBGetResult.h:75
> > +        , m_keyPath(keyPath)
> 
> These 3 constructors could use r-values.
> Not to say to do this in this patch though.
> 

Okay, will change it, likely not in this patch.

> > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:886
> > +            if (injectKeyIntoResult(result.primaryKeyData(), result.value(), result.keyPath().value(), injectedValue, addedBlobURLPaths)) {
> 
> Can injectKeyIntoResult return a pair of ThreadSafeDataBuffer +
> Vector<std::pair<String, String>> or an Optional pair?
> I find it easier to read to have Vector<Struct> with Struct having two named
> members.
> 

Sure. Changed to return Optional<InjectionResult>.

> > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:889
> > +                for (auto urlPath : addedBlobURLPaths) {
> 
> auto&
> 

Okay.

> > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:891
> > +                    paths.append(urlPath.second);
> 
> Could WTFMove these.
> 

Okay.

> > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:893
> > +                finalResultData.getResultPtr()->setValue({ injectedValue, urls, result.value().sessionID(), paths });
> 
> We could probably move urls and paths here.
> 

Okay.

> > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:947
> > +    }
> 
> This seems code that is very similar to above. Use a helper routine?
> 
> > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:367
> > +        IDBKeyData key = objectStore->lowestKeyWithRecordInRange(range);
> 
> auto
> 

Okay.

> > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:369
> > +            outValue = {key, ThreadSafeDataBuffer(), objectStore->info().keyPath()};
> 
> space {}
> 

Added.

> > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:371
> > +            outValue = {key, objectStore->valueForKey(key), objectStore->info().keyPath()};
> 
> Can we do outValue = { key, key.isNull() ? : , ... };
> Or move key?
> 

Yes, we can

> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1750
> > +    auto* objectStoreInfo = infoForObjectStore(info.objectStoreIdentifier());
> 
> Why are we sure to have an objectStoreInfo? applies to here and below.
> 

IDBIndex is created on IDBObjectStore. If we have an existing index, we should have corresponding existing objectStore. 
We extract or create databaseInfo before using the database, and databaseInfo should contain info of existing objectStores.

> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1752
> > +    generateIndexKeyForValue(*m_globalObject->globalExec(), info, jsValue, indexKey, objectStoreInfo->keyPath(), key);
> 
> Maybe it is not possible in this patch, but can we remove the need for
> generateIndex... to take an ExecState?
> 

I thought about this and it is not easy. We need to extract index value from value of record, which means we still deserialize the value into JSValue and then look up the JSValue properties to find index. And unlike this patch, this has to be done in the network process.

> > Source/WebCore/Modules/indexeddb/shared/IDBResultData.cpp:238
> > +    RELEASE_ASSERT(m_getResult);
> 
> Why release_assert here.
> Should probably return a IDBGetResult&
> 

I just followed the above const IDBGetResult& IDBResultData::getResult() const.
I guess it wants to make sure we always access the right type of result? 
I change this to return a IDBGetResult&.

> > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3070
> > +    Vector<std::pair<String, String>>* m_addedBlobURLPaths;
> 
> Why a pointer and not a reference?
> Also, this makes it difficult to see how m_addedBlobURLPaths is used.
> For instance, if m_addedBlobURLPaths is empty before the deserialization,
> maybe it would be better replaced by a getter to the vector used after the
> deserialization.

Because I only added addedBlobURLPaths to only one constructor, so this could be nullptr if not setting. 

Because the CloneDeserializer::deserialize is static and we still need to use reference parameter for it, I didn't add a getter in CloneDeserializer. 

Is it better to use an add function here?

[Geoffrey Garen]

Comment on attachment 366036 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366036&action=review

r=me, looks like you've addressed Youenn's comments

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:877
> +            auto urls = result.value().blobURLs();
> +            auto paths = result.value().blobFilePaths();
> +            for (auto& urlPath : injected.value().blobURLPathPairs) {
> +                urls.append(WTFMove(urlPath.first));
> +                paths.append(WTFMove(urlPath.second));
> +            }

I'd like to think through a way to avoid duplicating this pattern. Maybe there's a way to put it into an InjectionResult constructor or helper function. Or maybe there's a way to represent it directly in our serialized/deserialized form of an IDB result. But I don't have an immediate suggestion.

[Geoffrey Garen]

Comment on attachment 366036 [details]
Patch

Can you add tests for File, FileList, ImageData, and RTCCertificate?

[Brady Eidson]

I discussed this with Sihui IRL and I think we came up with a more straightforward idea. She's trying it now.

[Sihui Liu]

Created attachment 366396 [details]
Patch

[Geoffrey Garen]

Comment on attachment 366396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366396&action=review

I love this approach!

r=me, with one suggestion

> Source/WebCore/bindings/js/JSIDBRequestCustom.cpp:83
> +            auto& keys = getAllResult.keys();
> +            auto& values = getAllResult.values();
> +            auto& keyPath = getAllResult.keyPath();
> +            Vector<JSC::JSValue> results;
> +            for (unsigned i = 0; i < values.size(); i ++) {
> +                auto result = deserializeIDBValueWithKeyInjection(state, values[i], keys[i], keyPath);
> +                if (!result)
> +                    return jsNull();
> +                results.append(result.value());
> +            }
> +            
> +            auto scope = DECLARE_THROW_SCOPE(state.vm());
> +            JSC::MarkedArgumentBuffer list;
> +            for (auto& result : results)
> +                list.append(result);
> +            if (UNLIKELY(list.hasOverflowed())) {
> +                propagateException(state, scope, Exception(UnknownError));
> +                return jsNull();
> +            }
> +            return JSValue(JSC::constructArray(&state, nullptr, state.lexicalGlobalObject(), list));

To avoid copying into a MarkedArgumentBuffer, you can use this helper function instead:

    inline JSArray* constructArray(ExecState* exec, ArrayAllocationProfile* profile, JSGlobalObject* globalObject, const JSValue* values, unsigned length, JSValue newTarget = JSValue())

values is results.data() and length is results.size().

[Sihui Liu]

Created attachment 366615 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 366615 [details]
Patch for landing

Clearing flags on attachment: 366615

Committed r243807: <https://trac.webkit.org/changeset/243807>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49562115>

[Geoffrey Garen]

Comment on attachment 366615 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=366615&action=review

> Source/WebCore/bindings/js/JSIDBRequestCustom.cpp:74
> +            Vector<JSC::JSValue> results;
> +            for (unsigned i = 0; i < values.size(); i ++) {
> +                auto result = deserializeIDBValueWithKeyInjection(state, values[i], keys[i], keyPath);
> +                if (!result)
> +                    return jsNull();
> +                results.append(result.value());
> +            }
> +            return JSValue(JSC::constructArray(&state, nullptr, state.lexicalGlobalObject(), results.data(), results.size()));

Sorry, I noticed a bug after this landed:

It's a GC bug to keep JSValue in a Vector. The GC isn't aware of Vector memory on the C++ heap. So, these JSValues and everything they point to are at risk of being garbage collected, triggering use after free.

There are lots of options for fixing this. I think the simplest is to use MarkedArgumentBuffer instead of Vector, and to switch back to using the constructArray that accepts a MarkedArgumentBuffer.

[Sihui Liu]

(In reply to Geoffrey Garen from comment #17)
> Comment on attachment 366615 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366615&action=review
> 
> > Source/WebCore/bindings/js/JSIDBRequestCustom.cpp:74
> > +            Vector<JSC::JSValue> results;
> > +            for (unsigned i = 0; i < values.size(); i ++) {
> > +                auto result = deserializeIDBValueWithKeyInjection(state, values[i], keys[i], keyPath);
> > +                if (!result)
> > +                    return jsNull();
> > +                results.append(result.value());
> > +            }
> > +            return JSValue(JSC::constructArray(&state, nullptr, state.lexicalGlobalObject(), results.data(), results.size()));
> 
> Sorry, I noticed a bug after this landed:
> 
> It's a GC bug to keep JSValue in a Vector. The GC isn't aware of Vector
> memory on the C++ heap. So, these JSValues and everything they point to are
> at risk of being garbage collected, triggering use after free.
> 
> There are lots of options for fixing this. I think the simplest is to use
> MarkedArgumentBuffer instead of Vector, and to switch back to using the
> constructArray that accepts a MarkedArgumentBuffer.

In https://bugs.webkit.org/show_bug.cgi?id=196547.

[Shawn Roberts]

New test storage/indexeddb/modern/objectstore-autoincrement-types.html 

added in https://trac.webkit.org/changeset/243807/webkit is a flaky timeout on iOS Simulator Debug

Reproduced with:

run-webkit-tests storage/indexeddb/modern/objectstore-autoincrement-types.html --ios-simulator --iterations 500 -f --debug

[Shawn Roberts]

The 3 tests unskipped in https://trac.webkit.org/changeset/243807/webkit are now a flaky failure on Mojave Release WK2 and Mojave Release WK1. Dashboard only shows 2 of the tests failing but I can reproduce the flaky failures locally for all 3 tests.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2FIndexedDB%2Fnested-cloning%20storage%2Findexeddb%2Fmodern%2Fobjectstore-autoincrement-types.html%20imported%2Fw3c%2Fweb-platform-tests%2FIndexedDB%2Fnested-cloning-large-multiple.html

reproduced with:

run-webkit-tests imported/w3c/web-platform-tests/IndexedDB/nested* --iterations 500 -f

Diff:

--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/IndexedDB/nested-cloning-large-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/IndexedDB/nested-cloning-large-actual.txt
@@ -1,3 +1,5 @@
+
+Harness Error (TIMEOUT), message = null
 
 PASS large typed array 
 PASS blob with large typed array 
@@ -5,5 +7,5 @@
 PASS array of blobs and large typed arrays 
 PASS array of blobs and large typed arrays with key generator 
 PASS object with blobs and large typed arrays 
-PASS object with blobs and large typed arrays with key generator 
+TIMEOUT object with blobs and large typed arrays with key generator Test timed out

[WebKit Commit Bot]

Re-opened since this is blocked by bug 196611

[Sihui Liu]

Created attachment 367175 [details]
Patch

[Sihui Liu]

(In reply to Sihui Liu from comment #22)
> Created attachment 367175 [details]
> Patch

Re-upload the patch since Shawn and I cannot reproduce the timeout locally.
 
There are a few changes on this patch compared to the previous one:
1. this patch only updates test expectations of 3 wpt tests and doesn't enable them on EWS. I test them locally: they can pass with the patch but they are very slow. These tests could time out on bots and it's hard to tell if there is some bug or if the performance is bad. So I prefer enabling them after we verify this patch is correct.

2. this patch has its own layout testobjectstore-autoincrement-types.html and it also timed out a few times on iOS-simulator bot, which should not happen because it's a small test. I added error handling and logging to the test so we know what's wrong if the timeout happens again

[EWS Watchlist]

Comment on attachment 367175 [details]
Patch

Attachment 367175 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11836073

New failing tests:
storage/indexeddb/modern/objectstore-autoincrement-types.html

[EWS Watchlist]

Created attachment 367188 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ryosuke Niwa]

I guess we don't support file.webkitRelativePath on Windows:

--- /cygdrive/C/cygwin/home/buildbot/WebKit/WebKitBuild/Release/bin32/layout-test-results/storage/indexeddb/modern/objectstore-autoincrement-types-expected.txt
+++ /cygdrive/C/cygwin/home/buildbot/WebKit/WebKitBuild/Release/bin32/layout-test-results/storage/indexeddb/modern/objectstore-autoincrement-types-actual.txt
@@ -20,7 +20,6 @@
 PASS request.result.primaryKey is file.primaryKey
 PASS request.result.name is file.name
 PASS request.result.lastModified is file.lastModified
-PASS request.result.webkitRelativePath is file.webkitRelativePath
 PASS request.result.size is file.size
 PASS request.result.type is file.type
 PASS key is 3
@@ -36,7 +35,6 @@
 PASS request.result[1].primaryKey is file.primaryKey
 PASS request.result[1].name is file.name
 PASS request.result[1].lastModified is file.lastModified
-PASS request.result[1].webkitRelativePath is file.webkitRelativePath
 PASS request.result[1].size is file.size
 PASS request.result[1].type is file.type
 PASS request.result[2].primaryKey is imageData.primaryKey

[Sihui Liu]

Created attachment 367209 [details]
Patch

[Geoffrey Garen]

Comment on attachment 367209 [details]
Patch

r=me

[Geoffrey Garen]

If we have future problems with these new tests, or enabling the WPT tests, because of test-specific issues, let's resolve those by disabling the tests, rather than rolling out the bug fix. (Of course, if the bug fix regresses other tests, we may need to roll it out.)

[WebKit Commit Bot]

Comment on attachment 367209 [details]
Patch

Clearing flags on attachment: 367209

Committed r244436: <https://trac.webkit.org/changeset/244436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.