+stevenjb@ - any chance you have some cycles to help look into this?

And just to clarify, these failures appear when applying this CL[1]. The CL does not break any guarantees made by Mojo bindings, but it can affect the timing of arbitrary events. The failures therefore strongly imply that incorrect ordering assumptions being made somewhere, and this has been the case for all other blocking bugs which have been fixed so far.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1145692

Which builder specifically is failing? I don't see a failure on linux-chromeos-rel.

Those tests should only be getting run in non --mash mode. They shouldn't be testing anything using mojo as far as I can recall...

The failure unfortunately doesn't get listed by name, because the failure is actually in the pre-test setup (i.e. DisplayPrefsBrowserTest.PRE_DisplayRotation) and I guess we don't parse/report such failures properly. So it just looks like a generic browser_tests failure. It's this run[1] and this specific shard output[2].

[1] https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux-chromeos-rel/64188
[2] https://chromium-swarm.appspot.com/task?id=3f2aacac29d7b510&refresh=10

Ah. That's unfortunate.

So, the specific failure appears to be either:

local_state_->GetDictionary(ash::prefs::kDisplayProperties) -> nullptr

Or display properties for display 0 are not found.

I will try to repro this locally today or tomorrow, but I am mystified as to how a mojo change would affect this.

+jamescook@ for prefs fu

+msw, did you have to deal with prefs registration in shelf tests?

This is probably a prefs registration problem. It looks like ash::prefs::kDisplayProperties pref registration happens in ash:
https://cs.chromium.org/chromium/src/ash/display/display_prefs.cc?type=cs&sq=package:chromium&g=0&l=810

I believe chrome learns about those prefs asynchronously, as part of ash's mojo PrefService connection to the browser.

I could imagine that mojo message changes could break this, like if it takes more run loop spins now for chrome to learn about the pref.

I got around to investigating this more. It appears to be a legitimate race between SessionController and DisplayPrefs.

UserCanSaveDisplayPreference[1] returns false racily, because sometimes SessionController::AddUserSession hasn't been called by the time both DisplayPrefs::;OnLocalStatePrefServiceInitialized[2] *and* DisplayPrefs::StoreDisplayPrefs[3] have been called.

At this point I'm a little out of my depth and hoping someone else might be able to help correct the race. I would like to understand why we can't store display prefs until a session has been started, given that we do presumably have a working PrefService. The comments in display_prefs.cc say something about not saving while "the confirmation dialog is shown" but I don't know what that means.

Some possible solutions I was thinking about:

a) Make DisplayPrefs aware of SessionController events, like a session being added. This still feels weird to me because there doesn't seem to be any concern in DisplayPrefs for *which* session its storing the prefs for. So if it cares about user session, I would expect it to care about a specific user session.

b) Delete the check for UserCanSaveDisplayPreference, but this assumes there isn't actually a good reason for it.

c) Tie local state PrefService acquisition to session initialization. This assumes that pref storage in general must only be done during an active user session. I suspect that isn't true, but I would hope it is true given what this code is doing, combined with my ignorance of the system. :)

d) ???? 

[1] https://cs.chromium.org/chromium/src/ash/display/display_prefs.cc?rcl=d19ddb64b69ecf94a8b7cd6e0b43c5ebbbfdac95&l=208
[2] https://cs.chromium.org/chromium/src/ash/display/display_prefs.cc?rcl=d19ddb64b69ecf94a8b7cd6e0b43c5ebbbfdac95&l=845
[3] https://cs.chromium.org/chromium/src/ash/display/display_prefs.cc?rcl=d19ddb64b69ecf94a8b7cd6e0b43c5ebbbfdac95&l=860

Hacky solution: If you don't know what type the user is then assume the pref can be saved to local state. This might be bad because a user at the login screen could change the pref, but I don't think there's any display-related UI at the login screen. I don't know if Ctrl-Shift-plus/minus for resolution switches are persisted to local state. If they are persisted, then this hack won't work -- those keys work at the login screen.

Having DisplayPrefs wait for a session seems like a relatively simple and reliable way to solve this given that SessionObserver already supports hooking into session start. So hopefully we don't need to be hacky in this case.

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

commit bdc6f7711a46164362a9be5615279cf97fd83680
Author: Ken Rockot <rockot@google.com>
Date: Tue Oct 23 20:55:39 2018

[ash] Fix display pref storage

This races with session initialization and can cause display prefs to
fail to be saved properly. Exposed pretty consistently when making
subtle changes to Mojo dispatch timing.

The fix forces DisplayPrefs to wait for the first user session to be
started if a storage attempt happens prior to that.

Bug:  872074 
Change-Id: I40f9070b6a6421e8fa3bef2da44ab9e854d3d3c8
Reviewed-on: https://chromium-review.googlesource.com/c/1296547
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#602083}
[modify] https://crrev.com/bdc6f7711a46164362a9be5615279cf97fd83680/ash/display/cros_display_config.cc
[modify] https://crrev.com/bdc6f7711a46164362a9be5615279cf97fd83680/ash/display/display_configuration_observer.cc
[modify] https://crrev.com/bdc6f7711a46164362a9be5615279cf97fd83680/ash/display/display_prefs.cc
[modify] https://crrev.com/bdc6f7711a46164362a9be5615279cf97fd83680/ash/display/display_prefs.h
[modify] https://crrev.com/bdc6f7711a46164362a9be5615279cf97fd83680/ash/display/display_prefs_unittest.cc
[modify] https://crrev.com/bdc6f7711a46164362a9be5615279cf97fd83680/chrome/browser/chromeos/display/display_prefs_browsertest.cc