Avoid a crash in ProfileManager::GetLastUsedProfileAllowedByPolicy()

Avoid a crash in ProfileManager::GetLastUsedProfileAllowedByPolicy()

BUG=637333
Committed: https://crrev.com/9f27972d0fbb00d451c9ae3f741d1e22ddd25891
Cr-Commit-Position: refs/heads/master@{#413490}

[afakhry, 2016-08-12 18:40:27]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 18:40:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2016-08-12 18:44:45]

Patchset #1 (id:1) has been deleted

[afakhry, 2016-08-12 18:46:03]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 18:46:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2016-08-12 19:12:07]

afakhry@chromium.org changed reviewers:
+ anthonyvd@chromium.org, tommi@chromium.org

[afakhry, 2016-08-12 19:12:08]

tommi@chromium.org: Please review changes in 
tab_desktop_media_list.cc

anthonyvd@chromium.org: Please review changes in 
profile_manager.cc

[commit-bot: I haz the power, 2016-08-12 19:41:31]

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

[commit-bot: I haz the power, 2016-08-12 19:41:31]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2016-08-15 18:02:16]

profile_manager.cc lgtm

[afakhry, 2016-08-15 21:27:16]

On 2016/08/15 18:02:16, anthonyvd wrote:
> profile_manager.cc lgtm

tommi, friendly ping. Thank you.

[afakhry, 2016-08-16 16:48:43]

Description was changed from

==========
Avoid a crash in ProfileManager::GetLastUsedProfileAllowedByPolicy()

BUG=637333
==========

to

==========
Avoid a crash in ProfileManager::GetLastUsedProfileAllowedByPolicy()

BUG=637333
==========

[afakhry, 2016-08-16 16:48:44]

afakhry@chromium.org changed reviewers:
- tommi@chromium.org

[afakhry, 2016-08-16 16:49:45]

afakhry@chromium.org changed reviewers:
+ sergeyu@chromium.org

[afakhry, 2016-08-16 16:49:45]

tommi seems to be OOO.

sergeyu@chromium.org: Please review changes in 
tab_desktop_media_list.cc

[tommi (sloooow) - chröme, 2016-08-17 11:37:59]

tommi@chromium.org changed reviewers:
+ tommi@chromium.org

[tommi (sloooow) - chröme, 2016-08-17 11:38:00]

rs lgtm

https://codereview.chromium.org/2240823003/diff/20001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.cc (right):

https://codereview.chromium.org/2240823003/diff/20001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:76: if (profile) {
nit: Change to an early return?

[afakhry, 2016-08-17 15:33:00]

https://codereview.chromium.org/2240823003/diff/20001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.cc (right):

https://codereview.chromium.org/2240823003/diff/20001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:76: if (profile) {
On 2016/08/17 11:38:00, tommi-ööö (chrömium) wrote:
> nit: Change to an early return? 

Do we need to execute the ScheduleNextRefresh at the end of the function in this
case?

[Sergey Ulanov, 2016-08-18 05:42:26]

lgtm

https://codereview.chromium.org/2240823003/diff/20001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.cc (right):

https://codereview.chromium.org/2240823003/diff/20001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:76: if (profile) {
On 2016/08/17 15:33:00, afakhry wrote:
> On 2016/08/17 11:38:00, tommi-ööö (chrömium) wrote:
> > nit: Change to an early return? 
> 
> Do we need to execute the ScheduleNextRefresh at the end of the function in
this
> case?

If profile can change then the answer is yes. Maybe ,ove the code that posts
task for ScheduleNextRefresh() before this line?

[afakhry, 2016-08-18 16:33:45]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 16:34:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2016-08-18 16:34:52]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-08-18 16:35:45]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[afakhry, 2016-08-18 16:36:33]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[afakhry, 2016-08-18 16:36:51]

https://codereview.chromium.org/2240823003/diff/20001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.cc (right):

https://codereview.chromium.org/2240823003/diff/20001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:76: if (profile) {
On 2016/08/18 05:42:26, Sergey Ulanov wrote:
> On 2016/08/17 15:33:00, afakhry wrote:
> > On 2016/08/17 11:38:00, tommi-ööö (chrömium) wrote:
> > > nit: Change to an early return? 
> > 
> > Do we need to execute the ScheduleNextRefresh at the end of the function in
> this
> > case?
> 
> If profile can change then the answer is yes. Maybe ,ove the code that posts
> task for ScheduleNextRefresh() before this line?

It seems from the comment above the PostTaskAndRepy() at the end, that it should
happen after UpdateSourceThumbnail() has been called for all favicons.

Looks like this should be just an early return. PTAL.

[commit-bot: I haz the power, 2016-08-18 16:37:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 17:27:05]

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

[commit-bot: I haz the power, 2016-08-18 17:27:06]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2016-08-20 01:08:49]

https://codereview.chromium.org/2240823003/diff/60001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.cc (right):

https://codereview.chromium.org/2240823003/diff/60001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:77: return;
I think ScheduleNextRefresh still need to be posted in this case. You can just
call here directly:
 if (!profile) {
   ScheduleNextRefresh();
   return;
 }

Looking at this code again I'm not sure GetLastUsedProfileAllowedByPolicy() is
the right thing to do here. It's probably better to pass Profile in the
constructor and use it here.

[afakhry, 2016-08-20 01:33:38]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[afakhry, 2016-08-20 01:33:52]

https://codereview.chromium.org/2240823003/diff/60001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.cc (right):

https://codereview.chromium.org/2240823003/diff/60001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:77: return;
On 2016/08/20 01:08:48, Sergey Ulanov wrote:
> I think ScheduleNextRefresh still need to be posted in this case. You can just
> call here directly:
>  if (!profile) {
>    ScheduleNextRefresh();
>    return;
>  }
> 

Done.

> Looking at this code again I'm not sure GetLastUsedProfileAllowedByPolicy() is
> the right thing to do here. It's probably better to pass Profile in the
> constructor and use it here.

This comment should probably be handled as a refactor in a separate CL. For now
let's keep this to fixing the crash.

[commit-bot: I haz the power, 2016-08-20 01:34:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-20 04:00:21]

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

[commit-bot: I haz the power, 2016-08-20 04:00:22]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[afakhry, 2016-08-22 16:24:05]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2016-08-22 16:24:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, tommi@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2240823003/#ps80001
(title: "schedule next refresh")

[commit-bot: I haz the power, 2016-08-22 16:24:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 17:47:21]

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

[commit-bot: I haz the power, 2016-08-22 17:47:23]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[afakhry, 2016-08-22 18:02:31]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2016-08-22 18:03:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 18:53:52]

Description was changed from

==========
Avoid a crash in ProfileManager::GetLastUsedProfileAllowedByPolicy()

BUG=637333
==========

to

==========
Avoid a crash in ProfileManager::GetLastUsedProfileAllowedByPolicy()

BUG=637333
==========

[commit-bot: I haz the power, 2016-08-22 18:53:54]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-08-22 18:55:44]

Description was changed from

==========
Avoid a crash in ProfileManager::GetLastUsedProfileAllowedByPolicy()

BUG=637333
==========

to

==========
Avoid a crash in ProfileManager::GetLastUsedProfileAllowedByPolicy()

BUG=637333

Committed: https://crrev.com/9f27972d0fbb00d451c9ae3f741d1e22ddd25891
Cr-Commit-Position: refs/heads/master@{#413490}
==========

[commit-bot: I haz the power, 2016-08-22 18:55:46]

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