Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2110083007:
Updating FeatureList default initialization pattern (Closed)
DescriptionUpdating FeatureList default initialization pattern
FeatureList currently expects a singleton to be manually set by all code
which uses it. This works fine in the browser where it is intialized
and in unit tests which use a default initializer, however for code
outside the browser which relies on the various components in Chromium,
this causes their code to crash.
An example of this is CL:
https://codereview.chromium.org/2101283004/
This was an innocuous change in a component which CRD relied on.
Since the unit tests are set up to never catch these kind of problems,
our component started crashing and we had to investigate to figure out
the root cause.
This change updates the feature and trial checks to initialize the
singleton if it was not already initialized. If a caller tries to
re-initialize the singleton after that, the init code will DCHECK as
this represents a race condition and we want to ensure consistency for
the callers who rely on these checks being accurate.
BUG=624879
Committed: https://crrev.com/958f0473b8039a057b0e31412c2acae7dfda39cd
Cr-Commit-Position: refs/heads/master@{#404243}
Patch Set 1 #Patch Set 2 : Fixing some unittest expectations #Patch Set 3 : Fixing some unit test comments #Patch Set 4 : Fixing a DCHECK vs. CHECK problem #
Total comments: 6
Patch Set 5 : Adding some thread safety. #Patch Set 6 : Updating the init check and removing lock. #Patch Set 7 : Fixing a windows build issue. #
Total comments: 10
Patch Set 8 : Addressing feedback #
Total comments: 4
Patch Set 9 : Addressing feedback #
Messages
Total messages: 24 (5 generated)
joedow@chromium.org changed reviewers: + asvitkine@chromium.org, sergeyu@chromium.org
joedow@chromium.org changed required reviewers: + asvitkine@chromium.org
This is the FeatureList initialization change we dicsussed today, PTAL! Thanks, Joe
Fixing a unit test comment.
Fixing a DCHECK/CHECK problem.
Ping!
https://codereview.chromium.org/2110083007/diff/60001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:134: CHECK(InitializeInstance(std::string(), std::string())); As discussed offline I think the problem here is this initialization is not thread-safe. Previously InitializeInstance() wasn't thread-safe, but that would be a problem only in case it's used incorrectly. With this change IsEnabled() becomes non-thread-safe even in case it's used correctly. IMO a good solution to this problem is to change this class to use base::Singleton<> as it implements thread-safe initialization.
https://codereview.chromium.org/2110083007/diff/60001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:134: CHECK(InitializeInstance(std::string(), std::string())); On 2016/07/01 21:36:10, Sergey Ulanov wrote: > As discussed offline I think the problem here is this initialization is not > thread-safe. Previously InitializeInstance() wasn't thread-safe, but that would > be a problem only in case it's used incorrectly. With this change IsEnabled() > becomes non-thread-safe even in case it's used correctly. > IMO a good solution to this problem is to change this class to use > base::Singleton<> as it implements thread-safe initialization. I don't think we want this class to use base::Singleton since we don't want to incur grabbing a lock when checking IsEnabled(). However, I agree that we shouldn't call InitializeInstance() from IsEnabled(). The other way to do this is something like: if (!GetInstance()) { initialized_from_accessor_ = true; return false; } And make initialized_from_accessor_ a static member. Now, it does mean that the field may be written from multiple thread without synchronization - but actually this will be fine in practice. That's because this field is only read from InitializeInstance which only gets called in single threaded mode. In the case where a binary that doesn't initialize an instance runs this, then the field will just keep getting written to but never read, so that's OK - no races.
https://codereview.chromium.org/2110083007/diff/60001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:134: CHECK(InitializeInstance(std::string(), std::string())); On 2016/07/04 15:50:06, Alexei Svitkine (very slow) wrote: > On 2016/07/01 21:36:10, Sergey Ulanov wrote: > > As discussed offline I think the problem here is this initialization is not > > thread-safe. Previously InitializeInstance() wasn't thread-safe, but that > would > > be a problem only in case it's used incorrectly. With this change IsEnabled() > > becomes non-thread-safe even in case it's used correctly. > > IMO a good solution to this problem is to change this class to use > > base::Singleton<> as it implements thread-safe initialization. > > I don't think we want this class to use base::Singleton since we don't want to > incur grabbing a lock when checking IsEnabled(). base::Singleton<>::get() doesn't have any locks. It uses atomic ops to make access and initialization thread-safe.
https://codereview.chromium.org/2110083007/diff/60001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:134: CHECK(InitializeInstance(std::string(), std::string())); On 2016/07/04 16:52:53, Sergey Ulanov wrote: > On 2016/07/04 15:50:06, Alexei Svitkine (very slow) wrote: > > On 2016/07/01 21:36:10, Sergey Ulanov wrote: > > > As discussed offline I think the problem here is this initialization is not > > > thread-safe. Previously InitializeInstance() wasn't thread-safe, but that > > would > > > be a problem only in case it's used incorrectly. With this change > IsEnabled() > > > becomes non-thread-safe even in case it's used correctly. > > > IMO a good solution to this problem is to change this class to use > > > base::Singleton<> as it implements thread-safe initialization. > > > > I don't think we want this class to use base::Singleton since we don't want to > > incur grabbing a lock when checking IsEnabled(). > > base::Singleton<>::get() doesn't have any locks. It uses atomic ops to make > access and initialization thread-safe. Still, that's more overhead than we have currently - atomic ops can flush caches and so forth on multi-proc systems and we this API to very lightweight.
I've updated the CL based on the feedback I've received, PTAL! Thanks, Joe https://codereview.chromium.org/2110083007/diff/60001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:134: CHECK(InitializeInstance(std::string(), std::string())); On 2016/07/04 16:55:55, Alexei Svitkine (very slow) wrote: > On 2016/07/04 16:52:53, Sergey Ulanov wrote: > > On 2016/07/04 15:50:06, Alexei Svitkine (very slow) wrote: > > > On 2016/07/01 21:36:10, Sergey Ulanov wrote: > > > > As discussed offline I think the problem here is this initialization is > not > > > > thread-safe. Previously InitializeInstance() wasn't thread-safe, but that > > > would > > > > be a problem only in case it's used incorrectly. With this change > > IsEnabled() > > > > becomes non-thread-safe even in case it's used correctly. > > > > IMO a good solution to this problem is to change this class to use > > > > base::Singleton<> as it implements thread-safe initialization. > > > > > > I don't think we want this class to use base::Singleton since we don't want > to > > > incur grabbing a lock when checking IsEnabled(). > > > > base::Singleton<>::get() doesn't have any locks. It uses atomic ops to make > > access and initialization thread-safe. > > Still, that's more overhead than we have currently - atomic ops can flush caches > and so forth on multi-proc systems and we this API to very lightweight. My concern with the feedback above is that I don't think we should always return false from IsEnabled(), I think it is better to return the feature's default_state instead, that way consumers of feature enabled components will get the intended default state if the FeatureList is not initialized in that binary.
https://codereview.chromium.org/2110083007/diff/60001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:134: CHECK(InitializeInstance(std::string(), std::string())); On 2016/07/05 21:05:56, joedow wrote: > On 2016/07/04 16:55:55, Alexei Svitkine (very slow) wrote: > > On 2016/07/04 16:52:53, Sergey Ulanov wrote: > > > On 2016/07/04 15:50:06, Alexei Svitkine (very slow) wrote: > > > > On 2016/07/01 21:36:10, Sergey Ulanov wrote: > > > > > As discussed offline I think the problem here is this initialization is > > not > > > > > thread-safe. Previously InitializeInstance() wasn't thread-safe, but > that > > > > would > > > > > be a problem only in case it's used incorrectly. With this change > > > IsEnabled() > > > > > becomes non-thread-safe even in case it's used correctly. > > > > > IMO a good solution to this problem is to change this class to use > > > > > base::Singleton<> as it implements thread-safe initialization. > > > > > > > > I don't think we want this class to use base::Singleton since we don't > want > > to > > > > incur grabbing a lock when checking IsEnabled(). > > > > > > base::Singleton<>::get() doesn't have any locks. It uses atomic ops to make > > > access and initialization thread-safe. > > > > Still, that's more overhead than we have currently - atomic ops can flush > caches > > and so forth on multi-proc systems and we this API to very lightweight. > > My concern with the feedback above is that I don't think we should always return > false from IsEnabled(), I think it is better to return the feature's > default_state instead, that way consumers of feature enabled components will get > the intended default state if the FeatureList is not initialized in that binary. That's a good point. Returning default state SGTM. https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc#n... base/feature_list.cc:27: bool initialized_from_accessor = false; Nit: add g_ https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc#n... base/feature_list.cc:136: if (!GetInstance()) { Nit: Change this and line 140 to g_instance. No need for extra function calls. :) https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc#n... base/feature_list.cc:145: if (!GetInstance()) { Why do we need to add this check? In a non-Chrome case, why would anyone call GetFieldTrial()? https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc#n... base/feature_list.cc:172: DCHECK(!initialized_from_accessor); Make this a CHECK(). I would like this to fail in non-DCHECK builds too, which is true for previous failure mode (using IsEnabled() before FeatureLIst is initialized).
Addressed feedback, PTAL! https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc#n... base/feature_list.cc:27: bool initialized_from_accessor = false; On 2016/07/05 21:11:34, Alexei Svitkine (very slow) wrote: > Nit: add g_ Done. https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc#n... base/feature_list.cc:136: if (!GetInstance()) { On 2016/07/05 21:11:34, Alexei Svitkine (very slow) wrote: > Nit: Change this and line 140 to g_instance. No need for extra function calls. > :) Done. https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc#n... base/feature_list.cc:145: if (!GetInstance()) { On 2016/07/05 21:11:34, Alexei Svitkine (very slow) wrote: > Why do we need to add this check? > > In a non-Chrome case, why would anyone call GetFieldTrial()? I'm anticipating a scenario where a component which regularly ships with Chrome wants to participate in or key off of a FieldTrial but we don't initialize the Singleton. In that case, we would see a crash after syncing and need to root cause it (just like the IsEnabled() check this time around). https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc#n... base/feature_list.cc:172: DCHECK(!initialized_from_accessor); On 2016/07/05 21:11:34, Alexei Svitkine (very slow) wrote: > Make this a CHECK(). I would like this to fail in non-DCHECK builds too, which > is true for previous failure mode (using IsEnabled() before FeatureLIst is > initialized). Done.
lgtm https://codereview.chromium.org/2110083007/diff/140001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/140001/base/feature_list.cc#n... base/feature_list.cc:172: CHECK(!g_initialized_from_accessor); nit: I don't think this needs to be a CHECK(). Replace with a DCHECK()?
https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc#n... base/feature_list.cc:145: if (!GetInstance()) { On 2016/07/05 22:24:20, joedow wrote: > On 2016/07/05 21:11:34, Alexei Svitkine (very slow) wrote: > > Why do we need to add this check? > > > > In a non-Chrome case, why would anyone call GetFieldTrial()? > > I'm anticipating a scenario where a component which regularly ships with Chrome > wants to participate in or key off of a FieldTrial but we don't initialize the > Singleton. In that case, we would see a crash after syncing and need to root > cause it (just like the IsEnabled() check this time around). It's not obvious to me this would happen. I don't foresee anything outside the current uses ever using this API. Now, it's used by the GetVariationParamValueByFeature() API. So if that API gets used, then yes you could run into this. However - for that API to be used, you'd have to have the code using it depend on components/variations. Could that ever happen - i.e. the binaries you care about, will they ever depend on arbitrary components such as component/variations? https://codereview.chromium.org/2110083007/diff/140001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/140001/base/feature_list.cc#n... base/feature_list.cc:140: return GetInstance()->IsFeatureEnabled(feature); Nit: Use g_instance here too. https://codereview.chromium.org/2110083007/diff/140001/base/feature_list.cc#n... base/feature_list.cc:172: CHECK(!g_initialized_from_accessor); On 2016/07/06 19:11:51, Sergey Ulanov wrote: > nit: I don't think this needs to be a CHECK(). Replace with a DCHECK()? See my earlier comments, I want to preserve the same conditions under which it can fail and since previously it would crash even in release, I would like that to still be the case with these updated checks.
PTAL! https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/120001/base/feature_list.cc#n... base/feature_list.cc:145: if (!GetInstance()) { On 2016/07/06 19:59:06, Alexei Svitkine (very slow) wrote: > On 2016/07/05 22:24:20, joedow wrote: > > On 2016/07/05 21:11:34, Alexei Svitkine (very slow) wrote: > > > Why do we need to add this check? > > > > > > In a non-Chrome case, why would anyone call GetFieldTrial()? > > > > I'm anticipating a scenario where a component which regularly ships with > Chrome > > wants to participate in or key off of a FieldTrial but we don't initialize the > > Singleton. In that case, we would see a crash after syncing and need to root > > cause it (just like the IsEnabled() check this time around). > > It's not obvious to me this would happen. I don't foresee anything outside the > current uses ever using this API. > > Now, it's used by the GetVariationParamValueByFeature() API. So if that API gets > used, then yes you could run into this. However - for that API to be used, you'd > have to have the code using it depend on components/variations. Could that ever > happen - i.e. the binaries you care about, will they ever depend on arbitrary > components such as component/variations? I can remove it for now. Our problem is that we don't know when a component updates their dependencies until we sync. If that does occur we can add this back in. https://codereview.chromium.org/2110083007/diff/140001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2110083007/diff/140001/base/feature_list.cc#n... base/feature_list.cc:140: return GetInstance()->IsFeatureEnabled(feature); On 2016/07/06 19:59:06, Alexei Svitkine (very slow) wrote: > Nit: Use g_instance here too. Done.
lgtm
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2110083007/#ps160001 (title: "Addressing feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Updating FeatureList default initialization pattern FeatureList currently expects a singleton to be manually set by all code which uses it. This works fine in the browser where it is intialized and in unit tests which use a default initializer, however for code outside the browser which relies on the various components in Chromium, this causes their code to crash. An example of this is CL: https://codereview.chromium.org/2101283004/ This was an innocuous change in a component which CRD relied on. Since the unit tests are set up to never catch these kind of problems, our component started crashing and we had to investigate to figure out the root cause. This change updates the feature and trial checks to initialize the singleton if it was not already initialized. If a caller tries to re-initialize the singleton after that, the init code will DCHECK as this represents a race condition and we want to ensure consistency for the callers who rely on these checks being accurate. BUG=624879 ========== to ========== Updating FeatureList default initialization pattern FeatureList currently expects a singleton to be manually set by all code which uses it. This works fine in the browser where it is intialized and in unit tests which use a default initializer, however for code outside the browser which relies on the various components in Chromium, this causes their code to crash. An example of this is CL: https://codereview.chromium.org/2101283004/ This was an innocuous change in a component which CRD relied on. Since the unit tests are set up to never catch these kind of problems, our component started crashing and we had to investigate to figure out the root cause. This change updates the feature and trial checks to initialize the singleton if it was not already initialized. If a caller tries to re-initialize the singleton after that, the init code will DCHECK as this represents a race condition and we want to ensure consistency for the callers who rely on these checks being accurate. BUG=624879 Committed: https://crrev.com/958f0473b8039a057b0e31412c2acae7dfda39cd Cr-Commit-Position: refs/heads/master@{#404243} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/958f0473b8039a057b0e31412c2acae7dfda39cd Cr-Commit-Position: refs/heads/master@{#404243} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
