New issue
Advanced search Search tips

Issue 761123 link

Starred by 1 user
Status: Fixed
Owner:
jamescook@chromium.org
Closed: Sep 2017
Cc:
sa...@chromium.org
mustash-bugs@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug
Proj-Servicification



Sign in to add a comment

Put back DCHECK in overlay_user_pref_store.cc when incognito prefs are fixed

Project Member Reported by jamescook@chromium.org, Aug 31 2017 
https://chromium-review.googlesource.com/c/chromium/src/+/634329 causes the DCHECK below, but otherwise works fine. I commented out the DCHECK.

[178953:178953:0821/150928.521924:FATAL:overlay_user_pref_store.cc(84)] Check failed: ShallBeStoredInOverlay(key) || !overlay_->GetValue(key, NULL). 
#0 0x7efe84d88b4c base::debug::StackTrace::StackTrace()
#1 0x7efe84dae05c logging::LogMessage::~LogMessage()
#2 0x7efe80964c3f OverlayUserPrefStore::GetValue()
#3 0x7efe80970ac3 PrefValueStore::GetValueFromStoreWithType()
#4 0x7efe80970a2d PrefValueStore::GetValue()
#5 0x7efe8096bb7b PrefService::GetPreferenceValue()
#6 0x7efe8096b8e2 PrefService::GetBoolean()
#7 0x7efe7f921a16 ash::AccessibilityController::IsLargeCursorEnabled()
#8 0x7efe7fa0d5f6 ash::(anonymous namespace)::GetAccessibilityState()
#9 0x7efe7fa0d879 ash::TrayAccessibility::GetInitialVisibility()
#10 0x7efe7fa0dc60 ash::TrayAccessibility::OnAccessibilityStatusChanged()
#11 0x7efe7fa00184 ash::SystemTrayNotifier::NotifyAccessibilityStatusChanged()
#12 0x557b998319ec chromeos::AccessibilityManager::SetProfile()
#13 0x7f6e1ca14489 content::NotificationServiceImpl::Notify()
#14 0x557b9afe8aa0 chromeos::SigninScreenHandler::HandleLoginVisible()

https://chromium-review.googlesource.com/c/chromium/src/+/624772 will fix it, and we can put back the DCHECK.
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 1 2017 

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

commit ec14495df0f7ac39713e365138878ea462d2dbe7
Author: James Cook <jamescook@chromium.org>
Date: Fri Sep 01 00:05:16 2017

cros: Use prefs to communicate a11y large cursor state to ash

For go/mustash we need to eliminate in-process delegate calls from ash
back into chrome. This CL eliminates 2 AccessibilityDelegate methods.

* Connect to the signin screen profile pref service from ash
* Track the large cursor pref as a "foreign pref" from ash because chrome
has code that requires the pref to be registered at startup time
* Use pref changes to update the system tray menu
* Fix ash pref registration so that code using foreign prefs can be
unit tested more easily

TODO: Move the code that sets the cursor state out of chrome into ash

Bug: 594887,  760406 ,  761123 
Test: ash_unittests, chrome browser_tests
Change-Id: I8a4ebe5c65d815b20d8f4d014aa2d2dc7bdf8d62
Reviewed-on: https://chromium-review.googlesource.com/634329
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499078}
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/BUILD.gn
[add] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/accessibility/accessibility_controller.cc
[add] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/accessibility/accessibility_controller.h
[add] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/accessibility/accessibility_controller_unittest.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/accessibility_delegate.h
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/default_accessibility_delegate.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/default_accessibility_delegate.h
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/public/interfaces/pref_connector.mojom
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/session/session_controller.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/session/session_controller.h
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/session/session_controller_unittest.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/session/test_session_controller_client.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/shell.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/shell.h
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/system/tray_accessibility.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/system/tray_accessibility.h
[add] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/system/tray_accessibility_unittest.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/system/tray_caps_lock.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/system/tray_caps_lock.h
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/ash/system/tray_caps_lock_unittest.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/chrome/browser/chromeos/accessibility/accessibility_manager.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/chrome/browser/chromeos/login/enrollment/enrollment_screen_browsertest.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/chrome/browser/chromeos/login/wizard_controller_browsertest.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/chrome/browser/chromeos/policy/recommendation_restorer.h
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/chrome/browser/chromeos/preferences.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/chrome/browser/chromeos/prefs/pref_connector_service.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/chrome/browser/chromeos/prefs/pref_connector_service.h
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/chrome/browser/chromeos/system/tray_accessibility_browsertest.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/chrome/browser/ui/ash/chrome_shell_delegate.cc
[modify] https://crrev.com/ec14495df0f7ac39713e365138878ea462d2dbe7/components/prefs/overlay_user_pref_store.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 4 2017 

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

commit aa67d7f217d35c97787c140caf0c3e22643f7b08
Author: Sam McNally <sammc@chromium.org>
Date: Mon Sep 04 03:34:06 2017

Pref service: fix incognito support.

When creating a PrefService for incognito, the delegate holds onto the
underlay and the OverlayUserPrefStore (the composite view of incognito
prefs). Thus, clients observe all prefs being present in the overlay,
causing DCHECK failures.

Bind the PersistentPrefStoreImpls directly to the underlay and overlay
PrefStores so pref service clients observe prefs in the corresponding
pref stores as chrome. Pass the list of overlayed prefs to clients
(filtered to the prefs they observe) so they can correctly overlay.

Add unit tests for pref service with incognito prefs.

Change-Id: Iced63b840b682b6958d97853678ab9163c291f0c
Bug:  654988 ,  761123 
Reviewed-on: https://chromium-review.googlesource.com/624772
Commit-Queue: Sam McNally <sammc@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499432}
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/components/prefs/overlay_user_pref_store.cc
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/components/prefs/pref_value_store.h
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/components/sync_preferences/pref_service_syncable.cc
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/services/preferences/pref_service_factory_unittest.cc
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/services/preferences/pref_store_manager_impl.cc
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/services/preferences/pref_store_manager_impl.h
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/services/preferences/public/cpp/in_process_service_factory.cc
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/services/preferences/public/cpp/in_process_service_factory.h
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/services/preferences/public/cpp/pref_service_factory.cc
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/services/preferences/public/cpp/pref_service_main.cc
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/services/preferences/public/cpp/pref_service_main.h
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/services/preferences/public/interfaces/preferences.mojom
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/services/preferences/scoped_pref_connection_builder.cc
[modify] https://crrev.com/aa67d7f217d35c97787c140caf0c3e22643f7b08/services/preferences/scoped_pref_connection_builder.h

Comment 3 by sa...@chromium.org, Sep 5 2017

Status: Fixed (was: Assigned)

Comment 4 by jamescook@chromium.org, Sep 5 2017 

Thanks for taking care of this in your CL!

Comment 5 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 6 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)
►

Sign in to add a comment