New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 602807 link

Starred by 8 users

Issue metadata

Status: Verified
Owner:
Closed: May 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Corp account's windows are shown on multi-desktops

Project Member Reported by x...@chromium.org, Apr 12 2016

Issue description

OS: Chrome

The bug was reported by jmercay@ and was extracted from an email thread.

jmercay@:
"I'm been using the multi-desktop feature on my Pixel 2 for months: the main profile is my corp account, with my personal one as secondary. It is very helpful to have two desktops with their own set of windows and apps.

I got the latest stable build yesterday (49.0.2623.95), and multi-desktop seems to be gone. The usual next / previous keys (ctrl-alt-</>) do switch user (without any indications, since my shelf is auto-hidden). But all the windows share the same desktop in a unpredictable (and confusing) way: when the primary account is active, I see only those windows, but when the secondary user is active, I see both set of windows. This makes alt-tab window switching very difficult."

And a reboot fixed the problem described above.


 

Comment 1 by x...@chromium.org, Apr 12 2016

I guess the reason might be that the owner of the windows which belong to the corp account is set to an empty account id, and that's why they get shown on both desktops somehow.

I suspect the root reason might be same with Issue 532535. GetAccountIdFromProfile() might return an empty account id if the refresh token is revoked in the session (that also explains why it happened to the corp account).

Comment 2 by x...@chromium.org, Apr 14 2016

Cc: josa...@chromium.org x...@chromium.org
 Issue 602159  has been merged into this issue.
Cc: xiy...@chromium.org dhadd...@chromium.org sdantul...@chromium.org abod...@chromium.org
 Issue 601525  has been merged into this issue.
Project Member

Comment 5 by bugdroid1@chromium.org, May 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841

commit d2a0bb5307f8d0d09aff9d5d531dd9704fe85841
Author: xdai <xdai@chromium.org>
Date: Fri May 06 20:07:20 2016

GetAccountIdFromProfile() should not return an empty account id if a valid profile is provided.

On Chrome OS, we don't end the session immediately if a refresh token is revoked in the session. Thus it's possible that we get an empty account id for a profile from Profile::GetProfileUserName(). We should avoid this scenario on Chrome OS.

BUG= 602807 , 532535

Review-Url: https://codereview.chromium.org/1880923003
Cr-Commit-Position: refs/heads/master@{#392133}

[modify] https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841/chrome/browser/chromeos/profiles/profile_helper.h
[modify] https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841/chrome/browser/signin/signin_error_notifier_ash_unittest.cc
[modify] https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841/chrome/browser/sync/sync_error_notifier_ash_unittest.cc
[modify] https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841/chrome/browser/ui/ash/chrome_screenshot_grabber_unittest.cc
[modify] https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841/chrome/browser/ui/ash/multi_user/multi_user_util.cc
[add] https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841/chrome/browser/ui/ash/multi_user/multi_user_util_chromeos_unittest.cc
[modify] https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841/chrome/browser/ui/browser_finder_chromeos_unittest.cc
[modify] https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841/chrome/chrome_tests_unit.gypi
[modify] https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841/components/signin/core/browser/fake_signin_manager.cc
[modify] https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841/components/signin/core/browser/fake_signin_manager.h

Comment 6 by x...@chromium.org, May 6 2016

Status: Fixed (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, May 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8b8411dffb41a7f908aab07401953721c1395b2c

commit 8b8411dffb41a7f908aab07401953721c1395b2c
Author: thakis <thakis@chromium.org>
Date: Sat May 07 14:13:27 2016

Revert of GetAccountIdFromProfile() should not return an empty account id if a valid profile is provided. (patchset #7 id:160001 of https://codereview.chromium.org/1880923003/ )

Reason for revert:
Causes leaks: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/12421/steps/unit_tests%20on%20Ubuntu-12.04/logs/BrowserFinderChromeOSTest.FindBrowserOwnedByAnotherProfile

=================================================================
==5853==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0xa4f11b in operator new(unsigned long) (/tmp/runJuOWov/out/Release/unit_tests+0xa4f11b)
    #1 0xf4375a6 in chrome::MultiUserWindowManagerChromeOS::AddUser(content::BrowserContext*) chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:405:7
    #2 0x5654cde in test::BrowserFinderChromeOSTest::CreateMultiUserProfile(AccountId const&) chrome/browser/ui/browser_finder_chromeos_unittest.cc:43:5
    #3 0x237acd9 in BrowserWithTestWindowTest::SetUp() chrome/test/base/browser_with_test_window_test.cc:67:14
    #4 0x56539c2 in test::BrowserFinderChromeOSTest::SetUp() chrome/browser/ui/browser_finder_chromeos_unittest.cc:76:5
    #5 0x73422b6 in HandleExceptionsInMethodIfSupported<testing::Test, void> testing/gtest/src/gtest.cc:2458:12
    #6 0x73422b6 in testing::Test::Run() testing/gtest/src/gtest.cc:2470

Original issue's description:
> GetAccountIdFromProfile() should not return an empty account id if a valid profile is provided.
>
> On Chrome OS, we don't end the session immediately if a refresh token is revoked in the session. Thus it's possible that we get an empty account id for a profile from Profile::GetProfileUserName(). We should avoid this scenario on Chrome OS.
>
> BUG= 602807 , 532535
>
> Committed: https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841
> Cr-Commit-Position: refs/heads/master@{#392133}

TBR=skuhne@chromium.org,rogerta@chromium.org,xdai@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 602807 , 532535

Review-Url: https://codereview.chromium.org/1962463002
Cr-Commit-Position: refs/heads/master@{#392258}

[modify] https://crrev.com/8b8411dffb41a7f908aab07401953721c1395b2c/chrome/browser/chromeos/profiles/profile_helper.h
[modify] https://crrev.com/8b8411dffb41a7f908aab07401953721c1395b2c/chrome/browser/signin/signin_error_notifier_ash_unittest.cc
[modify] https://crrev.com/8b8411dffb41a7f908aab07401953721c1395b2c/chrome/browser/sync/sync_error_notifier_ash_unittest.cc
[modify] https://crrev.com/8b8411dffb41a7f908aab07401953721c1395b2c/chrome/browser/ui/ash/chrome_screenshot_grabber_unittest.cc
[modify] https://crrev.com/8b8411dffb41a7f908aab07401953721c1395b2c/chrome/browser/ui/ash/multi_user/multi_user_util.cc
[delete] https://crrev.com/db5fc4091985d801c3ff271a10fa59518ca47459/chrome/browser/ui/ash/multi_user/multi_user_util_chromeos_unittest.cc
[modify] https://crrev.com/8b8411dffb41a7f908aab07401953721c1395b2c/chrome/browser/ui/browser_finder_chromeos_unittest.cc
[modify] https://crrev.com/8b8411dffb41a7f908aab07401953721c1395b2c/chrome/chrome_tests_unit.gypi
[modify] https://crrev.com/8b8411dffb41a7f908aab07401953721c1395b2c/components/signin/core/browser/fake_signin_manager.cc
[modify] https://crrev.com/8b8411dffb41a7f908aab07401953721c1395b2c/components/signin/core/browser/fake_signin_manager.h

Comment 8 by x...@chromium.org, May 12 2016

 Issue 611438  has been merged into this issue.
Project Member

Comment 9 by bugdroid1@chromium.org, May 16 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a874a3155e9437511648e77acac3f8df0802de5

commit 8a874a3155e9437511648e77acac3f8df0802de5
Author: xdai <xdai@chromium.org>
Date: Mon May 16 19:22:17 2016

[Reland] GetAccountIdFromProfile() should not return an empty account id if a valid profile is provided.

This is a reland of https://codereview.chromium.org/1880923003/.

On Chrome OS, we don't end the session immediately if a refresh token is revoked in the session. Thus it's possible that we get an empty account id for a profile from Profile::GetProfileUserName(). We should avoid this scenario on Chrome OS.

BUG= 602807 
TBR=thakis@chromium.org, rogerta@chromium.org, skuhne@chromium.org

Committed: https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841
Cr-Commit-Position: refs/heads/master@{#392133}

Review-Url: https://codereview.chromium.org/1984733002
Cr-Commit-Position: refs/heads/master@{#393895}

[modify] https://crrev.com/8a874a3155e9437511648e77acac3f8df0802de5/chrome/browser/chromeos/profiles/profile_helper.h
[modify] https://crrev.com/8a874a3155e9437511648e77acac3f8df0802de5/chrome/browser/signin/signin_error_notifier_ash_unittest.cc
[modify] https://crrev.com/8a874a3155e9437511648e77acac3f8df0802de5/chrome/browser/sync/sync_error_notifier_ash_unittest.cc
[modify] https://crrev.com/8a874a3155e9437511648e77acac3f8df0802de5/chrome/browser/ui/ash/chrome_screenshot_grabber_unittest.cc
[modify] https://crrev.com/8a874a3155e9437511648e77acac3f8df0802de5/chrome/browser/ui/ash/multi_user/multi_user_context_menu_chromeos_unittest.cc
[modify] https://crrev.com/8a874a3155e9437511648e77acac3f8df0802de5/chrome/browser/ui/ash/multi_user/multi_user_util.cc
[add] https://crrev.com/8a874a3155e9437511648e77acac3f8df0802de5/chrome/browser/ui/ash/multi_user/multi_user_util_chromeos_unittest.cc
[modify] https://crrev.com/8a874a3155e9437511648e77acac3f8df0802de5/chrome/browser/ui/browser_finder_chromeos_unittest.cc
[modify] https://crrev.com/8a874a3155e9437511648e77acac3f8df0802de5/chrome/chrome_tests_unit.gypi
[modify] https://crrev.com/8a874a3155e9437511648e77acac3f8df0802de5/components/signin/core/browser/fake_signin_manager.cc
[modify] https://crrev.com/8a874a3155e9437511648e77acac3f8df0802de5/components/signin/core/browser/fake_signin_manager.h

Comment 10 by x...@chromium.org, May 17 2016

Labels: Merge-Request-51

Comment 11 by tin...@google.com, May 17 2016

Labels: -Merge-Request-51 Merge-Review-51 Hotlist-Merge-Review
[Automated comment] Less than 2 weeks to go before stable on M51, manual review required.
Status: Verified (was: Fixed)
Bulk verified

Comment 13 by zea@chromium.org, Jun 2 2016

 Issue 616656  has been merged into this issue.
 Issue 617354  has been merged into this issue.

Comment 15 by x...@chromium.org, Jul 18 2016

Cc: osh...@chromium.org
 Issue 618560  has been merged into this issue.

Comment 16 by derat@chromium.org, Jul 18 2016

Cc: tinazh@chromium.org
It's probably too late to make a difference here, but what happened to the merge to 51? It was requested in May, and then... nothing, as far as I can tell. Tina, did this slip through the cracks?

Comment 17 by tin...@google.com, Aug 4 2016

hey, pls check with M51 Chrome OS TPM:
Chrome OS:	Bernie Thompson
Cc: bhthompson@chromium.org
Bernie, do you know what happened with the merge request?
It looks like this came in just before the stable push and appears to have been missed (maybe as it has no milestone label? I probably need to be looser on queries). 

I probably should have caught this so that is my bad, FWIW it also appears that this was not a release blocker or a high priority bug, so it is not clear if we would have wanted to merge this so late in the cycle anyway. 
Thanks for investigating! I'll watch for missing milestone labels in the future.

Sign in to add a comment