Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2468873003:
Don't call CloseBubble multiple times in ExtensionInstalledBubbleView (Closed)
|
Created:
4 years, 1 month ago by Evan Stade Modified:
4 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't call CloseBubble multiple times in ExtensionInstalledBubbleView
fixes crashes on clicking "manage extensions" or "sign in to chrome"
BUG=661096
Committed: https://crrev.com/5d800ae3299fcbda1e0d9111aa83ae707c2c2fb6
Cr-Commit-Position: refs/heads/master@{#429912}
Patch Set 1 #
Total comments: 2
Patch Set 2 : check #
Total comments: 2
Messages
Total messages: 34 (15 generated)
The CQ bit was checked by estade@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.
estade@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Can we add a test for this? https://codereview.chromium.org/2468873003/diff/1/chrome/browser/ui/views/ext... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/2468873003/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:264: // Showing the sign-in UI will cause the bubble to close. Does this happen synchronously? If so, can we CHECK(GetWidget()->IsClosed())?
https://codereview.chromium.org/2468873003/diff/1/chrome/browser/ui/views/ext... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/2468873003/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:264: // Showing the sign-in UI will cause the bubble to close. On 2016/11/02 18:12:23, Devlin wrote: > Does this happen synchronously? If so, can we CHECK(GetWidget()->IsClosed())? Done.
The CQ bit was checked by estade@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 Fingers crossed that this is always synchronous... As an alternative, I would be fine with making ExtensionInstalledBubbleView::CloseBubble() safe to call multiple times, I'd be fine with that, too. Having a clearer path is always preferable, but chasing crashes can get exhausting.
The CQ bit was unchecked by estade@chromium.org
The CQ bit was checked by estade@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
estade@chromium.org changed reviewers: + pkasting@chromium.org
+pkasting for owners
LGTM https://codereview.chromium.org/2468873003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/2468873003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:265: CHECK(GetWidget()->IsClosed()); Nit: Why CHECK and not DCHECK? Generally we should prefer DCHECK, unless this is a security vulnerability or a temporary debugging measure; see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... .
https://codereview.chromium.org/2468873003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/2468873003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:265: CHECK(GetWidget()->IsClosed()); On 2016/11/03 00:56:39, Peter Kasting wrote: > Nit: Why CHECK and not DCHECK? Generally we should prefer DCHECK, unless this > is a security vulnerability or a temporary debugging measure; see > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > . Devlin has expressed a preference for CHECK. I'm happy to change it to DCHECK.
On 2016/11/03 15:55:29, Evan Stade wrote: > https://codereview.chromium.org/2468873003/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc > (right): > > https://codereview.chromium.org/2468873003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:265: > CHECK(GetWidget()->IsClosed()); > On 2016/11/03 00:56:39, Peter Kasting wrote: > > Nit: Why CHECK and not DCHECK? Generally we should prefer DCHECK, unless this > > is a security vulnerability or a temporary debugging measure; see > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > . > > Devlin has expressed a preference for CHECK. I'm happy to change it to DCHECK. The reason for that preference in this particular case is because there's a lot of ambiguity around the ordering that's happening here, and it can lead to a broken state for users if we continue on. Note that this CL is to fix a crash discovered from a related CHECK which wouldn't have been caught by a DCHECK, and would lead to harder-to-debug edge cases. I generally prefer DCHECKs too, but since so much of this seems to be wavering on "I'm pretty sure this is how it goes", I'd prefer we have more certainty in this case. As a compromise, I'd also be fine with e.g. adding a TODO to relax the CHECK to a DCHECK in a milestone once we know that it isn't crashing.
On 2016/11/03 16:00:40, Devlin (slow) wrote: > On 2016/11/03 15:55:29, Evan Stade wrote: > > > https://codereview.chromium.org/2468873003/diff/20001/chrome/browser/ui/views... > > File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc > > (right): > > > > > https://codereview.chromium.org/2468873003/diff/20001/chrome/browser/ui/views... > > chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:265: > > CHECK(GetWidget()->IsClosed()); > > On 2016/11/03 00:56:39, Peter Kasting wrote: > > > Nit: Why CHECK and not DCHECK? Generally we should prefer DCHECK, unless > this > > > is a security vulnerability or a temporary debugging measure; see > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > . > > > > Devlin has expressed a preference for CHECK. I'm happy to change it to DCHECK. > > The reason for that preference in this particular case is because there's a lot > of ambiguity around the ordering that's happening here, and it can lead to a > broken state for users if we continue on. Note that this CL is to fix a crash > discovered from a related CHECK which wouldn't have been caught by a DCHECK, and > would lead to harder-to-debug edge cases. I generally prefer DCHECKs too, but > since so much of this seems to be wavering on "I'm pretty sure this is how it > goes", I'd prefer we have more certainty in this case. > > As a compromise, I'd also be fine with e.g. adding a TODO to relax the CHECK to > a DCHECK in a milestone once we know that it isn't crashing. or once that test coverage gets added :) :(
Peter, wdyt of Devlin's proposal?
CHECKs are fine for temporarily sniffing out whether assumptions are being violated, that's explicitly OK in the style guide. Will there be a bug that tracks relaxing this? TODOs are easily forgotten/lost. I like the idea of adding tests that exercise this.
filed crbug.com/662269
The CQ bit was checked by estade@chromium.org
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Don't call CloseBubble multiple times in ExtensionInstalledBubbleView fixes crashes on clicking "manage extensions" or "sign in to chrome" BUG=661096 ========== to ========== Don't call CloseBubble multiple times in ExtensionInstalledBubbleView fixes crashes on clicking "manage extensions" or "sign in to chrome" BUG=661096 Committed: https://crrev.com/5d800ae3299fcbda1e0d9111aa83ae707c2c2fb6 Cr-Commit-Position: refs/heads/master@{#429912} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5d800ae3299fcbda1e0d9111aa83ae707c2c2fb6 Cr-Commit-Position: refs/heads/master@{#429912} |
