Bug 202500 - Only wrapping CryptoKeys for IDB during serialization
Summary: Only wrapping CryptoKeys for IDB during serialization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-02 16:03 PDT by Jiewen Tan
Modified: 2019-10-09 14:07 PDT (History)
14 users (show)

See Also:


Attachments
Patch (63.82 KB, patch)
2019-10-02 22:49 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (62.58 KB, patch)
2019-10-03 02:01 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (62.86 KB, patch)
2019-10-06 18:13 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (62.82 KB, patch)
2019-10-07 12:54 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (63.21 KB, patch)
2019-10-07 14:09 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2019-10-02 16:03:37 PDT
Only wrapping CryptoKeys for IDB during serialization.
Comment 1 Jiewen Tan 2019-10-02 16:03:51 PDT
<rdar://problem/52445927>
Comment 2 Jiewen Tan 2019-10-02 22:49:57 PDT
Created attachment 380081 [details]
Patch
Comment 3 Jiewen Tan 2019-10-03 02:01:19 PDT
Created attachment 380091 [details]
Patch
Comment 4 Chris Dumez 2019-10-03 08:42:24 PDT
Comment on attachment 380091 [details]
Patch

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

> Source/WebCore/workers/WorkerGlobalScope.cpp:417
> +    m_thread.workerLoaderProxy().postTaskToLoader([resultContainer = resultContainer.copyRef(), key, wrappedKeyContainer = wrappedKeyContainer.copyRef(), doneContainer = doneContainer.copyRef(), workerMessagingProxy = Ref<WorkerMessagingProxy>(downcast<WorkerMessagingProxy>(m_thread.workerLoaderProxy()))](ScriptExecutionContext& context) {

Ref'ing the WorkerMessagingProxy and passing it to another thread like this is not correct. If you look at the constructor / destructor of WorkerMessagingProxy, you'll see that this object needs to be constructed and destroyed on the same thread. The fact that it subclasses ThreadSafeRefCounted<> does not change that.

> Source/WebCore/workers/WorkerGlobalScope.cpp:420
> +        ((WorkerLoaderProxy&)workerMessagingProxy.get()).postTaskForModeToWorkerGlobalScope([](ScriptExecutionContext& context) {

No C-casts please.

> Source/WebCore/workers/WorkerGlobalScope.cpp:427
>          waitResult = m_thread.runLoop().runInMode(this, WorkerRunLoop::defaultMode());

What makes sure that |this| stays alive while spinning the run loop? I do not see you holding a Ref<> to the WorkerGlobalScope, which seems wrong.

> Tools/ChangeLog:9
> +        Modifies IndexedDB.StructuredCloneBackwardCompatibility test to include CryptoKeys.

Do these reproduce the crash?
Comment 5 Jiewen Tan 2019-10-06 16:51:44 PDT
Comment on attachment 380091 [details]
Patch

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

Thanks Chris for taking a look.

>> Source/WebCore/workers/WorkerGlobalScope.cpp:417
>> +    m_thread.workerLoaderProxy().postTaskToLoader([resultContainer = resultContainer.copyRef(), key, wrappedKeyContainer = wrappedKeyContainer.copyRef(), doneContainer = doneContainer.copyRef(), workerMessagingProxy = Ref<WorkerMessagingProxy>(downcast<WorkerMessagingProxy>(m_thread.workerLoaderProxy()))](ScriptExecutionContext& context) {
> 
> Ref'ing the WorkerMessagingProxy and passing it to another thread like this is not correct. If you look at the constructor / destructor of WorkerMessagingProxy, you'll see that this object needs to be constructed and destroyed on the same thread. The fact that it subclasses ThreadSafeRefCounted<> does not change that.

Thanks for pointing it out. I didn't consider that. However, it looks like that WorkerMessagingProxy expects itself to be destructed in main thread which is where this lambda is going to be executed. It looks fine for me then.

>> Source/WebCore/workers/WorkerGlobalScope.cpp:420
>> +        ((WorkerLoaderProxy&)workerMessagingProxy.get()).postTaskForModeToWorkerGlobalScope([](ScriptExecutionContext& context) {
> 
> No C-casts please.

Fixed.

>> Source/WebCore/workers/WorkerGlobalScope.cpp:427
>>          waitResult = m_thread.runLoop().runInMode(this, WorkerRunLoop::defaultMode());
> 
> What makes sure that |this| stays alive while spinning the run loop? I do not see you holding a Ref<> to the WorkerGlobalScope, which seems wrong.

Added.

>> Tools/ChangeLog:9
>> +        Modifies IndexedDB.StructuredCloneBackwardCompatibility test to include CryptoKeys.
> 
> Do these reproduce the crash?

No, none of the tests help reproduce the crash. The crash is very racy and it is very hard to reproduce. I will give it one more shot.
Comment 6 Jiewen Tan 2019-10-06 18:13:59 PDT
Created attachment 380305 [details]
Patch
Comment 7 Chris Dumez 2019-10-07 08:34:16 PDT
Comment on attachment 380305 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Only wrapping CryptoKeys for IDB during serialization

EWS failures look legit.
Comment 8 Chris Dumez 2019-10-07 08:36:11 PDT
(In reply to Chris Dumez from comment #7)
> Comment on attachment 380305 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380305&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Only wrapping CryptoKeys for IDB during serialization
> 
> EWS failures look legit.

For example:
ASSERTION FAILED: isActive()
./Modules/indexeddb/IDBTransaction.cpp(1254) : Ref<WebCore::IDBRequest> WebCore::IDBTransaction::requestPutOrAdd(JSC::ExecState &, WebCore::IDBObjectStore &, RefPtr<WebCore::IDBKey> &&, WebCore::SerializedScriptValue &, IndexedDB::ObjectStoreOverwriteMode)
1   0x102724019 WTFCrash
2   0x1120c834b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x1138996dc WebCore::IDBTransaction::requestPutOrAdd(JSC::ExecState&, WebCore::IDBObjectStore&, WTF::RefPtr<WebCore::IDBKey, WTF::DumbPtrTraits<WebCore::IDBKey> >&&, WebCore::SerializedScriptValue&, WebCore::IndexedDB::ObjectStoreOverwriteMode)
4   0x113899325 WebCore::IDBObjectStore::putOrAdd(JSC::ExecState&, JSC::JSValue, WTF::RefPtr<WebCore::IDBKey, WTF::DumbPtrTraits<WebCore::IDBKey> >, WebCore::IndexedDB::ObjectStoreOverwriteMode, WebCore::IDBObjectStore::InlineKeyCheck)
5   0x113899433 WebCore::IDBObjectStore::put(JSC::ExecState&, JSC::JSValue, JSC::JSValue)
6   0x112d9e364 WebCore::jsIDBObjectStorePrototypeFunctionPutBody(JSC::ExecState*, WebCore::JSIDBObjectStore*, JSC::ThrowScope&)
7   0x112d812e0 long long WebCore::IDLOperation<WebCore::JSIDBObjectStore>::call<&(WebCore::jsIDBObjectStorePrototypeFunctionPutBody(JSC::ExecState*, WebCore::JSIDBObjectStore*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*)
8   0x112d80fcc WebCore::jsIDBObjectStorePrototypeFunctionPut(JSC::ExecState*)
9   0x2d4ae99894b
10  0x102c63562 op_call_slow_return_location_narrow
11  0x102c63562 op_call_slow_return_location_narrow
12  0x102c47063 vmEntryToJavaScript
13  0x1039772ce JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
14  0x1039778ff JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
15  0x103c5531c JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
16  0x103c5540a JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
17  0x103c556fe JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
18  0x113e9283b WebCore::JSExecState::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
19  0x113ee082a WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)
20  0x1145332b3 WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul>, WebCore::EventTarget::EventInvokePhase)
21  0x11452eac2 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase)
22  0x114508c67 WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const
23  0x114509781 WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&)
24  0x114509ba2 void WebCore::dispatchEventWithType<WebCore::EventTarget>(WTF::Vector<WebCore::EventTarget*, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::Event&)
25  0x114509a3d WebCore::EventDispatcher::dispatchEvent(WTF::Vector<WebCore::EventTarget*, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::Event&)
26  0x1138a1f2a WebCore::IDBRequest::dispatchEvent(WebCore::Event&)
27  0x1138a1a4e WebCore::IDBOpenDBRequest::dispatchEvent(WebCore::Event&)
28  0x1160464a0 WebCore::WorkerEventQueue::EventDispatcher::dispatch()
29  0x116046414 WebCore::WorkerEventQueue::enqueueEvent(WTF::Ref<WebCore::Event, WTF::DumbPtrTraits<WebCore::Event> >&&)::$_1::operator()(WebCore::ScriptExecutionContext&) const
30  0x116046204 WTF::Detail::CallableWrapper<WebCore::WorkerEventQueue::enqueueEvent(WTF::Ref<WebCore::Event, WTF::DumbPtrTraits<WebCore::Event> >&&)::$_1, void, WebCore::ScriptExecutionContext&>::call(WebCore::ScriptExecutionContext&)
31  0x113d1ae00 WTF::Function<void (WebCore::ScriptExecutionContext&)>::operator()(WebCore::ScriptExecutionContext&) const
Comment 9 Chris Dumez 2019-10-07 08:46:54 PDT
Comment on attachment 380305 [details]
Patch

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

r- due to failures.

> Source/WebCore/workers/WorkerGlobalScope.cpp:418
> +    m_thread.workerLoaderProxy().postTaskToLoader([resultContainer = resultContainer.copyRef(), key, wrappedKeyContainer = wrappedKeyContainer.copyRef(), doneContainer = doneContainer.copyRef(), workerMessagingProxy = Ref<WorkerMessagingProxy>(downcast<WorkerMessagingProxy>(m_thread.workerLoaderProxy()))](ScriptExecutionContext& context) {

Why not use makeRef() instead of Ref<WorkerMessagingProxy>() ?

> Source/WebCore/workers/WorkerGlobalScope.cpp:421
> +        static_cast<WorkerLoaderProxy&>(workerMessagingProxy.get()).postTaskForModeToWorkerGlobalScope([](ScriptExecutionContext& context) {

Why is the static_cast() needed?
Comment 10 Jiewen Tan 2019-10-07 11:36:56 PDT
(In reply to Chris Dumez from comment #7)
> Comment on attachment 380305 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380305&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Only wrapping CryptoKeys for IDB during serialization
> 
> EWS failures look legit.

Not sure how. Will talk to Brady or Sihui about this.
Comment 11 Jiewen Tan 2019-10-07 11:40:58 PDT
Comment on attachment 380305 [details]
Patch

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

>> Source/WebCore/workers/WorkerGlobalScope.cpp:418
>> +    m_thread.workerLoaderProxy().postTaskToLoader([resultContainer = resultContainer.copyRef(), key, wrappedKeyContainer = wrappedKeyContainer.copyRef(), doneContainer = doneContainer.copyRef(), workerMessagingProxy = Ref<WorkerMessagingProxy>(downcast<WorkerMessagingProxy>(m_thread.workerLoaderProxy()))](ScriptExecutionContext& context) {
> 
> Why not use makeRef() instead of Ref<WorkerMessagingProxy>() ?

Fixed.

>> Source/WebCore/workers/WorkerGlobalScope.cpp:421
>> +        static_cast<WorkerLoaderProxy&>(workerMessagingProxy.get()).postTaskForModeToWorkerGlobalScope([](ScriptExecutionContext& context) {
> 
> Why is the static_cast() needed?

postTaskForModeToWorkerGlobalScope is not exposed in WorkerMessagingProxy, and therefore I need to upcast the object to WorkerLoaderProxy to use it.
Comment 12 Jiewen Tan 2019-10-07 12:50:04 PDT
Comment on attachment 380305 [details]
Patch

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

>>>> Source/WebCore/ChangeLog:3
>>>> +        Only wrapping CryptoKeys for IDB during serialization
>>> 
>>> EWS failures look legit.
>> 
>> For example:
>> ASSERTION FAILED: isActive()
>> ./Modules/indexeddb/IDBTransaction.cpp(1254) : Ref<WebCore::IDBRequest> WebCore::IDBTransaction::requestPutOrAdd(JSC::ExecState &, WebCore::IDBObjectStore &, RefPtr<WebCore::IDBKey> &&, WebCore::SerializedScriptValue &, IndexedDB::ObjectStoreOverwriteMode)
>> 1   0x102724019 WTFCrash
>> 2   0x1120c834b WTFCrashWithInfo(int, char const*, char const*, int)
>> 3   0x1138996dc WebCore::IDBTransaction::requestPutOrAdd(JSC::ExecState&, WebCore::IDBObjectStore&, WTF::RefPtr<WebCore::IDBKey, WTF::DumbPtrTraits<WebCore::IDBKey> >&&, WebCore::SerializedScriptValue&, WebCore::IndexedDB::ObjectStoreOverwriteMode)
>> 4   0x113899325 WebCore::IDBObjectStore::putOrAdd(JSC::ExecState&, JSC::JSValue, WTF::RefPtr<WebCore::IDBKey, WTF::DumbPtrTraits<WebCore::IDBKey> >, WebCore::IndexedDB::ObjectStoreOverwriteMode, WebCore::IDBObjectStore::InlineKeyCheck)
>> 5   0x113899433 WebCore::IDBObjectStore::put(JSC::ExecState&, JSC::JSValue, JSC::JSValue)
>> 6   0x112d9e364 WebCore::jsIDBObjectStorePrototypeFunctionPutBody(JSC::ExecState*, WebCore::JSIDBObjectStore*, JSC::ThrowScope&)
>> 7   0x112d812e0 long long WebCore::IDLOperation<WebCore::JSIDBObjectStore>::call<&(WebCore::jsIDBObjectStorePrototypeFunctionPutBody(JSC::ExecState*, WebCore::JSIDBObjectStore*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*)
>> 8   0x112d80fcc WebCore::jsIDBObjectStorePrototypeFunctionPut(JSC::ExecState*)
>> 9   0x2d4ae99894b
>> 10  0x102c63562 op_call_slow_return_location_narrow
>> 11  0x102c63562 op_call_slow_return_location_narrow
>> 12  0x102c47063 vmEntryToJavaScript
>> 13  0x1039772ce JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
>> 14  0x1039778ff JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
>> 15  0x103c5531c JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
>> 16  0x103c5540a JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
>> 17  0x103c556fe JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
>> 18  0x113e9283b WebCore::JSExecState::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
>> 19  0x113ee082a WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)
>> 20  0x1145332b3 WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul>, WebCore::EventTarget::EventInvokePhase)
>> 21  0x11452eac2 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase)
>> 22  0x114508c67 WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const
>> 23  0x114509781 WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&)
>> 24  0x114509ba2 void WebCore::dispatchEventWithType<WebCore::EventTarget>(WTF::Vector<WebCore::EventTarget*, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::Event&)
>> 25  0x114509a3d WebCore::EventDispatcher::dispatchEvent(WTF::Vector<WebCore::EventTarget*, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::Event&)
>> 26  0x1138a1f2a WebCore::IDBRequest::dispatchEvent(WebCore::Event&)
>> 27  0x1138a1a4e WebCore::IDBOpenDBRequest::dispatchEvent(WebCore::Event&)
>> 28  0x1160464a0 WebCore::WorkerEventQueue::EventDispatcher::dispatch()
>> 29  0x116046414 WebCore::WorkerEventQueue::enqueueEvent(WTF::Ref<WebCore::Event, WTF::DumbPtrTraits<WebCore::Event> >&&)::$_1::operator()(WebCore::ScriptExecutionContext&) const
>> 30  0x116046204 WTF::Detail::CallableWrapper<WebCore::WorkerEventQueue::enqueueEvent(WTF::Ref<WebCore::Event, WTF::DumbPtrTraits<WebCore::Event> >&&)::$_1, void, WebCore::ScriptExecutionContext&>::call(WebCore::ScriptExecutionContext&)
>> 31  0x113d1ae00 WTF::Function<void (WebCore::ScriptExecutionContext&)>::operator()(WebCore::ScriptExecutionContext&) const
> 
> Not sure how. Will talk to Brady or Sihui about this.

Without my change, indexeddb crashes at the same location with my test. Bug 202648.
Comment 13 Jiewen Tan 2019-10-07 12:54:18 PDT
Created attachment 380347 [details]
Patch
Comment 14 Chris Dumez 2019-10-07 13:01:35 PDT
(In reply to Jiewen Tan from comment #11)
> Comment on attachment 380305 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380305&action=review
> 
> >> Source/WebCore/workers/WorkerGlobalScope.cpp:418
> >> +    m_thread.workerLoaderProxy().postTaskToLoader([resultContainer = resultContainer.copyRef(), key, wrappedKeyContainer = wrappedKeyContainer.copyRef(), doneContainer = doneContainer.copyRef(), workerMessagingProxy = Ref<WorkerMessagingProxy>(downcast<WorkerMessagingProxy>(m_thread.workerLoaderProxy()))](ScriptExecutionContext& context) {
> > 
> > Why not use makeRef() instead of Ref<WorkerMessagingProxy>() ?
> 
> Fixed.
> 
> >> Source/WebCore/workers/WorkerGlobalScope.cpp:421
> >> +        static_cast<WorkerLoaderProxy&>(workerMessagingProxy.get()).postTaskForModeToWorkerGlobalScope([](ScriptExecutionContext& context) {
> > 
> > Why is the static_cast() needed?
> 
> postTaskForModeToWorkerGlobalScope is not exposed in WorkerMessagingProxy,
> and therefore I need to upcast the object to WorkerLoaderProxy to use it.

WorkerMessagingProxy is a subclass of WorkerLoaderProxy so there should be no reason to cast. If the issue is that an override was marked as private in the subclass, then just make it public.
Or maybe the subclass has a good reason to make this private, in which case you'd have a bug.
Comment 15 Jiewen Tan 2019-10-07 13:29:29 PDT
(In reply to Chris Dumez from comment #14)
> (In reply to Jiewen Tan from comment #11)
> > Comment on attachment 380305 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=380305&action=review
> > 
> > >> Source/WebCore/workers/WorkerGlobalScope.cpp:418
> > >> +    m_thread.workerLoaderProxy().postTaskToLoader([resultContainer = resultContainer.copyRef(), key, wrappedKeyContainer = wrappedKeyContainer.copyRef(), doneContainer = doneContainer.copyRef(), workerMessagingProxy = Ref<WorkerMessagingProxy>(downcast<WorkerMessagingProxy>(m_thread.workerLoaderProxy()))](ScriptExecutionContext& context) {
> > > 
> > > Why not use makeRef() instead of Ref<WorkerMessagingProxy>() ?
> > 
> > Fixed.
> > 
> > >> Source/WebCore/workers/WorkerGlobalScope.cpp:421
> > >> +        static_cast<WorkerLoaderProxy&>(workerMessagingProxy.get()).postTaskForModeToWorkerGlobalScope([](ScriptExecutionContext& context) {
> > > 
> > > Why is the static_cast() needed?
> > 
> > postTaskForModeToWorkerGlobalScope is not exposed in WorkerMessagingProxy,
> > and therefore I need to upcast the object to WorkerLoaderProxy to use it.
> 
> WorkerMessagingProxy is a subclass of WorkerLoaderProxy so there should be
> no reason to cast. If the issue is that an override was marked as private in
> the subclass, then just make it public.
> Or maybe the subclass has a good reason to make this private, in which case
> you'd have a bug.

Made public.
Comment 16 Chris Dumez 2019-10-07 13:37:06 PDT
Still not concerned about those?
Tests that failed text/pixel/audio diff (2): flag all

 test 	results		actual failure	expected failure	history
+imported/w3c/web-platform-tests/2dcontext/imagebitmap/createImageBitmap-serializable.html	expected actual diff pretty diff		text	pass	history
+imported/w3c/web-platform-tests/workers/semantics/structured-clone/dedicated.html	expected actual diff pretty diff		text	pass	history


You did touch the structured cloning implementation which I why I believed they could be related.
Comment 17 Jiewen Tan 2019-10-07 14:05:01 PDT
Comment on attachment 380347 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:-3093
> -        case ImageBitmapTag:

Oops... I'm bad at merging stuffs...
Comment 18 Jiewen Tan 2019-10-07 14:09:51 PDT
Created attachment 380360 [details]
Patch
Comment 19 Chris Dumez 2019-10-07 14:14:07 PDT
Comment on attachment 380360 [details]
Patch

r=me assuming the bots are happy this time.
Comment 20 Jiewen Tan 2019-10-07 14:14:59 PDT
(In reply to Chris Dumez from comment #19)
> Comment on attachment 380360 [details]
> Patch
> 
> r=me assuming the bots are happy this time.

Thanks, Chris. I will only cq+ it once the bots are all green.
Comment 21 WebKit Commit Bot 2019-10-07 18:16:15 PDT
Comment on attachment 380360 [details]
Patch

Clearing flags on attachment: 380360

Committed r250811: <https://trac.webkit.org/changeset/250811>
Comment 22 WebKit Commit Bot 2019-10-07 18:16:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Truitt Savell 2019-10-08 09:32:07 PDT
The new test crypto/workers/subtle/aes-indexeddb.html

added in https://trac.webkit.org/changeset/250811/webkit

is timing out and crashing depending on platform.this is a constant crash on Mojave debug wk2

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=crypto%2Fworkers%2Fsubtle%2Faes-indexeddb.html

log:
ASSERTION FAILED: isActive()
./Modules/indexeddb/IDBTransaction.cpp(1254) : Ref<WebCore::IDBRequest> WebCore::IDBTransaction::requestPutOrAdd(JSC::ExecState &, WebCore::IDBObjectStore &, RefPtr<WebCore::IDBKey> &&, WebCore::SerializedScriptValue &, IndexedDB::ObjectStoreOverwriteMode)
1   0x552cf0239 WTFCrash
Comment 24 Jiewen Tan 2019-10-08 11:15:31 PDT
(In reply to Truitt Savell from comment #23)
> The new test crypto/workers/subtle/aes-indexeddb.html
> 
> added in https://trac.webkit.org/changeset/250811/webkit
> 
> is timing out and crashing depending on platform.this is a constant crash on
> Mojave debug wk2
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=crypto%2Fworkers%2Fsubtle%2Faes-indexeddb.html
> 
> log:
> ASSERTION FAILED: isActive()
> ./Modules/indexeddb/IDBTransaction.cpp(1254) : Ref<WebCore::IDBRequest>
> WebCore::IDBTransaction::requestPutOrAdd(JSC::ExecState &,
> WebCore::IDBObjectStore &, RefPtr<WebCore::IDBKey> &&,
> WebCore::SerializedScriptValue &, IndexedDB::ObjectStoreOverwriteMode)
> 1   0x552cf0239 WTFCrash

Could you add some expectations? Here is the bug I have created. It is nothing to do with my code change here. IDB is flaky somehow.
Comment 25 Truitt Savell 2019-10-08 11:23:41 PDT
(In reply to Jiewen Tan from comment #24)
> Could you add some expectations? Here is the bug I have created. It is
> nothing to do with my code change here. IDB is flaky somehow.

Giving expectations like this to your new test would be the same as not having a test at all for your commit, which is against webkit policy. Is there a better way of resolving this?
Comment 26 Truitt Savell 2019-10-08 11:28:01 PDT
I do understand there are other tests in your commit but my concern is still there.
Comment 27 Jiewen Tan 2019-10-08 11:51:23 PDT
(In reply to Truitt Savell from comment #26)
> I do understand there are other tests in your commit but my concern is still
> there.

Here is the test gardening:
Committed r250844: <https://trac.webkit.org/changeset/250844>

We still have release macOS coverage.
Comment 28 Truitt Savell 2019-10-09 14:07:21 PDT
Marked this test as a Timeout on Mojave WK2 Release: https://trac.webkit.org/changeset/250931/webkit