Better to ask that an owner of the omnibox or search engine to do that, or maybe the author of the CL that was reverted.

The root of the problem was that the RLZ value was being cached locally by the search engine code.  This was likely based on the assumption that the RLZ values never changes at runtime.  We now know this is an incorrect assumption for cros.  I'm not exactly sure how to refactor RLZ so that users cannot cache the RLZ value.  Any thoughts?

Looking at the bug attached to the reverted change, seems like this may have been meant as a windows and/or mac only change, that inadvertently was applied to cros.  You should follow up with the folks on that bug to determine the real issue they were trying to address and implement a different fix.

Assigning to rpop to find an eng owner.

I think it's not just error-prone but it's also plain wrong to indicate RLZ opt-out by setting an empty brand code in [1] because (via google_brand::GetBrand()) there are many non-RLZ consumers of brand code in metrics, updater and other places.

Thus instead overloading the brand code with RLZ opt-out, we should convey opt-out through a separate function call.

[1] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/session/user_session_manager.cc?type=cs&q=UserSessionManager::InitRlzImpl&sq=package:chromium&l=1559

Let's say we change:

   std::string google_brand::GetBrand();

to something like:

   // Returns true if user is opted in to RLZ, false otherwise.  If opted in,
   // |brand| contains the brand code to use with RLZ and search endpoints.
   bool google_brand::GetBrand(std::string* brand);

This still does not prevent someone from caching the fact that RLZ is either opted in or not.  So does not prevent a recurrence of the error.  Callers need to know that they should never cache the values of this call.  Let me know if I'm missing something.

Just as a random thought, and with a lot of handwaving, wouldn't the safest measure against caching be the prevention of direct access?

void AppendRLZtoSearchURL(GURL* url_to_modify);
void AppendRLZtoInstallPing(URLRequest* request_to_modify);

Both access the string under the hood, but don't expose it.

> Returns true if user is opted in to RLZ, false otherwise.  If opted in,
> |brand| contains the brand code to use with RLZ and search endpoints.

As I wrote above, there are consumers who need the brand code even when RLZ is disabled. To my mind it would be cleaner to separate the concerns:

// may be cached
std::string GetBrand();

// must not be cached
bool IsRlzEnabled();

And also IsRlzEnabled() must default to false and only flip to true after it has been determined that the opt-out file doesn't exist. (As we have seen the current behaviour, defaulting to true and flipping to false when the opt-out file exists, is error-prone.)

I'm not the right owner for this. Who owned the CL that got reverted? msramek, are you interested in driving this to closure?

In my opinion this falls into the responsibility of the feature team (RLZ).

Small correction to comment #5: IsRlzEnabled() may be cached as well, but only once it has been guaranteed that the correct value has been seeded (e.g. after trying to read the RLZ opt-out file).