Bug 203101 – PaymentRequest / PaymentResponse should not prevent entering the back/forward cache

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

PaymentRequest / PaymentResponse should not prevent entering the back/forward cache.

[Radar WebKit Bug Importer]

<rdar://problem/56744409>

[Andy Estes]

Created attachment 383151 [details]
Patch

[Chris Dumez]

Looks like EWS is unhappy.

[Chris Dumez]

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

[Andy Estes]

Created attachment 383155 [details]
Patch

[Andy Estes]

Created attachment 383162 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

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.

[Andy Estes]

(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)

[Andy Estes]

Created attachment 383270 [details]
Crash log

[Chris Dumez]

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.

[Andy Estes]

(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.

[Chris Dumez]

(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

[Andy Estes]

Created attachment 383284 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 383284 [details]
Patch

Clearing flags on attachment: 383284

Committed r252338: <https://trac.webkit.org/changeset/252338>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.