Bug 189386 – [Apple Pay] Dispatch a paymentmethodchange event when the payment method changes

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[Apple Pay] Dispatch a paymentmethodchange event when the payment method changes

[Andy Estes]

Created attachment 349095 [details]
Patch

[youenn fablet]

Comment on attachment 349095 [details]
Patch

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

> Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.h:60
>      JSC::Strong<JSC::JSObject> m_methodDetails;

Cannot it create some unbreakable reference cycles if methodDetails makes a reference to the PaymentMethodChangeEvent?
There is a potentially similar issue with IDB (bug 189201).
Ideally, we might want to keep a strong but clear it at creation of the JSPaymentMethodChangeEvent and do regular marking at that time.
Other approaches might be to use DOMGuarded to clear the strong at document destruction time but this may have observable differences.

[Andy Estes]

(In reply to youenn fablet from comment #2)
> Comment on attachment 349095 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349095&action=review
> 
> > Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.h:60
> >      JSC::Strong<JSC::JSObject> m_methodDetails;
> 
> Cannot it create some unbreakable reference cycles if methodDetails makes a
> reference to the PaymentMethodChangeEvent?
> There is a potentially similar issue with IDB (bug 189201).
> Ideally, we might want to keep a strong but clear it at creation of the
> JSPaymentMethodChangeEvent and do regular marking at that time.
> Other approaches might be to use DOMGuarded to clear the strong at document
> destruction time but this may have observable differences.

Yeah, I guess it could, but I don't think there's a reference cycle right now.

methodDetails is composed only of DOMStrings (see ApplePayPaymentMethod.idl). Is your concern that a script might add references to methodDetails after we fire the event?

What if we froze methodDetails to prevent scripts from modifying it?

I'll also check out your suggestions.

[Andy Estes]

Created attachment 349259 [details]
Patch

[EWS Watchlist]

Attachment 349259 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.cpp:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

(In reply to Andy Estes from comment #3)
> Is your concern that a script might add
> references to methodDetails after we fire the event?

Yes. This kind of thing can happen any time we use Strong in DOM bindings. That’s what the test fast/dom/reference-cycle-leaks.html is about, and ideally you would add new cases to that test to demonstrate that leaks do not occur.

One pattern to avoid this is that instead of Strong, we use JSValueInWrappedObject. Examples of this include CustomEvent.

> What if we froze methodDetails to prevent scripts from modifying it?

I don’t know exactly what "freezing" a JavaScript object would mean, but if we can somehow forbid attaching custom property values, than that would eliminate the reference cycle issue. But is that really compatible with what other web browsers would do?

[Andy Estes]

(In reply to Darin Adler from comment #6)
> (In reply to Andy Estes from comment #3)
> > Is your concern that a script might add
> > references to methodDetails after we fire the event?
> 
> Yes. This kind of thing can happen any time we use Strong in DOM bindings.
> That’s what the test fast/dom/reference-cycle-leaks.html is about, and
> ideally you would add new cases to that test to demonstrate that leaks do
> not occur.

Ok, good to know. I'll add a test to reference-cycle-leaks.html.

> One pattern to avoid this is that instead of Strong, we use
> JSValueInWrappedObject. Examples of this include CustomEvent.

I switched to using JSValueInWrappedObject in my latest patch.

[Andy Estes]

Created attachment 349261 [details]
Patch

[EWS Watchlist]

Attachment 349261 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.cpp:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 349261 [details]
Patch

Attachment 349261 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9144332

New failing tests:
fast/dom/reference-cycle-leaks.html

[EWS Watchlist]

Created attachment 349262 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Andy Estes]

Need to move the new reference-cycle-leaks result to platform/mac-wk2.

[EWS Watchlist]

Comment on attachment 349261 [details]
Patch

Attachment 349261 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9144451

New failing tests:
fast/dom/reference-cycle-leaks.html

[EWS Watchlist]

Created attachment 349263 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 349261 [details]
Patch

Attachment 349261 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9144393

New failing tests:
fast/dom/reference-cycle-leaks.html

[EWS Watchlist]

Created attachment 349264 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Andy Estes]

Created attachment 349265 [details]
Patch

[Darin Adler]

Comment on attachment 349261 [details]
Patch

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

> LayoutTests/fast/dom/reference-cycle-leaks.html:89
> +    leakDetectionNode.event = new PaymentMethodChangeEvent("x", { methodDetails: leakDetectionNode });

How does this handle platforms where we don't have PaymentMethodChangeEvent?

[EWS Watchlist]

Attachment 349265 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.cpp:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

(In reply to Darin Adler from comment #18)
> Comment on attachment 349261 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349261&action=review
> 
> > LayoutTests/fast/dom/reference-cycle-leaks.html:89
> > +    leakDetectionNode.event = new PaymentMethodChangeEvent("x", { methodDetails: leakDetectionNode });
> 
> How does this handle platforms where we don't have PaymentMethodChangeEvent?

In the new patch, the baseline expected result prints "---- Did not test PaymentMethodChangeEvent because it is not enabled." Then mac-wk2 and ios-wk2 have platform expected results with PASS messages.

[WebKit Commit Bot]

Comment on attachment 349265 [details]
Patch

Clearing flags on attachment: 349265

Committed r235833: <https://trac.webkit.org/changeset/235833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44268288>