[Mac] Check if a profile is locked before allowing a new window to be opened.

[Mac] Check if a profile is locked before allowing a new window to be opened.

BUG=331406
TEST=With the --new-profile-management flag, start Chrome. Sign in a profile, and
choose "View all people" from the avatar menu once that is complete. Close the
profile browser, then close the user manager window (you should have no windows open)
1. Clicking on the docked Chrome icon should open the user manager and 2. Command-N
(or Open New Window) form the docked Chrome icon should do the same.

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

[noms (inactive), 2014-01-08 21:20:49]

Hi Nico,

I've added some checks when opening a new browser window that we don't
shamelessly open locked profiles from the docked app icon. In the case of the
guest profile, the reason why I didn't add a similar check in -commandDispatch
is because the guest profile is forcedIncognito, which prevents Command-N from
even triggering.

[Nico, 2014-01-09 03:53:03]

Is it possible to write a test for this?

[noms (inactive), 2014-01-09 18:49:53]

Done! Not one, but three tests! :)

[noms (inactive), 2014-01-13 21:15:05]

ping! :)

[Nico, 2014-01-14 02:16:03]

Thanks for the tests. I assume you checked that some of them fail without your
code change :-)

Two minor comments below:

(sorry about the delay!)

https://codereview.chromium.org/129333002/diff/90001/chrome/browser/app_contr...
File chrome/browser/app_controller_mac_browsertest.mm (right):

https://codereview.chromium.org/129333002/diff/90001/chrome/browser/app_contr...
chrome/browser/app_controller_mac_browsertest.mm:159:
cache.SetProfileSigninRequiredAtIndex(profile_index, true);
This is an in-memory thing and doesn't need to be undone after the test, right?

https://codereview.chromium.org/129333002/diff/90001/chrome/browser/app_contr...
chrome/browser/app_controller_mac_browsertest.mm:170:
base::MessageLoop::current()->RunUntilIdle();
Huh, you're doing RunUntilIdle() twice in succession. Is more work added to the
message pump while UserManagerMac::IsShowing() is executed, or why is this
needed?

https://codereview.chromium.org/129333002/diff/90001/chrome/browser/app_contr...
chrome/browser/app_controller_mac_browsertest.mm:172: EXPECT_FALSE(result);
Maybe check this right after you assign something to `result`

(The 2 comments on this test also apply to the next)

[noms (inactive), 2014-01-14 17:59:50]

Yes, the Locked/Guest tests fail like champions pre-changes :)

https://codereview.chromium.org/129333002/diff/90001/chrome/browser/app_contr...
File chrome/browser/app_controller_mac_browsertest.mm (right):

https://codereview.chromium.org/129333002/diff/90001/chrome/browser/app_contr...
chrome/browser/app_controller_mac_browsertest.mm:159:
cache.SetProfileSigninRequiredAtIndex(profile_index, true);
Correct. The browser process gets cleaned up at the beginning of a test.
On 2014/01/14 02:16:04, Nico wrote:
> This is an in-memory thing and doesn't need to be undone after the test,
right?

https://codereview.chromium.org/129333002/diff/90001/chrome/browser/app_contr...
chrome/browser/app_controller_mac_browsertest.mm:170:
base::MessageLoop::current()->RunUntilIdle();
IsShowing() doesn't do anything, but without the extra check the test is flaky
-- I can do RunUntilIdle, it will complete, and IsShowing() won't return true.
This also happens on Windows, and my only convincing argument is that creating a
guest profile has a bunch of nested callbacks, some of which can get enqueued
after the MessageLoop enqueues its "I'm done" task. Is this complete nonsense?

On 2014/01/14 02:16:04, Nico wrote:
> Huh, you're doing RunUntilIdle() twice in succession. Is more work added to
the
> message pump while UserManagerMac::IsShowing() is executed, or why is this
> needed?

https://codereview.chromium.org/129333002/diff/90001/chrome/browser/app_contr...
chrome/browser/app_controller_mac_browsertest.mm:172: EXPECT_FALSE(result);
Oops. Bad copy paste. Done.
On 2014/01/14 02:16:04, Nico wrote:
> Maybe check this right after you assign something to `result`
> 
> (The 2 comments on this test also apply to the next)

[noms (inactive), 2014-01-18 18:17:14]

pong

[Nico, 2014-01-21 04:13:03]

Sorry about the delay. The two back-to-back "run message loop" calls don't
look right to me, so I wanted to patch this in and check what's going on,
but I haven't had the chance yet :-/


On Sat, Jan 18, 2014 at 10:17 AM, <noms@chromium.org> wrote:

> pong
>
> https://codereview.chromium.org/129333002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[noms (inactive), 2014-01-21 21:32:31]

I see. Let me know if you find anything!

[Nico, 2014-01-23 21:15:40]

https://codereview.chromium.org/129333002/diff/90001/chrome/browser/app_contr...
File chrome/browser/app_controller_mac_browsertest.mm (right):

https://codereview.chromium.org/129333002/diff/90001/chrome/browser/app_contr...
chrome/browser/app_controller_mac_browsertest.mm:170:
base::MessageLoop::current()->RunUntilIdle();
On 2014/01/14 17:59:50, Monica Dinculescu wrote:
> IsShowing() doesn't do anything, but without the extra check the test is flaky
> -- I can do RunUntilIdle, it will complete, and IsShowing() won't return true.
> This also happens on Windows, and my only convincing argument is that creating
a
> guest profile has a bunch of nested callbacks, some of which can get enqueued
> after the MessageLoop enqueues its "I'm done" task. Is this complete nonsense?

As far as I understand (after patching this in, playing around a bit, and
reading code), this is still flaky:

* app controller calls UserManagerMac::Show
* that calls profile_manager->CreateProfileAsync
* that posts a task to the file thread
* when the file thread is done, it adds more tasks to the ui thread

So what happens is:
* the first RunUntilIdle() drains all UI thread tasks
* then UI gets descheduled, filethread runs, does its stuff, posts a new UI
thread task, gets descheduled
* the second RunUntilIdle() drains the new UI thread tasks.

This happens to work if the deschedules happen in the right order, but they
won't always.

(What do you mean with "its I'm done" task?)

[noms (inactive), 2014-01-27 20:12:01]

I agree with everything you've said about what is happening. That's what I was
trying to explain, but decidedly less articulate. 

I don't think I understand why this makes it flaky, though. If the deschedules
are done in a different order, then everything
will get scheduled before the UI thread is completely done, so IsShowing() will
be true and the second RunUntilIdle() isn't needed.
I don't see how after the second RunUntilIdle() drains the thread, there would
be anything left on the MessageLoop.

This is sort of confirmed by the NewAvatarMenuButtonTest.SignOut test, which is
setup in this way and hasn't been flaky.
(I'd link to the flakiness dashboard but it seems sad today).

On 2014/01/23 21:15:40, Nico wrote:
>
https://codereview.chromium.org/129333002/diff/90001/chrome/browser/app_contr...
> File chrome/browser/app_controller_mac_browsertest.mm (right):
> 
>
https://codereview.chromium.org/129333002/diff/90001/chrome/browser/app_contr...
> chrome/browser/app_controller_mac_browsertest.mm:170:
> base::MessageLoop::current()->RunUntilIdle();
> On 2014/01/14 17:59:50, Monica Dinculescu wrote:
> > IsShowing() doesn't do anything, but without the extra check the test is
flaky
> > -- I can do RunUntilIdle, it will complete, and IsShowing() won't return
true.
> > This also happens on Windows, and my only convincing argument is that
creating
> a
> > guest profile has a bunch of nested callbacks, some of which can get
enqueued
> > after the MessageLoop enqueues its "I'm done" task. Is this complete
nonsense?
> 
> As far as I understand (after patching this in, playing around a bit, and
> reading code), this is still flaky:
> 
> * app controller calls UserManagerMac::Show
> * that calls profile_manager->CreateProfileAsync
> * that posts a task to the file thread
> * when the file thread is done, it adds more tasks to the ui thread
> 
> So what happens is:
> * the first RunUntilIdle() drains all UI thread tasks
> * then UI gets descheduled, filethread runs, does its stuff, posts a new UI
> thread task, gets descheduled
> * the second RunUntilIdle() drains the new UI thread tasks.
> 
> This happens to work if the deschedules happen in the right order, but they
> won't always.
> 
> (What do you mean with "its I'm done" task?)

[Nico, 2014-01-28 16:48:27]

On Mon, Jan 27, 2014 at 12:12 PM, <noms@chromium.org> wrote:

> I agree with everything you've said about what is happening. That's what I
> was
> trying to explain, but decidedly less articulate.
>
> I don't think I understand why this makes it flaky, though. If the
> deschedules
> are done in a different order, then everything
> will get scheduled before the UI thread is completely done, so IsShowing()
> will
> be true and the second RunUntilIdle() isn't needed.
> I don't see how after the second RunUntilIdle() drains the thread, there
> would
> be anything left on the MessageLoop.
>

I thought that deschedules aren't guaranteed to happen though – if you have
enough cores, won't all your threads happily run at all times?


> This is sort of confirmed by the NewAvatarMenuButtonTest.SignOut test,
> which is
> setup in this way and hasn't been flaky.
> (I'd link to the flakiness dashboard but it seems sad today).


https://codereview.chromium.org/44583002/diff/1660001/chrome/browser/ui/views...
I see only one call to RunUntilIdle there, not two back-to-back ones?


>
>
> On 2014/01/23 21:15:40, Nico wrote:
>
> https://codereview.chromium.org/129333002/diff/90001/
> chrome/browser/app_controller_mac_browsertest.mm
>
>> File chrome/browser/app_controller_mac_browsertest.mm (right):
>>
>
>
> https://codereview.chromium.org/129333002/diff/90001/
> chrome/browser/app_controller_mac_browsertest.mm#newcode170
>
>> chrome/browser/app_controller_mac_browsertest.mm:170:
>> base::MessageLoop::current()->RunUntilIdle();
>> On 2014/01/14 17:59:50, Monica Dinculescu wrote:
>> > IsShowing() doesn't do anything, but without the extra check the test is
>>
> flaky
>
>> > -- I can do RunUntilIdle, it will complete, and IsShowing() won't return
>>
> true.
>
>> > This also happens on Windows, and my only convincing argument is that
>>
> creating
>
>> a
>> > guest profile has a bunch of nested callbacks, some of which can get
>>
> enqueued
>
>> > after the MessageLoop enqueues its "I'm done" task. Is this complete
>>
> nonsense?
>
>  As far as I understand (after patching this in, playing around a bit, and
>> reading code), this is still flaky:
>>
>
>  * app controller calls UserManagerMac::Show
>> * that calls profile_manager->CreateProfileAsync
>> * that posts a task to the file thread
>> * when the file thread is done, it adds more tasks to the ui thread
>>
>
>  So what happens is:
>> * the first RunUntilIdle() drains all UI thread tasks
>> * then UI gets descheduled, filethread runs, does its stuff, posts a new
>> UI
>> thread task, gets descheduled
>> * the second RunUntilIdle() drains the new UI thread tasks.
>>
>
>  This happens to work if the deschedules happen in the right order, but
>> they
>> won't always.
>>
>
>  (What do you mean with "its I'm done" task?)
>>
>
>
>
> https://codereview.chromium.org/129333002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[noms (inactive), 2014-01-29 18:05:09]

I fixed the test!

[Nico, 2014-01-29 18:37:48]

Nice!

I think I saw similar code elsewhere, maybe that could be factored into a helper
later. One last comment:

https://codereview.chromium.org/129333002/diff/360001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac_browsertest.mm (right):

https://codereview.chromium.org/129333002/diff/360001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:145:
base::MessageLoop::current()->RunUntilIdle();
Since you have a QuitClosure now, do you want Run() instead of RunUntilIdle()?

(Also, I think MessageLoop::blah is deprecated and we prefer `base::RunLoop
run_loop; run_loop.Run(UntilIdle)?()` these days.)

[noms (inactive), 2014-01-29 19:16:10]

https://codereview.chromium.org/129333002/diff/360001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac_browsertest.mm (right):

https://codereview.chromium.org/129333002/diff/360001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:145:
base::MessageLoop::current()->RunUntilIdle();
On 2014/01/29 18:37:48, Nico wrote:
> Since you have a QuitClosure now, do you want Run() instead of RunUntilIdle()?
> 
> (Also, I think MessageLoop::blah is deprecated and we prefer `base::RunLoop
> run_loop; run_loop.Run(UntilIdle)?()` these days.)

Done.

[Nico, 2014-01-29 19:17:11]

lgtm!

[noms (inactive), 2014-01-29 19:18:17]

EEEEEE! Thanks! To the CQ!

[commit-bot: I haz the power, 2014-01-29 19:20:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/129333002/380001

[commit-bot: I haz the power, 2014-01-30 03:03:43]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 03:09:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/129333002/380001

[commit-bot: I haz the power, 2014-01-30 08:10:52]

Change committed as 247891