PaymentRequest / PaymentResponse should not prevent entering the back/forward cache.
<rdar://problem/56744409>
Created attachment 383151 [details] Patch
Looks like EWS is unhappy.
Comment on attachment 383151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383151&action=review > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:508 > + showPromise->queueTaskToSettleWithResultCallback(TaskSource::UserInteraction, [] { Soon won't be needed anymore: https://bugs.webkit.org/show_bug.cgi?id=203976
Created attachment 383155 [details] Patch
Created attachment 383162 [details] Patch
Comment on attachment 383162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383162&action=review > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:508 > + showPromise->queueTaskToSettleWithResultCallback(TaskSource::UserInteraction, [] { My patch landed so this is really not needed anymore.
Comment on attachment 383162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383162&action=review > Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:177 > +void DeferredPromise::queueTaskKeepingSelfAlive(TaskSource source, Function<void()>&& task) This is no longer needed now that my patch landed. > Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:170 > + void queueTaskToSettleWithResultCallback(TaskSource source, Function<ExceptionOr<typename IDLType::ParameterType>()>&& resultCallback) No longer needed. > Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:177 > + void queueTaskToSettleWithResultCallback(TaskSource source, Function<ExceptionOr<void>()>&& resultCallback) No longer needed.
(In reply to Chris Dumez from comment #8) > Comment on attachment 383162 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383162&action=review > > > Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:177 > > +void DeferredPromise::queueTaskKeepingSelfAlive(TaskSource source, Function<void()>&& task) > > This is no longer needed now that my patch landed. I rebased on top of your patch and switched to calling DOMPromiseDeferred::settle in my suspend() overrides, but two of my new tests now crash on ASSERT(vm.currentThreadIsHoldingAPILock()) in JSC::sanitizeStackForVM when the promises are being settled in the WindowEventLoop. CRASHING TEST: /paymentrequest/page-cache-interactive-payment-request.https.html Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 JavaScriptCore 0x0000000240a66d9e WTFCrash + 14 (Assertions.cpp:305) 1 JavaScriptCore 0x0000000241c3307b WTFCrashWithInfo(int, char const*, char const*, int) + 27 2 JavaScriptCore 0x0000000241db8ccf JSC::sanitizeStackForVM(JSC::VM&) + 143 (VM.cpp:1157) 3 JavaScriptCore 0x00000002415d2e02 JSC::Heap::runCurrentPhase(JSC::GCConductor, JSC::CurrentThreadState*) + 98 4 JavaScriptCore 0x00000002415d7d4f JSC::Heap::collectInMutatorThread() + 47 (Heap.cpp:1838) 5 JavaScriptCore 0x00000002415d7b6b JSC::Heap::stopIfNecessarySlow(unsigned int) + 363 (Heap.cpp:1831) 6 JavaScriptCore 0x00000002415d78d7 JSC::Heap::stopIfNecessarySlow() + 151 (Heap.cpp:1800) 7 JavaScriptCore 0x00000002415d271f JSC::Heap::stopIfNecessary() + 143 (HeapInlines.h:270) 8 JavaScriptCore 0x00000002415cf0e2 JSC::Heap::collectIfNecessaryOrDefer(JSC::GCDeferralContext*) + 434 9 JavaScriptCore 0x00000002415da7b3 JSC::Heap::decrementDeferralDepthAndGCIfNeededSlow() + 163 (Heap.cpp:2667) 10 JavaScriptCore 0x0000000241b6a058 JSC::Heap::decrementDeferralDepthAndGCIfNeeded() + 184 (HeapInlines.h:215) 11 JavaScriptCore 0x0000000241a4607f JSC::DeferGC::~DeferGC() + 127 (DeferGC.h:49) 12 JavaScriptCore 0x0000000241b5ee15 JSC::DeferGC::~DeferGC() + 21 (DeferGC.h:49) 13 JavaScriptCore 0x0000000240df6733 JSC::GCSafeConcurrentJSLocker::~GCSafeConcurrentJSLocker() + 51 (ConcurrentJSLock.h:91) 14 JavaScriptCore 0x0000000240df6365 JSC::GCSafeConcurrentJSLocker::~GCSafeConcurrentJSLocker() + 21 (ConcurrentJSLock.h:91) 15 JavaScriptCore 0x0000000241d779e9 int JSC::Structure::add<(JSC::Structure::ShouldPin)0, JSC::Structure::add(JSC::VM&, JSC::PropertyName, unsigned int)::$_4>(JSC::VM&, JSC::PropertyName, unsigned int, JSC::Structure::add(JSC::VM&, JSC::PropertyName, unsigned int)::$_4 const&) + 697 (StructureInlines.h:460) 16 JavaScriptCore 0x0000000241d7504e JSC::Structure::add(JSC::VM&, JSC::PropertyName, unsigned int) + 62 (Structure.cpp:962) 17 JavaScriptCore 0x0000000241d74e95 JSC::Structure::addNewPropertyTransition(JSC::VM&, JSC::Structure*, JSC::PropertyName, unsigned int, int&, JSC::PutPropertySlot::Context, JSC::DeferredStructureTransitionWatchpointFire*) + 1093 (Structure.cpp:508) 18 JavaScriptCore 0x0000000241a48cfc bool JSC::JSObject::putDirectInternal<(JSC::JSObject::PutMode)1>(JSC::VM&, JSC::PropertyName, JSC::JSValue, unsigned int, JSC::PutPropertySlot&) + 2812 (JSObjectInlines.h:375) 19 JavaScriptCore 0x0000000241a382a2 JSC::JSObject::putDirect(JSC::VM&, JSC::PropertyName, JSC::JSValue, unsigned int) + 354 (JSObject.h:1490) 20 JavaScriptCore 0x0000000241c49e7f JSC::JSObject::putDirectBuiltinFunction(JSC::VM&, JSC::JSGlobalObject*, JSC::PropertyName const&, JSC::FunctionExecutable*, unsigned int) + 271 (JSObject.cpp:3203) 21 JavaScriptCore 0x0000000241c44a6d JSC::reifyStaticProperty(JSC::VM&, JSC::ClassInfo const*, JSC::PropertyName const&, JSC::HashTableValue const&, JSC::JSObject&) + 269 (Lookup.h:329) 22 JavaScriptCore 0x0000000241cc4ce0 JSC::setUpStaticFunctionSlot(JSC::VM&, JSC::ClassInfo const*, JSC::HashTableValue const*, JSC::JSObject*, JSC::PropertyName, JSC::PropertySlot&) + 384 (Lookup.cpp:63) 23 JavaScriptCore 0x0000000241c43216 JSC::getStaticPropertySlotFromTable(JSC::VM&, JSC::ClassInfo const*, JSC::HashTable const&, JSC::JSObject*, JSC::PropertyName, JSC::PropertySlot&) + 182 (Lookup.h:233) 24 JavaScriptCore 0x0000000241c4310e JSC::JSObject::getOwnStaticPropertySlot(JSC::VM&, JSC::PropertyName, JSC::PropertySlot&) + 126 (JSObject.cpp:2215) 25 JavaScriptCore 0x0000000241c56c79 JSC::JSObject::getOwnNonIndexPropertySlot(JSC::VM&, JSC::Structure*, JSC::PropertyName, JSC::PropertySlot&) + 153 (JSObject.h:1355) 26 JavaScriptCore 0x0000000241a4a3b4 bool JSC::JSObject::getPropertySlot<false>(JSC::JSGlobalObject*, JSC::PropertyName, JSC::PropertySlot&) + 436 (JSObject.h:1444) 27 JavaScriptCore 0x0000000241b653be JSC::JSValue::getPropertySlot(JSC::JSGlobalObject*, JSC::PropertyName, JSC::PropertySlot&) const + 670 (JSCJSValueInlines.h:909) 28 JavaScriptCore 0x0000000241a4d278 JSC::JSValue::get(JSC::JSGlobalObject*, JSC::PropertyName, JSC::PropertySlot&) const + 184 (JSCJSValueInlines.h:866) 29 JavaScriptCore 0x0000000241892965 llint_slow_path_get_by_id + 437 (LLIntSlowPaths.cpp:770) 30 JavaScriptCore 0x0000000240dc1a5e llint_entry + 40048 31 JavaScriptCore 0x0000000240db7b83 vmEntryToJavaScript + 273 32 JavaScriptCore 0x0000000241785977 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 199 (JITCodeInlines.h:38) 33 JavaScriptCore 0x0000000241785fc6 JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1494 (Interpreter.cpp:905) 34 JavaScriptCore 0x0000000241a76859 JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 233 (CallData.cpp:59) 35 JavaScriptCore 0x0000000241c4ebe6 JSC::callFunction(JSC::JSGlobalObject*, JSC::JSValue, JSC::JSPromise*, JSC::JSValue) + 438 (JSPromise.cpp:154) 36 JavaScriptCore 0x0000000241c4ee44 JSC::JSPromise::reject(JSC::JSGlobalObject*, JSC::JSValue) + 388 (JSPromise.cpp:180) 37 com.apple.WebCore 0x00000002473598e8 WebCore::DeferredPromise::callFunction(JSC::JSGlobalObject&, WebCore::DeferredPromise::ResolveMode, JSC::JSValue) + 440 38 com.apple.WebCore 0x000000024739cd9c WebCore::DeferredPromise::callFunction(JSC::JSGlobalObject&, WebCore::DeferredPromise::ResolveMode, JSC::JSValue)::$_31::operator()() + 108 (JSDOMPromiseDeferred.cpp:59) 39 com.apple.WebCore 0x000000024739cc4e WTF::Detail::CallableWrapper<WebCore::DeferredPromise::callFunction(JSC::JSGlobalObject&, WebCore::DeferredPromise::ResolveMode, JSC::JSValue)::$_31, void>::call() + 30 (Function.h:52) 40 com.apple.WebCore 0x0000000245583df2 WTF::Function<void ()>::operator()() const + 130 (Function.h:79) 41 com.apple.WebCore 0x0000000247acf7ee WebCore::WindowEventLoop::run() + 382 (WindowEventLoop.cpp:110)
Created attachment 383270 [details] Crash log
Oh, I think I found the bug. Can you try this? diff --git a/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp b/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp index 0bc5a89984b..972544a5250 100644 --- a/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp +++ b/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp @@ -52,8 +52,12 @@ void DeferredPromise::callFunction(JSGlobalObject& lexicalGlobalObject, ResolveM if (activeDOMObjectsAreSuspended()) { JSC::Strong<JSC::Unknown, ShouldStrongDestructorGrabLock::Yes> strongResolution(lexicalGlobalObject.vm(), resolution); scriptExecutionContext()->eventLoop().queueTask(TaskSource::Networking, *scriptExecutionContext(), [this, protectedThis = makeRef(*this), mode, strongResolution = WTFMove(strongResolution)]() mutable { - if (!shouldIgnoreRequestToFulfill()) - callFunction(*globalObject(), mode, strongResolution.get()); + if (shouldIgnoreRequestToFulfill()) + return; + + JSC::JSGlobalObject* lexicalGlobalObject = globalObject(); + JSC::JSLockHolder locker(lexicalGlobalObject); + callFunction(*lexicalGlobalObject, mode, strongResolution.get()); }); return; } I forgot to re-grab the JS lock when the lambda runs.
(In reply to Chris Dumez from comment #11) > Oh, I think I found the bug. Can you try this? > diff --git a/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp > b/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp > index 0bc5a89984b..972544a5250 100644 > --- a/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp > +++ b/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp > @@ -52,8 +52,12 @@ void DeferredPromise::callFunction(JSGlobalObject& > lexicalGlobalObject, ResolveM > if (activeDOMObjectsAreSuspended()) { > JSC::Strong<JSC::Unknown, ShouldStrongDestructorGrabLock::Yes> > strongResolution(lexicalGlobalObject.vm(), resolution); > > scriptExecutionContext()->eventLoop().queueTask(TaskSource::Networking, > *scriptExecutionContext(), [this, protectedThis = makeRef(*this), mode, > strongResolution = WTFMove(strongResolution)]() mutable { > - if (!shouldIgnoreRequestToFulfill()) > - callFunction(*globalObject(), mode, strongResolution.get()); > + if (shouldIgnoreRequestToFulfill()) > + return; > + > + JSC::JSGlobalObject* lexicalGlobalObject = globalObject(); > + JSC::JSLockHolder locker(lexicalGlobalObject); > + callFunction(*lexicalGlobalObject, mode, > strongResolution.get()); > }); > return; > } > > I forgot to re-grab the JS lock when the lambda runs. Looks good now, thanks.
(In reply to Andy Estes from comment #12) > (In reply to Chris Dumez from comment #11) > > Oh, I think I found the bug. Can you try this? > > diff --git a/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp > > b/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp > > index 0bc5a89984b..972544a5250 100644 > > --- a/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp > > +++ b/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp > > @@ -52,8 +52,12 @@ void DeferredPromise::callFunction(JSGlobalObject& > > lexicalGlobalObject, ResolveM > > if (activeDOMObjectsAreSuspended()) { > > JSC::Strong<JSC::Unknown, ShouldStrongDestructorGrabLock::Yes> > > strongResolution(lexicalGlobalObject.vm(), resolution); > > > > scriptExecutionContext()->eventLoop().queueTask(TaskSource::Networking, > > *scriptExecutionContext(), [this, protectedThis = makeRef(*this), mode, > > strongResolution = WTFMove(strongResolution)]() mutable { > > - if (!shouldIgnoreRequestToFulfill()) > > - callFunction(*globalObject(), mode, strongResolution.get()); > > + if (shouldIgnoreRequestToFulfill()) > > + return; > > + > > + JSC::JSGlobalObject* lexicalGlobalObject = globalObject(); > > + JSC::JSLockHolder locker(lexicalGlobalObject); > > + callFunction(*lexicalGlobalObject, mode, > > strongResolution.get()); > > }); > > return; > > } > > > > I forgot to re-grab the JS lock when the lambda runs. > > Looks good now, thanks. Great, let’s land with this fix then. Still r=me
Created attachment 383284 [details] Patch
Comment on attachment 383284 [details] Patch Clearing flags on attachment: 383284 Committed r252338: <https://trac.webkit.org/changeset/252338>
All reviewed patches have been landed. Closing bug.