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.
Created attachment 365992 [details] Patch
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.
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
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
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.
Created attachment 366036 [details] Patch
(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?
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.
Comment on attachment 366036 [details] Patch Can you add tests for File, FileList, ImageData, and RTCCertificate?
I discussed this with Sihui IRL and I think we came up with a more straightforward idea. She's trying it now.
Created attachment 366396 [details] Patch
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().
Created attachment 366615 [details] Patch for landing
Comment on attachment 366615 [details] Patch for landing Clearing flags on attachment: 366615 Committed r243807: <https://trac.webkit.org/changeset/243807>
All reviewed patches have been landed. Closing bug.
<rdar://problem/49562115>
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 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.
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
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
Re-opened since this is blocked by bug 196611
Created attachment 367175 [details] Patch
(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
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
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
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
Created attachment 367209 [details] Patch
Comment on attachment 367209 [details] Patch r=me
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.)
Comment on attachment 367209 [details] Patch Clearing flags on attachment: 367209 Committed r244436: <https://trac.webkit.org/changeset/244436>