[webkit-reviews] review granted: [Bug 182631] [Payment Request] Crash in PaymentRequest::canMakePayment() when Apple Pay payment method data is missing required fields : [Attachment 333435] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 182631: [Payment Request] Crash in PaymentRequest::canMakePayment() when
Apple Pay payment method data is missing required fields
https://bugs.webkit.org/show_bug.cgi?id=182631

Attachment 333435: Patch

https://bugs.webkit.org/attachment.cgi?id=333435&action=review




--- Comment #2 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 333435
  --> https://bugs.webkit.org/attachment.cgi?id=333435
Patch

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

r=me with suggestions.

> Source/WebCore/ChangeLog:16
> +	   release assertion is raised about there being an unexpected
exception in the VM.

I suggest /being/seeing/.

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:493
> +	   auto scope = DECLARE_CATCH_SCOPE(document.execState()->vm());

Is there a reason to put the CatchScope in here instead of at the top of the
function?  As a convention, the only time we should declare it in this local
scope is if there are parts of this function outside of this scope that can
throw exceptions.  Is that true?  If not, I suggest moving this declaration to
the top of the function.