Issue metadata
Sign in to add a comment
|
Regression: Browser crash is seen on clicking on reset button for second time in chrome://md-settings |
||||||||||||||||||||||
Issue descriptionVersion: 54.0.2840.0 Dev OS: Ubuntu 14.04, Windows What steps will reproduce the problem? 1. Sign into chrome>>Navigate to chrome://md-settings/advanced page>>Click on Reset and Uncheck the Help check box present at the bottom left corner of the reset overlay. 2. Click on reset button and immediately click cancel(Please refer video)>> Now again click on reset button in reset overlay and Observe browser crash. Expected: Browser should not crash on clicking on reset button for the second time. Actual: Instead browser crash is seen. Crash id: f4b8031100000000 This is Regression issue broken in M-48.Unable to do tool bisect as issue is seen only after signing into chrome which can't be done in chromium builds. Manual bisect info: Good build:48.0.2560.0 Bad build:48.0.2561.0 CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/48.0.2560.0..48.0.2561.0?pretty=fuller&n=10000 Suspecting https://codereview.chromium.org/1418803004 from change-log. @dpapad- Please help in reassigning if it is not related to your change. Attaching screen-cast for reference.
,
Sep 14 2016
,
Sep 14 2016
There are two problems here. 1) The "cancel" button should not be enabled when clearing is in progress. 2) Because the user can close the dialog anyway, using the 'x' button or 'esc' key, the dialog should be smart enough to know that clearing is in progress when it is re-shown and therefore have the "reset" button still disabled. The above matches exactly the behavior of this dialog in the old Options (see attached video comparing the two), and addresses the crash issue. I implemented this at https://codereview.chromium.org/2339853003.
,
Sep 15 2016
@bettes: Could you verify if the behavior seen in the screencast at comment#3 is OK? This matches the old Options behavior. Specifically the question is: What should be done after the user clicks "Reset" in the reset profile dialog? The approaches mentioned so far are 1) Disable "reset" and "cancel" button, leave the dialog open. User can still close the dialog using 'esc' key or the 'x' button. In case the user brings the dialog up again, "reset" and "cancel" buttons remain disabled until the operation finishes, which closes the dialog. 2) (Mentioned at https://codereview.chromium.org/2339853003#msg12 by dbeam@): Immediately close the dialog. Once the operation finishes show a toast informing the user that it finished. It is unclear what happens in this approach when the user brings up the dialog again, while the clearing operation is in progress.
,
Sep 15 2016
,
Sep 15 2016
I asked Demetrios to not disable the "Cancel" button while a dialog is working because the user can still close the dialog via Escape or the (X) button, so why not leave "Cancel" enabled? But now I understand why it'd be confusing; the main point of contention here is that we show a dialog like this: ____________________________ | | | X | | Do something. | | | | [ Cancel ][ Do it ] | |__________________________| before they tap "Do it", it's fairly clear that "Cancel" means "Don't do it". but /while/ an action is being performed, it's ambiguous. do users think clicking "Cancel" stops doing things? undoes or reverts anything that might've already been done? or just closes the dialog? protip: right now it just means "closes the dialog", as far as I know. Closing the dialog immediately when starting the "Do it" action and showing a toast when done is great (but requires more work). Alternatively, we could just... not use "Cancel" or change the text after the action is started. this seems less great but less work.
,
Sep 16 2016
Although a reasonable concern, I don't think the expectation of Cancel's function changes for the majority of users who click 'Do it.' I think we should keep the behavior that we do today which is #1 on comment 4: 1) Disable "reset" and "cancel" button, leave the dialog open. User can still close the dialog using 'esc' key or the 'x' button. In case the user brings the dialog up again, "reset" and "cancel" buttons remain disabled until the operation finishes, which closes the dialog.
,
Sep 16 2016
thanks for the clarification, bettes@
,
Sep 19 2016
fwiw: I'll sign off on the code that makes this change, but I don't think it's clear what happens if you click (X) or press Esc while something is happening. i bet half of users are trying to / expect that the action is cancelled or reverted.
,
Sep 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/882a6dc0c0299130c7dc91bce0d2ba50c63cbec3 commit 882a6dc0c0299130c7dc91bce0d2ba50c63cbec3 Author: dpapad <dpapad@chromium.org> Date: Tue Sep 20 03:04:20 2016 MD Settings: Fix Reset profile dialog scenario that causes a crash. 1) Disable the "Cancel" button when clearing is in progress. User can still close though using 'x' or 'esc' key. 2) Stop destroying the dialog once it is closed. This allows the dialog to be re-shown with the progress spinner visible and action buttons disabled, if clearing is still in progress when it is re-shown. This matches exactly the behavior of this dialog in the old Options. BUG= 641291 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2339853003 Cr-Commit-Position: refs/heads/master@{#419664} [modify] https://crrev.com/882a6dc0c0299130c7dc91bce0d2ba50c63cbec3/chrome/browser/resources/settings/reset_page/powerwash_dialog.js [modify] https://crrev.com/882a6dc0c0299130c7dc91bce0d2ba50c63cbec3/chrome/browser/resources/settings/reset_page/reset_page.html [modify] https://crrev.com/882a6dc0c0299130c7dc91bce0d2ba50c63cbec3/chrome/browser/resources/settings/reset_page/reset_page.js [modify] https://crrev.com/882a6dc0c0299130c7dc91bce0d2ba50c63cbec3/chrome/browser/resources/settings/reset_page/reset_profile_dialog.html [modify] https://crrev.com/882a6dc0c0299130c7dc91bce0d2ba50c63cbec3/chrome/browser/resources/settings/reset_page/reset_profile_dialog.js [modify] https://crrev.com/882a6dc0c0299130c7dc91bce0d2ba50c63cbec3/chrome/browser/resources/settings/settings_resources.grd [modify] https://crrev.com/882a6dc0c0299130c7dc91bce0d2ba50c63cbec3/chrome/test/data/webui/settings/reset_page_test.js
,
Sep 20 2016
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by brajkumar@chromium.org
, Aug 26 2016