New issue
Advanced search Search tips

Issue 611827 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

Gather metrics about user engagement with the Settings app when setting default browser.

Project Member Reported by grt@chromium.org, May 13 2016

Issue description

It would be nice to know where the pain points are.
 

Comment 2 by grt@chromium.org, Jun 13 2016

Labels: Merge-Request-52
Status: Fixed (was: Started)
Shipped on dev with no issues. Requesting merge to 52 so we can get numbers from a bigger audience sooner. Thanks.

Comment 3 by tin...@google.com, Jun 13 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)

Comment 4 by grt@chromium.org, Jun 13 2016

Thanks. I checked again and there are two shutdown crashes. A fix is out for review. I'll hold off on the merge until the fix lands.
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c76bfb795b85473a03397b185dce94f7c263de3

commit 2c76bfb795b85473a03397b185dce94f7c263de3
Author: grt <grt@chromium.org>
Date: Mon Jun 13 20:36:37 2016

Fix shutdown crash in settings app monitor.

Don't assume that the caller's task runner will outlive the monitor.

BUG= 611827 
R=pmonette@chromium.org

Review-Url: https://codereview.chromium.org/2065673002
Cr-Commit-Position: refs/heads/master@{#399528}

[modify] https://crrev.com/2c76bfb795b85473a03397b185dce94f7c263de3/chrome/browser/win/settings_app_monitor.cc

Labels: Needs-Feedback
@grt, please let us know if this can this be tested manually so that we can verify it on Canary before it gets merged in to M52 branch ?
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c76bfb795b85473a03397b185dce94f7c263de3

commit 2c76bfb795b85473a03397b185dce94f7c263de3
Author: grt <grt@chromium.org>
Date: Mon Jun 13 20:36:37 2016

Fix shutdown crash in settings app monitor.

Don't assume that the caller's task runner will outlive the monitor.

BUG= 611827 
R=pmonette@chromium.org

Review-Url: https://codereview.chromium.org/2065673002
Cr-Commit-Position: refs/heads/master@{#399528}

[modify] https://crrev.com/2c76bfb795b85473a03397b185dce94f7c263de3/chrome/browser/win/settings_app_monitor.cc

Comment 8 by grt@chromium.org, Jun 15 2016

Since Canary does not prompt users to become default, this cannot be tested on canary. We can test it on dev once we ship a post-r399528 build to dev. It's likely difficult to reproduce the crash. I imagine one must do something like click the default button on either the default browser infobar or on chrome://settings and then immediately quit the browser. Alternatively, we can wait for r399528 to hit dev and look for crashes. I am fairly confident that the fix is safe and addresses the issue.
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 16 2016

Project Member

Comment 10 by bugdroid1@chromium.org, Jun 16 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af9f7fdfda79a235f62a3bc6045d3771f2408a4b

commit af9f7fdfda79a235f62a3bc6045d3771f2408a4b
Author: Greg Thompson <grt@chromium.org>
Date: Thu Jun 16 19:27:48 2016

Fix shutdown crash in settings app monitor.

Don't assume that the caller's task runner will outlive the monitor.

BUG= 611827 
R=pmonette@chromium.org

Review-Url: https://codereview.chromium.org/2065673002
Cr-Commit-Position: refs/heads/master@{#399528}
(cherry picked from commit 2c76bfb795b85473a03397b185dce94f7c263de3)
TBR=grt@chromium.org

Review URL: https://codereview.chromium.org/2070933002 .

Cr-Commit-Position: refs/branch-heads/2743@{#370}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/af9f7fdfda79a235f62a3bc6045d3771f2408a4b/chrome/browser/win/settings_app_monitor.cc

Sign in to add a comment