Bug 196128 - Blob type cannot be stored correctly in IDB when IDBObjectStore has autoIncrement and keyPath options
Summary: Blob type cannot be stored correctly in IDB when IDBObjectStore has autoIncre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on: 196611
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-21 19:38 PDT by Sihui Liu
Modified: 2019-04-18 14:45 PDT (History)
10 users (show)

See Also:


Attachments
Patch (74.48 KB, patch)
2019-03-26 13:23 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.67 MB, application/zip)
2019-03-26 14:49 PDT, Build Bot
no flags Details
Patch (74.03 KB, patch)
2019-03-26 18:36 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (75.44 KB, patch)
2019-04-01 08:17 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (75.20 KB, patch)
2019-04-03 09:41 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (75.48 KB, patch)
2019-04-10 16:33 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.85 MB, application/zip)
2019-04-10 19:11 PDT, Build Bot
no flags Details
Patch (75.51 KB, patch)
2019-04-11 01:23 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2019-03-21 19:38:11 PDT
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.
Comment 1 Sihui Liu 2019-03-26 13:23:36 PDT
Created attachment 365992 [details]
Patch
Comment 2 Build Bot 2019-03-26 13:26:02 PDT
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 3 Build Bot 2019-03-26 14:49:10 PDT
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
Comment 4 Build Bot 2019-03-26 14:49:12 PDT
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 5 youenn fablet 2019-03-26 15:03:14 PDT
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.
Comment 6 Sihui Liu 2019-03-26 18:36:04 PDT
Created attachment 366036 [details]
Patch
Comment 7 Sihui Liu 2019-03-27 08:55:23 PDT
(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 8 Geoffrey Garen 2019-03-28 13:41:11 PDT
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 9 Geoffrey Garen 2019-03-28 13:43:33 PDT
Comment on attachment 366036 [details]
Patch

Can you add tests for File, FileList, ImageData, and RTCCertificate?
Comment 10 Brady Eidson 2019-03-29 09:53:55 PDT
I discussed this with Sihui IRL and I think we came up with a more straightforward idea. She's trying it now.
Comment 11 Sihui Liu 2019-04-01 08:17:03 PDT
Created attachment 366396 [details]
Patch
Comment 12 Geoffrey Garen 2019-04-02 16:09:20 PDT
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().
Comment 13 Sihui Liu 2019-04-03 09:41:08 PDT
Created attachment 366615 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2019-04-03 10:04:05 PDT
Comment on attachment 366615 [details]
Patch for landing

Clearing flags on attachment: 366615

Committed r243807: <https://trac.webkit.org/changeset/243807>
Comment 15 WebKit Commit Bot 2019-04-03 10:04:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-04-03 10:05:29 PDT
<rdar://problem/49562115>
Comment 17 Geoffrey Garen 2019-04-03 10:39:03 PDT
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.
Comment 18 Sihui Liu 2019-04-03 11:22:04 PDT
(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.
Comment 19 Shawn Roberts 2019-04-04 09:59:28 PDT
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
Comment 20 Shawn Roberts 2019-04-04 10:02:22 PDT
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
Comment 21 WebKit Commit Bot 2019-04-04 10:14:29 PDT
Re-opened since this is blocked by bug 196611
Comment 22 Sihui Liu 2019-04-10 16:33:51 PDT
Created attachment 367175 [details]
Patch
Comment 23 Sihui Liu 2019-04-10 16:52:09 PDT
(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 24 Build Bot 2019-04-10 19:11:42 PDT
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
Comment 25 Build Bot 2019-04-10 19:11:54 PDT
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
Comment 26 Ryosuke Niwa 2019-04-11 00:48:32 PDT
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
Comment 27 Sihui Liu 2019-04-11 01:23:09 PDT
Created attachment 367209 [details]
Patch
Comment 28 Geoffrey Garen 2019-04-18 13:06:01 PDT
Comment on attachment 367209 [details]
Patch

r=me
Comment 29 Geoffrey Garen 2019-04-18 13:06:55 PDT
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 30 WebKit Commit Bot 2019-04-18 14:45:46 PDT
Comment on attachment 367209 [details]
Patch

Clearing flags on attachment: 367209

Committed r244436: <https://trac.webkit.org/changeset/244436>
Comment 31 WebKit Commit Bot 2019-04-18 14:45:48 PDT
All reviewed patches have been landed.  Closing bug.