New issue
Advanced search Search tips

Issue 657939 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

HostDiscardableSharedMemoryManager reads "MemoryReduction" FieldTrial prior to FieldTrial initialization

Project Member Reported by nick@chromium.org, Oct 20 2016

Issue description

In the ctor of HostDiscardableSharedMemoryManager is the following line:

  memory_limit_(GetDefaultMemoryLimit())

This calls base::SysInfo::IsLowEndDevice(), which is controlled by a field trial, but this happens before the FieldTrial state is actually initialized, so the Find() operation on the field trial always returns null:

FieldTrial* FieldTrialList::Find(const std::string& trial_name) {
  if (!global_)
    return NULL;  // We hit this case.
  ...
}

In the calling function, SysInfo::IsLowEndDevice(), |group_name| is empty, so we never will hit the "return true" case here:

  if (StartsWith(group_name, "Enabled", CompareCase::SENSITIVE))
    return true;

cnwan@ -- you are the owner of this Finch trial
bashi@ -- you touched this GetDefaultMemoryLimit() call recently (note that I was debugging on a build prior to your r422700) 
 

Comment 1 by nick@chromium.org, Oct 20 2016

(note: I noticed this while debugging a similar issue with a different field trial;  bug 657629 . As part of that I aim to add a DCHECK to catch problems like this in the future)

Comment 2 by nick@chromium.org, Oct 20 2016

Cc: alex...@chromium.org
+alexmos fyi

Comment 3 by bashi@chromium.org, Oct 26 2016

Sorry I missed this and thanks for the report.

How can we make sure that IsLowEndDevice() and/or FieldTrial flags are ready to use? Since SysInfo live in base/, it's a bit difficult to foresee this kind of dependency problem. Also, I feel that it's unfortunate if we forbid to call base/ until some conditions are satisfied. IIUC base/ should work without having any dependency (but probably doesn't have to?).

We can PostTask() to the IO thread to execute GetDefaultMemoryLimit() when OnMemoryStateChange() is called, but I don't think it's a better solution because calling GetDefaultMemoryLimit() every time wouldn't make much sense (The value is unlikely change while running).

Sign in to add a comment