Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(20)

Issue 23532034: Postpone loading about:flags ui until the certificates have been loaded (Closed)

Created:
7 years, 3 months ago by tbarzic
Modified:
7 years, 3 months ago
CC:
chromium-reviews, nkostylev+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, native-client-reviews_googlegroups.com, stevenjb+watch_chromium.org, davemoore+watch_chromium.org
Visibility:
Public.

Description

Postpone loading about:flags ui until the certificates have been loaded FlagsUI assumes that when DeviceSettingsService::GetOwnershipStatusAsync is done, it is known whether the current user is the owner. Since the certificates are loaded asynchronously it is possible that a SessionManagerOperation is completed (which triggers GetOwnershipStatusAsync callbacks) before the certs are loaded. In that case, the owner private key will not be known even it the current user is the owner, so FlagsUI could be created with a wrong FlagsDOMHandler. This is very likely to happen if the about:flags is opened during session restore. This CL changes GetOwnershipStatusAsync to only report ownership status (not whether the current user is the owner). HasPrivateOwnerKeyAsync is added. This method should be used to check whether the current user is the owner. It is guaranteed to trigger callback after the certificates have been loaded. Also, to avoid showing "This webpage is not available" for the FlagsUI on session restore (due to cert loading taking too long), change flags UI to load but not to show anything untli the ownership status is determined. BUG=280935 TEST=open chrome://flags and reboot. In the chrome://flags page opened during session restore (make sure 'Continue where I left off' setting is set), change enable-nacl flag and reboot. On the next login, there should be no black screen. Note: test steps assume usage of the owner account. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222104

Patch Set 1 #

Patch Set 2 : tests #

Patch Set 3 : . #

Patch Set 4 : . #

Patch Set 5 : . #

Patch Set 6 : . #

Total comments: 6

Patch Set 7 : . #

Total comments: 2

Patch Set 8 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+301 lines, -101 lines) Patch
M chrome/browser/chromeos/kiosk_mode/kiosk_mode_settings.h View 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/chromeos/kiosk_mode/kiosk_mode_settings.cc View 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/login/existing_user_controller.h View 2 chunks +2 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/login/existing_user_controller.cc View 3 chunks +3 lines, -6 lines 0 comments Download
M chrome/browser/chromeos/login/login_display_host_impl.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/login/login_display_host_impl.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/chromeos/login/parallel_authenticator.h View 1 2 3 4 5 6 7 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/chromeos/login/parallel_authenticator.cc View 1 2 3 4 5 6 7 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/login/user_manager_impl.h View 1 2 3 4 5 6 7 1 chunk +1 line, -5 lines 0 comments Download
M chrome/browser/chromeos/login/user_manager_impl.cc View 1 2 3 4 5 6 7 2 chunks +3 lines, -10 lines 0 comments Download
M chrome/browser/chromeos/policy/device_local_account_policy_store.h View 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/chromeos/policy/device_local_account_policy_store.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/chromeos/settings/device_settings_service.h View 1 2 3 4 5 3 chunks +14 lines, -1 line 0 comments Download
M chrome/browser/chromeos/settings/device_settings_service.cc View 1 2 3 4 5 5 chunks +40 lines, -5 lines 0 comments Download
M chrome/browser/chromeos/settings/device_settings_service_unittest.cc View 1 2 3 4 5 chunks +143 lines, -6 lines 0 comments Download
M chrome/browser/resources/flags.html View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/flags.js View 1 2 chunks +5 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/flags_ui.h View 1 chunk +0 lines, -10 lines 0 comments Download
M chrome/browser/ui/webui/flags_ui.cc View 1 2 3 4 5 6 8 chunks +76 lines, -34 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
tbarzic
Julian, can you take a look and see if this makes sense? https://codereview.chromium.org/23532034/diff/21001/chrome/browser/chromeos/login/user_manager_impl.cc File chrome/browser/chromeos/login/user_manager_impl.cc ...
7 years, 3 months ago (2013-09-04 23:57:25 UTC) #1
pastarmovj
I like this change! It LGTM. It makes a lot of the calling sites cleaner. ...
7 years, 3 months ago (2013-09-05 12:48:18 UTC) #2
tbarzic
https://codereview.chromium.org/23532034/diff/21001/chrome/browser/chromeos/login/parallel_authenticator.cc File chrome/browser/chromeos/login/parallel_authenticator.cc (right): https://codereview.chromium.org/23532034/diff/21001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode462 chrome/browser/chromeos/login/parallel_authenticator.cc:462: // TODO(tbarzic): This is broken. At this point, DeviceSettingsService ...
7 years, 3 months ago (2013-09-05 17:59:17 UTC) #3
tbarzic
+thakis: for flags ui code +dpolukhin: owner review for c/b/c/login
7 years, 3 months ago (2013-09-05 18:01:59 UTC) #4
Dmitry Polukhin
LGTM for c/b/c/login
7 years, 3 months ago (2013-09-05 18:49:28 UTC) #5
Nico
lgtm, sorry about the delay Is it possible to test the delayed request behavior in ...
7 years, 3 months ago (2013-09-06 22:48:45 UTC) #6
tbarzic
It looks like pastarmovj@ has an assigned bug to test about:flags (http://crbug.com/180228), so I think ...
7 years, 3 months ago (2013-09-07 00:49:34 UTC) #7
pastarmovj
On 2013/09/07 00:49:34, tbarzic wrote: > It looks like pastarmovj@ has an assigned bug to ...
7 years, 3 months ago (2013-09-09 08:26:48 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tbarzic@chromium.org/23532034/40001
7 years, 3 months ago (2013-09-09 18:45:09 UTC) #9
commit-bot: I haz the power
7 years, 3 months ago (2013-09-09 21:51:48 UTC) #10
Message was sent while issue was closed.
Change committed as 222104

Powered by Google App Engine
This is Rietveld 408576698