Complete crash when calling PaymentRequestUpdateEvent.updateWith({})
Reported by
mcace...@mozilla.com,
Aug 21 2017
|
|||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3191.0 Safari/537.36 Steps to reproduce the problem: 1. run the crash script attached. 2. in the payment sheet, try to change address What is the expected behavior? For the browser not to crash. What went wrong? The whole browser crashed 🙀 Did this work before? N/A Does this work in other browsers? Yes Chrome version: 62.0.3191.0 Channel: canary OS Version: OS X 10.12.6 Flash Version:
,
Aug 21 2017
,
Aug 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4cbda82c0488efd0651651f892f11491877afcf6 commit 4cbda82c0488efd0651651f892f11491877afcf6 Author: Rouslan Solomakhin <rouslan@chromium.org> Date: Wed Aug 23 18:50:39 2017 [Payments] Handle empty dictionary in updateWith call. Before this patch, calling PaymentRequestUpdateEvent.updateWith({}) would crash desktop Chrome, because the browser assumed that the total would always be present in {} without enforcing it. This patch adds integration tests for updateWith({}) on Android and desktop. (These tests fail on desktop without a fix.) The patch fixes the crash by enforcing the presence of "total" in both renderer and desktop browser side. This patch brings the desktop implementation in line with Android and fixes the crash, although this behavior is not exactly matching the spec. Work on matching the spec closer is continuing. After this patch, calling PaymentRequestUpdateEvent.updateWith({}) will reject the PaymentRequest.show() promise with SyntaxError and a message "Total required." Bug: 757285 Change-Id: If2209dbad2687c935d6e47fc68df2ea92375a8e5 Reviewed-on: https://chromium-review.googlesource.com/626482 Reviewed-by: Anthony Vallee-Dubois <anthonyvd@chromium.org> Reviewed-by: Ganggui Tang <gogerald@chromium.org> Commit-Queue: Rouslan Solomakhin <rouslan@chromium.org> Cr-Commit-Position: refs/heads/master@{#496747} [modify] https://crrev.com/4cbda82c0488efd0651651f892f11491877afcf6/chrome/android/java_sources.gni [add] https://crrev.com/4cbda82c0488efd0651651f892f11491877afcf6/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmptyUpdateTest.java [add] https://crrev.com/4cbda82c0488efd0651651f892f11491877afcf6/chrome/browser/ui/views/payments/empty_update_browsertest.cc [modify] https://crrev.com/4cbda82c0488efd0651651f892f11491877afcf6/chrome/test/BUILD.gn [modify] https://crrev.com/4cbda82c0488efd0651651f892f11491877afcf6/components/payments/content/payment_request.cc [add] https://crrev.com/4cbda82c0488efd0651651f892f11491877afcf6/components/test/data/payments/empty_update.js [add] https://crrev.com/4cbda82c0488efd0651651f892f11491877afcf6/components/test/data/payments/payment_request_empty_update_test.html [modify] https://crrev.com/4cbda82c0488efd0651651f892f11491877afcf6/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
,
Aug 23 2017
,
Aug 23 2017
Waiting for canary bake, then will request a merge.
,
Aug 24 2017
Any reason why you are using SyntaxError? IIRC, WebIDL says to use TypeError for conversions. It's what we assume in the WPT test suite. Unrelated: I spotted a few other SyntaxError being thrown... the ones to do with currency code validation should be RangeError instead (as per spec).
,
Aug 24 2017
I'm using SyntaxError to stay consistent with the rest of the PaymentRequest code. You're correct that's not spec-compliant. I'm working on spec compliance in parallel. Your tests are very helpful for that. Thank you!
,
Aug 25 2017
,
Aug 25 2017
This bug requires manual review: We are only 10 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 26 2017
Before we approve merge to M61, please answer followings: * Is this M61 regression? Is it critical? (currently marked as P2 and not a blocker) * Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M61? * Any other important details to justify the merge. Please note We're only few weeks away from M61 Stable promotion, so merge bar is very high. Thank you.
,
Aug 28 2017
Rechecked this issue on MAC 10.12.6 using chrome version 62.0.3198.0 and still able to reproduce the crash after changing and saving the address Crash ID: 3cca6fa22a5c9ac2 Stack Trace: ============ CRASHED [EXC_BAD_ACCESS / KERN_INVALID_ADDRESS @ 0x000000d8 ] MAGIC SIGNATURE THREAD Stack Quality79%Show frame trust levels 0x00000001103be5d8 (Google Chrome Framework -string:1221 ) i18n::addressinput::StringCompare::NaturalEquals(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) const 0x00000001103c043c (Google Chrome Framework -address_normalizer.cc:83 ) i18n::addressinput::AddressNormalizer::Normalize(i18n::addressinput::AddressData*) const 0x00000001103ba4f5 (Google Chrome Framework -chrome_address_validator.cc:156 ) autofill::AddressValidator::NormalizeAddress(i18n::addressinput::AddressData*) const 0x00000001103e84cf (Google Chrome Framework -address_normalizer_impl.cc:84 ) payments::(anonymous namespace)::AddressNormalizationRequest::OnRulesLoaded(bool) 0x00000001103e825b (Google Chrome Framework -address_normalizer_impl.cc:183 ) payments::AddressNormalizerImpl::OnAddressValidationRulesLoaded(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, bool) 0x00000001103b970d (Google Chrome Framework -chrome_address_validator.cc:181 ) autofill::AddressValidator::RulesLoaded(bool, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int) 0x00000001103c301c (Google Chrome Framework -preload_supplier.cc:249 ) i18n::addressinput::(anonymous namespace)::Helper::OnRetrieved(bool, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) 0x00000001103c5d6b (Google Chrome Framework -retriever.cc:60 ) i18n::addressinput::(anonymous namespace)::Helper::OnValidatedDataReady(bool, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*) 0x00000001103c6cac (Google Chrome Framework -validating_storage.cc:66 ) i18n::addressinput::(anonymous namespace)::Helper::OnWrappedDataReady(bool, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*) 0x00000001103bb50d (Google Chrome Framework -chrome_storage_impl.cc:58 ) autofill::ChromeStorageImpl::DoGet(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, i18n::addressinput::Callback<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*> const&) 0x00000001103c19f7 (Google Chrome Framework -preload_supplier.cc:90 ) i18n::addressinput::PreloadSupplier::LoadRules(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, i18n::addressinput::Callback<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int> const&) 0x00000001103b9baa (Google Chrome Framework -chrome_address_validator.cc:60 ) autofill::AddressValidator::LoadRules(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) 0x00000001103e81fa (Google Chrome Framework -address_normalizer_impl.cc:172 ) payments::AddressNormalizerImpl::StartAddressNormalization(autofill::AutofillProfile const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int, payments::AddressNormalizer::Delegate*) 0x00000001110d84f8 (Google Chrome Framework -payment_request_state.cc:226 ) payments::PaymentRequestState::SetSelectedShippingProfile(autofill::AutofillProfile*) 0x0000000110ec0069 (Google Chrome Framework -callback.h:91 ) payments::ShippingAddressEditorViewController::ValidateModelAndSave() 0x0000000110eaf91d (Google Chrome Framework -editor_view_controller.cc:167 ) payments::EditorViewController::ButtonPressed(views::Button*, ui::Event const&) 0x0000000110066dfc (Google Chrome Framework -button.cc:241 ) views::Button::OnMouseReleased(ui::MouseEvent const&) 0x000000011004fda1 (Google Chrome Framework -ink_drop_host_view.cc:263 ) views::InkDropHostView::OnMouseEvent(ui::MouseEvent*) 0x000000010f531941 (Google Chrome Framework -scoped_target_handler.cc:32 ) ui::ScopedTargetHandler::OnEvent(ui::Event*) 0x000000010f52f5ed (Google Chrome Framework -event_dispatcher.cc:191 ) ui::EventDispatcher::ProcessEvent(ui::EventTarget*, ui::Event*) 0x000000010f52f427 (Google Chrome Framework -event_dispatcher.cc:86 ) ui::EventDispatcherDelegate::DispatchEvent(ui::EventTarget*, ui::Event*) 0x00000001100c5318 (Google Chrome Framework -root_view.cc:435 ) views::internal::RootView::OnMouseReleased(ui::MouseEvent const&) 0x00000001100ca096 (Google Chrome Framework -widget.cc:1216 ) views::Widget::OnMouseEvent(ui::MouseEvent*) 0x000000011005cae5 (Google Chrome Framework -bridged_content_view.mm:642 ) -[BridgedContentView mouseEvent:] 0x00000001100639b2 (Google Chrome Framework -cocoa_mouse_capture.mm:78 ) ___ZN5views17CocoaMouseCapture14ActiveEventTap4InitEv_block_invoke 0x00007fffc84767f9 (AppKit + 0x001c77f9 ) _NSSendEventToObservers 0x00007fffc8a6f23e (AppKit + 0x007c023e ) -[NSApplication(NSEvent) sendEvent:] 0x000000010e365e4b (Google Chrome Framework -chrome_browser_application_mac.mm:277 ) __34-[BrowserCrApplication sendEvent:]_block_invoke 0x000000010e7798e9 (Google Chrome Framework + 0x01b798e9 ) base::mac::CallWithEHFrame(void () block_pointer) 0x000000010e365c36 (Google Chrome Framework -chrome_browser_application_mac.mm:261 ) -[BrowserCrApplication sendEvent:] 0x00007fffc82ea426 (AppKit + 0x0003b426 ) -[NSApplication run] 0x000000010e7890cd (Google Chrome Framework -message_pump_mac.mm:749 ) base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) 0x000000010e787a6b (Google Chrome Framework -message_pump_mac.mm:141 ) base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) 0x000000010e7aab72 (Google Chrome Framework -run_loop.cc:123 ) base::RunLoop::Run() 0x000000010e36b0d7 (Google Chrome Framework -chrome_browser_main.cc:1916 ) ChromeBrowserMainParts::MainMessageLoopRun(int*) 0x000000010d249aa3 (Google Chrome Framework -browser_main_loop.cc:1196 ) content::BrowserMainLoop::RunMainMessageLoopParts() 0x000000010d24c091 (Google Chrome Framework -browser_main_runner.cc:152 ) content::BrowserMainRunnerImpl::Run() 0x000000010d245abb (Google Chrome Framework -browser_main.cc:46 ) content::BrowserMain(content::MainFunctionParams const&) 0x000000010e32125d (Google Chrome Framework -content_main_runner.cc:709 ) content::ContentMainRunnerImpl::Run() 0x000000010fbea26f (Google Chrome Framework -main.cc:469 ) service_manager::Main(service_manager::MainParams const&) 0x000000010e320813 (Google Chrome Framework -content_main.cc:19 ) content::ContentMain(content::ContentMainParams const&) 0x000000010cc03a8e (Google Chrome Framework -chrome_main.cc:122 ) ChromeMain 0x000000010cb86dd3 (Google Chrome Canary + 0x00000dd3 ) 0x00007fffdff7c234 (libdyld.dylib + 0x00005234 ) start @rouslan: Request you to please take a look into it.
,
Aug 28 2017
,
Aug 28 2017
ranjitkan@: Can you provide more detailed steps to reproduce? In particular, what address did you use?
,
Aug 28 2017
+Seb +Parastoo FYI go/crash/3cca6fa22a5c9ac2 is in the address normalizer.
,
Aug 28 2017
Re comment #10: > Is this M61 regression? Is it critical? (currently marked as P2 and not a blocker) No, this is a bug in the new feature that we plan to ship in M-61: web payments on desktop. > Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M61? Canary no longer shows the problem in the original bug report. The automation test is PaymentRequestEmptyUpdateTest. (Comment #11 is describing a different bug, which I will be fixing as well.)
,
Aug 28 2017
FYI, was able to reproduce the bug in comment #11 after editing an existing US address to have IN country code. Working on a fix now.
,
Aug 28 2017
In terms of risk, we have an experimental flag to disable the feature in case if anything goes wrong: WebPayments. This flag can also be manually controlled at chrome://flags/#web-payments.
,
Aug 28 2017
Re comment #11: That crash is a new regression introduced between 62.0.3193.3 (good) and 62.0.3196.0 (bad). It's not related to the original bug report in Chrome 61 and there's no merge required. I've moved comment #11 to bug https://crbug.com/759597 . Good thing we caught it early. Thank you!
,
Aug 28 2017
Approving merge to M61 branch 3163 based on comment #15, #17, #18 and per chat with rosulan@ the merge is safe, i.e., small. Please merge ASAP. Thank you.
,
Aug 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/df2950a8b81a34c6e735a799b2d0c1aae1429aaf commit df2950a8b81a34c6e735a799b2d0c1aae1429aaf Author: Rouslan Solomakhin <rouslan@chromium.org> Date: Mon Aug 28 15:24:58 2017 [Merge M-61] [Payments] Handle empty dictionary in updateWith call. Before this patch, calling PaymentRequestUpdateEvent.updateWith({}) would crash desktop Chrome, because the browser assumed that the total would always be present in {} without enforcing it. This patch adds integration tests for updateWith({}) on Android and desktop. (These tests fail on desktop without a fix.) The patch fixes the crash by enforcing the presence of "total" in both renderer and desktop browser side. This patch brings the desktop implementation in line with Android and fixes the crash, although this behavior is not exactly matching the spec. Work on matching the spec closer is continuing. After this patch, calling PaymentRequestUpdateEvent.updateWith({}) will reject the PaymentRequest.show() promise with SyntaxError and a message "Total required." TBR=rouslan@chromium.org (cherry picked from commit 4cbda82c0488efd0651651f892f11491877afcf6) Bug: 757285 Change-Id: If2209dbad2687c935d6e47fc68df2ea92375a8e5 Reviewed-on: https://chromium-review.googlesource.com/626482 Reviewed-by: Anthony Vallee-Dubois <anthonyvd@chromium.org> Reviewed-by: Ganggui Tang <gogerald@chromium.org> Commit-Queue: Rouslan Solomakhin <rouslan@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#496747} Reviewed-on: https://chromium-review.googlesource.com/638252 Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org> Cr-Commit-Position: refs/branch-heads/3163@{#923} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [modify] https://crrev.com/df2950a8b81a34c6e735a799b2d0c1aae1429aaf/chrome/android/java_sources.gni [add] https://crrev.com/df2950a8b81a34c6e735a799b2d0c1aae1429aaf/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmptyUpdateTest.java [add] https://crrev.com/df2950a8b81a34c6e735a799b2d0c1aae1429aaf/chrome/browser/ui/views/payments/empty_update_browsertest.cc [modify] https://crrev.com/df2950a8b81a34c6e735a799b2d0c1aae1429aaf/chrome/test/BUILD.gn [modify] https://crrev.com/df2950a8b81a34c6e735a799b2d0c1aae1429aaf/components/payments/content/payment_request.cc [add] https://crrev.com/df2950a8b81a34c6e735a799b2d0c1aae1429aaf/components/test/data/payments/empty_update.js [add] https://crrev.com/df2950a8b81a34c6e735a799b2d0c1aae1429aaf/components/test/data/payments/payment_request_empty_update_test.html [modify] https://crrev.com/df2950a8b81a34c6e735a799b2d0c1aae1429aaf/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
,
Aug 28 2017
Note for testers: The fix for bug in question can be verified by selecting a shipping address on https://rsolomakhin.github.io/pr/ko/empty/. Before the fix, the browser crashed. After the fix, the web page shows "SyntaxError: Total required" error message. For the next drop of M-61, any shipping address can be used for verification on https://rsolomakhin.github.io/pr/ko/empty/. For M-62, US works properly, but selecting an IN shipping address on https://rsolomakhin.github.io/pr/ko/empty/ will hit https://crbug.com/759597 . This is a separate issue from https://crbug.com/757285 and I'm working to resolve it asap as well.
,
Aug 29 2017
Rechecked this issue on MAC 10.12.6 using chrome version 62.0.3199.0 following the steps mentioned above and observed that fix is working as intended. For both the countries once submitting the address " SyntaxError: Total required" error message is displayed. Adding TE-Verified labels for M62 |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by nyerramilli@google.com
, Aug 21 2017