It is possible to sometimes have a null PermissionBubbleManager.

It is possible to sometimes have a null PermissionBubbleManager.

BUG=457091
Committed: https://crrev.com/2bb0feb026e70d97ee92cb72fe5c93e43eaa5c32
Cr-Commit-Position: refs/heads/master@{#321812}

[mlamouri (slow - plz ping), 2015-03-20 14:41:38]

mlamouri@chromium.org changed reviewers:
+ bauerb@chromium.org, felt@chromium.org

[mlamouri (slow - plz ping), 2015-03-20 14:41:38]

mlamouri@chromium.org changed required reviewers:
+ bauerb@chromium.org

[felt, 2015-03-20 14:44:17]

https://codereview.chromium.org/1023213002/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/1023213002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/permission_context_base.cc:129: if
(!bubble_manager)
i think style is to have either a DCHECK or an if, but not both

[mlamouri (slow - plz ping), 2015-03-20 14:51:49]

PTAL

https://codereview.chromium.org/1023213002/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/1023213002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/permission_context_base.cc:129: if
(!bubble_manager)
On 2015/03/20 at 14:44:17, felt wrote:
> i think style is to have either a DCHECK or an if, but not both

The if is really there just to prevent the crash. We shouldn't be running into
this. The intent is to fix the sympton, if possible. The cause is clearly
somewhere else. Will add a comment.

[felt, 2015-03-20 15:00:42]

https://codereview.chromium.org/1023213002/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/1023213002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/permission_context_base.cc:129: if
(!bubble_manager)
On 2015/03/20 14:51:49, Mounir Lamouri wrote:
> On 2015/03/20 at 14:44:17, felt wrote:
> > i think style is to have either a DCHECK or an if, but not both
> 
> The if is really there just to prevent the crash. We shouldn't be running into
> this. The intent is to fix the sympton, if possible. The cause is clearly
> somewhere else. Will add a comment.

http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

I still think you ought to remove the DCHECK, at least temporarily. Once the
underlying issue is fixed the DCHECK can be added back again.

[mlamouri (slow - plz ping), 2015-03-20 15:10:56]

I removed it. Though, for the record, I think that the DCHECK would be very
useful if we hit it. Here, we are making us entirely blind of the issue.

[felt, 2015-03-20 15:14:04]

The CQ bit was checked by felt@chromium.org

[felt, 2015-03-20 15:14:05]

lgtm

[commit-bot: I haz the power, 2015-03-20 15:14:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1023213002/40001

[commit-bot: I haz the power, 2015-03-20 15:14:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-20 15:14:33]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[felt, 2015-03-20 15:20:06]

On 2015/03/20 15:14:33, I haz the power (commit-bot) wrote:
> All required reviewers (with asterisk prefixes) have not yet approved this CL.
> 
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

oh damn this is in content settings. can you add markus since bernhard is ooo?

[mlamouri (slow - plz ping), 2015-03-20 15:21:55]

mlamouri@chromium.org changed reviewers:
+ markusheintz@chromium.org
- bauerb@chromium.org

[mlamouri (slow - plz ping), 2015-03-20 15:21:55]

mlamouri@chromium.org changed required reviewers:
- bauerb@chromium.org

[mlamouri (slow - plz ping), 2015-03-23 11:07:27]

mlamouri@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2015-03-23 17:39:32]

LGTM to unblock you, but I agree that the right thing to do would be to fix the
underlying issue. If you want to just stabilize M42, you could land this CL,
merge it to M42 and then revert it on trunk afterwards, so you'll continue to
get crash reports and can investigate.

[mlamouri (slow - plz ping), 2015-03-23 17:40:10]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-03-23 17:40:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1023213002/40001

[felt, 2015-03-23 18:03:45]

On 2015/03/23 17:39:32, Bernhard Bauer wrote:
> LGTM to unblock you, but I agree that the right thing to do would be to fix
the
> underlying issue. If you want to just stabilize M42, you could land this CL,
> merge it to M42 and then revert it on trunk afterwards, so you'll continue to
> get crash reports and can investigate.

+1

[commit-bot: I haz the power, 2015-03-23 18:24:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-23 18:25:16]

Patchset 3 (id:??) landed as
https://crrev.com/2bb0feb026e70d97ee92cb72fe5c93e43eaa5c32
Cr-Commit-Position: refs/heads/master@{#321812}