Prevent new Browser windows from violating the IncognitoModeAvailability policy.

Prevent new Browser windows from violating the IncognitoModeAvailability policy.

This policy may disable incognito mode, or set it to enforced (i.e. disable
non-incognito windows). This change introduces a CHECK that prevents any code
paths from opening a disabled window type; such code paths should still be
fixed, but this CHECK prevents working around the policy.

BUG=244246
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204011

[Joao da Silva, 2013-05-29 11:26:18]

Please review:

@mnissler: prefs/
@pkasting: ui/

So this is a big hammer but it's better to crash for 2 reasons: we'll learn that
there is a bug, and we prevent escape hatches from the enforced policy in
managed environments. The known escape hatches must still be fixed; this is
meant as a catch-all for those that we aren't aware of.

[Mattias Nissler (ping if slow), 2013-05-29 11:33:08]

LGTM with a nit. What's the plan for the known offenders?

https://codereview.chromium.org/15670011/diff/5001/chrome/browser/prefs/incog...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/15670011/diff/5001/chrome/browser/prefs/incog...
chrome/browser/prefs/incognito_mode_prefs.cc:80: // it should be fixed.
Shouldn't this comment go with the CHECK()?

[Joao da Silva, 2013-05-29 11:40:40]

The known offender listed in the bug will be fixed in separate CLs; unknown
offenders will be shot :-)

https://codereview.chromium.org/15670011/diff/5001/chrome/browser/prefs/incog...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/15670011/diff/5001/chrome/browser/prefs/incog...
chrome/browser/prefs/incognito_mode_prefs.cc:80: // it should be fixed.
On 2013/05/29 11:33:08, Mattias Nissler wrote:
> Shouldn't this comment go with the CHECK()?

Done.

[Peter Kasting, 2013-05-29 18:18:48]

CHECK means this can't happen.  It seems like right now, it can happen.  Since
this is not a security bug but rather a policy-enforcement bug, I'm not
comfortable with CHECK for this.  We should fix all known problem codepaths
before considering something like CHECK.

At least until then, we should instead do something like "if (problem case)
return false" or something to just silently not open the window.  We could also
log and/or collect UMA metrics to try and still capture data on whether this
happens.  If we fix the problem cases and we see that this can still happen,
then we insert the CHECK.

[Joao da Silva, 2013-05-30 00:22:15]

On 2013/05/29 18:18:48, Peter Kasting wrote:
> CHECK means this can't happen.  It seems like right now, it can happen.  Since
> this is not a security bug but rather a policy-enforcement bug, I'm not
> comfortable with CHECK for this.  We should fix all known problem codepaths
> before considering something like CHECK.
>

Agreed; there are 3 known paths, and they're fixed by

https://codereview.chromium.org/16072010/
https://codereview.chromium.org/15859011/
https://codereview.chromium.org/16072014/
 
> At least until then, we should instead do something like "if (problem case)
> return false" or something to just silently not open the window.  We could
also
> log and/or collect UMA metrics to try and still capture data on whether this
> happens.  If we fix the problem cases and we see that this can still happen,
> then we insert the CHECK.

Most call sites that open a new browser window use a "valid" profile, and
introducing NULL checks all over the place doesn't seem ideal.

I believe there's no way to open a non-incognito window when that's not allowed
after the fixes listed above go in. A CHECK will send us data if something went
wrong, with a stack trace indicating the culprit. The CHECK has the additional
benefit that, until such potential bugs are fixed, then the users don't really
have a workaround for the policy. If new code breaks the policy again then the
CHECK may also bring this to the developer's attention, which a silent failure
won't do.

So I do think the CHECK is the right thing to do in this case, but let me know
if you're not convinced :-)

[Peter Kasting, 2013-05-30 00:27:11]

On 2013/05/30 00:22:15, Joao da Silva wrote:
> On 2013/05/29 18:18:48, Peter Kasting wrote:
> > CHECK means this can't happen.  It seems like right now, it can happen. 
Since
> > this is not a security bug but rather a policy-enforcement bug, I'm not
> > comfortable with CHECK for this.  We should fix all known problem codepaths
> > before considering something like CHECK.
> >
> 
> Agreed; there are 3 known paths, and they're fixed by
> 
> https://codereview.chromium.org/16072010/
> https://codereview.chromium.org/15859011/
> https://codereview.chromium.org/16072014/

If you land these first, I'm OK with CHECK, assuming you monitor it and
downgrade to DCHECK once we have had no instances of hitting the check for a
sufficiently long time that we're confident in it.

[Peter Kasting, 2013-05-30 00:27:33]

And LGTM since you probably need that to do that.

[Joao da Silva, 2013-06-04 12:05:29]

On 2013/05/30 00:27:11, Peter Kasting wrote:
> On 2013/05/30 00:22:15, Joao da Silva wrote:
> > On 2013/05/29 18:18:48, Peter Kasting wrote:
> > > CHECK means this can't happen.  It seems like right now, it can happen. 
> Since
> > > this is not a security bug but rather a policy-enforcement bug, I'm not
> > > comfortable with CHECK for this.  We should fix all known problem
codepaths
> > > before considering something like CHECK.
> > >
> > 
> > Agreed; there are 3 known paths, and they're fixed by
> > 
> > https://codereview.chromium.org/16072010/
> > https://codereview.chromium.org/15859011/
> > https://codereview.chromium.org/16072014/
> 
> If you land these first, I'm OK with CHECK, assuming you monitor it and
> downgrade to DCHECK once we have had no instances of hitting the check for a
> sufficiently long time that we're confident in it.

Cool, I'll do that. The other CLs have landed so I'm landing this as well.

[commit-bot: I haz the power, 2013-06-04 12:05:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/15670011/31001

[commit-bot: I haz the power, 2013-06-04 19:19:12]

Change committed as 204011