Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2862933002:
[autofill] Avoid duplicate instances of the SaveCardBubble. (Closed)
|
Created:
3 years, 7 months ago by Roger McFarlane (Chromium) Modified:
3 years, 7 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[autofill] Avoid duplicate instances of the SaveCardBubble.
autofill::SaveCardBubbleControllerImpl::ShowBubble() expects
(via DCHECK) to only be called when the save card bubble is
not already visible. This constraint is violated if the user
clicks multiple times on a submit button.
If the underlying page goes away, the last SaveCardBubbleView
created by the controller will be automatically cleaned up,
but any others are left visible on the screen... holding a
refence to a possibly-deleted controller.
This CL early exits the ShowBubbleFor*** and ReshowBubble logic
if the bubble is already visible.
BUG=708819
Review-Url: https://codereview.chromium.org/2862933002
Cr-Commit-Position: refs/heads/master@{#469768}
Committed: https://chromium.googlesource.com/chromium/src/+/8b457840e3512ef8c2af8d966a3edce5c653a835
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add unittests #
Total comments: 2
Patch Set 3 : fix typo #
Messages
Total messages: 29 (15 generated)
rogerm@chromium.org changed reviewers: + csashi@google.com, jsaul@google.com, mathp@chromium.org
PTAL? jsaul/cshashi: you've been playing in these files recently mathp: for OWNERS
The CQ bit was checked by rogerm@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...
lgtm Is it possible to write a unit test for this?
https://codereview.chromium.org/2862933002/diff/1/chrome/browser/ui/autofill/... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right): https://codereview.chromium.org/2862933002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:47: save_card_bubble_view_->Hide(); Why did this line let the bug exist? If the controller was cleaned up, shouldn't the popup have gone away? https://codereview.chromium.org/2862933002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:114: if (save_card_bubble_view_) If the icon in the Omnibox is clicked which hides the bubble, then reclicked to reshow the bubble, what's the value of save_card_bubble_view_? Is it truly null? Because I'm assuming if save_card_bubble_view_->Hide() is called, that doesn't mean save_card_bubble_view_ no longer exists, because I'd expect save_card_bubble_view->Show() to bring it back.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rogerm@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...
Added unittests. Sashi, as you're looking at increasing card save rates, this bug points out a problem with the save bubble... The user's interaction with the bubble is racing against the underlying page navigating away as a result of the submit! We need to separate state that's tied to the page from the state required to perform the card save, so that the view can exist independently of the page, maybe tied to the tab instead? https://codereview.chromium.org/2862933002/diff/1/chrome/browser/ui/autofill/... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right): https://codereview.chromium.org/2862933002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:47: save_card_bubble_view_->Hide(); On 2017/05/04 20:51:12, Jared Saul wrote: > Why did this line let the bug exist? If the controller was cleaned up, > shouldn't the popup have gone away? the problem is not in the cleanup, it's in the Show. The Show paths were not checking that if a bubble was already shown. Instead, they just overwrote save_card_bubble_view_ to point to a new bubble instance, effectively leaking the previous bubble. https://codereview.chromium.org/2862933002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:114: if (save_card_bubble_view_) On 2017/05/04 20:51:12, Jared Saul wrote: > If the icon in the Omnibox is clicked which hides the bubble, then reclicked to > reshow the bubble, what's the value of save_card_bubble_view_? Is it truly > null? > > Because I'm assuming if save_card_bubble_view_->Hide() is called, that doesn't > mean save_card_bubble_view_ no longer exists, because I'd expect > save_card_bubble_view->Show() to bring it back. The bubble actually manages it's own lifetime. We're just managing the controllers references to it and its references to the controller. SaveCardBubbleControllerImpl::HideBubble() asks the view to Hide() itself then discards it's pointer to the view (see line 108). SaveBubbleCardViews::Hide() both hides the view and discards its pointer back to the controller. Showing the view again doesn't reuse the view, it creates a new one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm >> Sashi, as you're looking at increasing card save rates, this bug points out a problem with the save bubble... The user's interaction with the bubble is racing against the underlying page navigating away as a result of the submit! IIRC, that should be handled by line 34 in save_card_bubble_controller_impl.cc, which persists the bubble for at least 5 seconds regardless of any page navigation. https://codereview.chromium.org/2862933002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc (right): https://codereview.chromium.org/2862933002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:510: TEST_F(SaveCardBubbleControllerImplTest, OnlyOneActiveBubble_UploadTheLocal) { s/UploadTheLocal/UploadThenLocal
re: kSurviveNavigationSeconds = 5 I don't think the application of this works. Navigation happens, the controller gets notified skips closing the bubble if the elapsed time is too short... ... but then the controller gets deleted and is forced to close the bubble... irrespective of how much time has elapsed. https://codereview.chromium.org/2862933002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc (right): https://codereview.chromium.org/2862933002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:510: TEST_F(SaveCardBubbleControllerImplTest, OnlyOneActiveBubble_UploadTheLocal) { On 2017/05/05 17:47:16, Jared Saul wrote: > s/UploadTheLocal/UploadThenLocal Done.
The CQ bit was checked by rogerm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csashi@google.com, jsaul@google.com Link to the patchset: https://codereview.chromium.org/2862933002/#ps40001 (title: "fix typo")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
On 2017/05/05 18:18:01, Roger McFarlane (Chromium) wrote: > re: kSurviveNavigationSeconds = 5 > > I don't think the application of this works. > > Navigation happens, the controller gets notified skips closing the bubble if the > elapsed time is too short... > > ... but then the controller gets deleted and is forced to close the bubble... > irrespective of how much time has elapsed. On our test site, if you complete a flow and the bubble appears, you can continue to navigate wherever you want for 5 seconds and it will stick around and still work correctly. Doesn't that imply the application works? (Or are you referring to a different scenario?)
lgtm
On 2017/05/05 18:49:06, Jared Saul wrote: > On 2017/05/05 18:18:01, Roger McFarlane (Chromium) wrote: > > re: kSurviveNavigationSeconds = 5 > > > > I don't think the application of this works. > > > > Navigation happens, the controller gets notified skips closing the bubble if > the > > elapsed time is too short... > > > > ... but then the controller gets deleted and is forced to close the bubble... > > irrespective of how much time has elapsed. > > On our test site, if you complete a flow and the bubble appears, you can > continue to navigate wherever you want for 5 seconds and it will stick around > and still work correctly. Doesn't that imply the application works? (Or are > you referring to a different scenario?) Try the example in the referenced bug.
On 2017/05/05 19:46:41, Roger McFarlane (Chromium) wrote: > On 2017/05/05 18:49:06, Jared Saul wrote: > > On 2017/05/05 18:18:01, Roger McFarlane (Chromium) wrote: > > > re: kSurviveNavigationSeconds = 5 > > > > > > I don't think the application of this works. > > > > > > Navigation happens, the controller gets notified skips closing the bubble if > > the > > > elapsed time is too short... > > > > > > ... but then the controller gets deleted and is forced to close the > bubble... > > > irrespective of how much time has elapsed. > > > > On our test site, if you complete a flow and the bubble appears, you can > > continue to navigate wherever you want for 5 seconds and it will stick around > > and still work correctly. Doesn't that imply the application works? (Or are > > you referring to a different scenario?) > > Try the example in the referenced bug. Note: I don't have any particular reason to think this is a huge problem in the wild. I'm just raising it as a possible failure mode. This test case does seem like a peculiar thing for a web-site to do, but that doesn't meant that there aren't sites doing something equivalent. In this specific case of the bugs repro, the window is going away due to external forces, not the user's direct navigation.
The CQ bit was checked by rogerm@chromium.org
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": 40001, "attempt_start_ts": 1494015370749390,
"parent_rev": "9b0ef8da376582de0ba4d3aaa223bd78c3ab2f19", "commit_rev":
"8b457840e3512ef8c2af8d966a3edce5c653a835"}
Message was sent while issue was closed.
Description was changed from ========== [autofill] Avoid duplicate instances of the SaveCardBubble. autofill::SaveCardBubbleControllerImpl::ShowBubble() expects (via DCHECK) to only be called when the save card bubble is not already visible. This constraint is violated if the user clicks multiple times on a submit button. If the underlying page goes away, the last SaveCardBubbleView created by the controller will be automatically cleaned up, but any others are left visible on the screen... holding a refence to a possibly-deleted controller. This CL early exits the ShowBubbleFor*** and ReshowBubble logic if the bubble is already visible. BUG=708819 ========== to ========== [autofill] Avoid duplicate instances of the SaveCardBubble. autofill::SaveCardBubbleControllerImpl::ShowBubble() expects (via DCHECK) to only be called when the save card bubble is not already visible. This constraint is violated if the user clicks multiple times on a submit button. If the underlying page goes away, the last SaveCardBubbleView created by the controller will be automatically cleaned up, but any others are left visible on the screen... holding a refence to a possibly-deleted controller. This CL early exits the ShowBubbleFor*** and ReshowBubble logic if the bubble is already visible. BUG=708819 Review-Url: https://codereview.chromium.org/2862933002 Cr-Commit-Position: refs/heads/master@{#469768} Committed: https://chromium.googlesource.com/chromium/src/+/8b457840e3512ef8c2af8d966a3e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8b457840e3512ef8c2af8d966a3e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
