Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2874883002:
Fix chrome crash issue when payment modifiers are enabled on android. (Closed)
|
Created:
3 years, 7 months ago by wuandy1 Modified:
3 years, 7 months ago Reviewers:
gogerald1 CC:
chromium-reviews, mahmadi+paymentswatch_chromium.org, rouslan+payments_chromium.org, agrieve+watch_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix chrome crash issue when payment modifiers are enabled on android.
PaymentRequestImpl tries to call Set.retainAll in order to match payment
method and modifiers. However, the Set is Unmodifieable when returned
from AutoFill and other PaymentApp. This caused chrome crash. Fixed by
copying the set into a HashSet then do retainAll.
After above, a second bug is triggerd from inside updateOrderSummary
because it tries to dereference UI object before it is created. Simply
removed the dereference because it seems redundant.
BUG=718097
Review-Url: https://codereview.chromium.org/2874883002
Cr-Commit-Position: refs/heads/master@{#471385}
Committed: https://chromium.googlesource.com/chromium/src/+/7e09b31bb236f5fa84ee8c987c91329dc3e22509
Patch Set 1 #Patch Set 2 : deletion instead of commenting. #Patch Set 3 : fix a crash issue caused by trying to change unmodifiable set, and a subsquent issue where mUI is r… #
Total comments: 3
Patch Set 4 : more formatting #
Total comments: 4
Patch Set 5 : addressing reviewer comments #Messages
Total messages: 24 (18 generated)
The CQ bit was checked by wuandy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wuandy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix chrome crash issue when payment modifiers are enabled on android. PaymentRequestImpl tries to call Set.retainAll in order to match payment method and modifiers. However, the Set is Unmodifieable when returned from AutoFill and other PaymentApp. This caused chrome crash. Fixed by copying the set into a HashSet then do retainAll. After above, a second bug is triggerd from inside updateOrderSummary because it tries to dereference UI object before it is created. Simply removed the dereference because it seems redundant. BUG=718097 ========== to ========== Fix chrome crash issue when payment modifiers are enabled on android. PaymentRequestImpl tries to call Set.retainAll in order to match payment method and modifiers. However, the Set is Unmodifieable when returned from AutoFill and other PaymentApp. This caused chrome crash. Fixed by copying the set into a HashSet then do retainAll. After above, a second bug is triggerd from inside updateOrderSummary because it tries to dereference UI object before it is created. Simply removed the dereference because it seems redundant. BUG=718097 ==========
wuandy@chromium.org changed reviewers: + gogerald@chromium.org
https://codereview.chromium.org/2874883002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (left): https://codereview.chromium.org/2874883002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:860: mUI.updateOrderSummarySection(mUiShoppingCart); why is this redundant? we have to update UI if mUiShoppingCart is changed, and this change might happened after UI is shown, so you can do "if (mUI != null) mUI.update....."
https://codereview.chromium.org/2874883002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (left): https://codereview.chromium.org/2874883002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:860: mUI.updateOrderSummarySection(mUiShoppingCart); On 2017/05/11 20:11:20, gogerald1 wrote: > why is this redundant? we have to update UI if mUiShoppingCart is changed, and > this change might happened after UI is shown, so you can do "if (mUI != null) > mUI.update....." what i meant was it looked like at the end of updateWith(), mUI is updated with new shopping cart eventually. But i think even it's true, it's good to update here anyways, just in case, so made the change as you suggested.
The CQ bit was checked by wuandy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/o nits https://codereview.chromium.org/2874883002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (left): https://codereview.chromium.org/2874883002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:860: mUI.updateOrderSummarySection(mUiShoppingCart); On 2017/05/12 01:34:28, wuandy1 wrote: > On 2017/05/11 20:11:20, gogerald1 wrote: > > why is this redundant? we have to update UI if mUiShoppingCart is changed, and > > this change might happened after UI is shown, so you can do "if (mUI != null) > > mUI.update....." > > what i meant was it looked like at the end of updateWith(), mUI is updated with > new shopping cart eventually. > > But i think even it's true, it's good to update here anyways, just in case, so > made the change as you suggested. This is also called when selected payment option is changed. We should be able to remove mUI.updateOrderSummarySection from updateWith after WEB_PAYMENTS_MODIFIERS is removed https://codereview.chromium.org/2874883002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2874883002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:861: mUI.updateOrderSummarySection(mUiShoppingCart); nit: please put "if(mUI != null) mUI.updateOrderSummarySection(mUiShoppingCart);" in a single line if possible https://codereview.chromium.org/2874883002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:868: // make a copy to ensure it is modifiable. nit: // Makes*
https://codereview.chromium.org/2874883002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2874883002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:861: mUI.updateOrderSummarySection(mUiShoppingCart); On 2017/05/12 14:23:05, gogerald1 wrote: > nit: please put "if(mUI != null) > mUI.updateOrderSummarySection(mUiShoppingCart);" in a single line if possible Done. https://codereview.chromium.org/2874883002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:868: // make a copy to ensure it is modifiable. On 2017/05/12 14:23:05, gogerald1 wrote: > nit: // Makes* Done.
The CQ bit was checked by wuandy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gogerald@chromium.org Link to the patchset: https://codereview.chromium.org/2874883002/#ps80001 (title: "addressing reviewer comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494610935564360,
"parent_rev": "ad2bef1e6dd219c8b8e1fbe58b86a896550634c9", "commit_rev":
"7e09b31bb236f5fa84ee8c987c91329dc3e22509"}
Message was sent while issue was closed.
Description was changed from ========== Fix chrome crash issue when payment modifiers are enabled on android. PaymentRequestImpl tries to call Set.retainAll in order to match payment method and modifiers. However, the Set is Unmodifieable when returned from AutoFill and other PaymentApp. This caused chrome crash. Fixed by copying the set into a HashSet then do retainAll. After above, a second bug is triggerd from inside updateOrderSummary because it tries to dereference UI object before it is created. Simply removed the dereference because it seems redundant. BUG=718097 ========== to ========== Fix chrome crash issue when payment modifiers are enabled on android. PaymentRequestImpl tries to call Set.retainAll in order to match payment method and modifiers. However, the Set is Unmodifieable when returned from AutoFill and other PaymentApp. This caused chrome crash. Fixed by copying the set into a HashSet then do retainAll. After above, a second bug is triggerd from inside updateOrderSummary because it tries to dereference UI object before it is created. Simply removed the dereference because it seems redundant. BUG=718097 Review-Url: https://codereview.chromium.org/2874883002 Cr-Commit-Position: refs/heads/master@{#471385} Committed: https://chromium.googlesource.com/chromium/src/+/7e09b31bb236f5fa84ee8c987c91... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/7e09b31bb236f5fa84ee8c987c91... |
