[Apple Pay] Dispatch a paymentmethodchange event when the payment method changes
Created attachment 349095 [details] Patch
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.
(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.
Created attachment 349259 [details] Patch
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.
(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?
(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.
Created attachment 349261 [details] Patch
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.
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
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
Need to move the new reference-cycle-leaks result to platform/mac-wk2.
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
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
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
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
Created attachment 349265 [details] Patch
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?
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.
(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.
Comment on attachment 349265 [details] Patch Clearing flags on attachment: 349265 Committed r235833: <https://trac.webkit.org/changeset/235833>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44268288>