Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2546653002:
Store and retrieve features from shared memory (Closed)
|
Created:
4 years ago by lawrencewu Modified:
4 years ago CC:
chromium-reviews, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStore and retrieve features from shared memory
If using shared memory, stores base::Features in the field trial
allocator (under a different type defined in FeatureList) when
spawning the first new process. When a child process is created,
it will first try to get initialized from shared memory. If that
doesn't work, it'll fallback to the command line.
BUG=661224
Committed: https://crrev.com/5e03cd3432b1f7904a11a94766956e8871795d9f
Cr-Commit-Position: refs/heads/master@{#436311}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address comments and fallback to command line. #
Total comments: 6
Patch Set 3 : Address comments and write unittest #
Total comments: 5
Patch Set 4 : asvitkine@ comments #
Total comments: 20
Patch Set 5 : complete unit test #Patch Set 6 : fix some tests #Patch Set 7 : git rebase-update #Patch Set 8 : try fixing InstantiateAllocator on ios-simulator #Patch Set 9 : ifdef InstantiateAllocator test out for ios #
Total comments: 4
Patch Set 10 : address comment and remove ios ifdef #Patch Set 11 : add some logging statements #Patch Set 12 : add some more logging #Patch Set 13 : fix nit and use scoped_feature_list #Patch Set 14 : removing log statements #
Total comments: 7
Patch Set 15 : address nits #Patch Set 16 : add comment #
Messages
Total messages: 62 (34 generated)
self-review https://codereview.chromium.org/2546653002/diff/1/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2546653002/diff/1/base/feature_list.cc#newcode65 base/feature_list.cc:65: What we probably wanna do here is try calling InitializeFromSharedMemory() first here, and then fallback to using enable_features and disable_features. But we can't do that here, since we don't have access to kUseSharedMemoryForFieldTrials...maybe fallback in InitializeFieldTrialAndFeatureList()? https://codereview.chromium.org/2546653002/diff/1/base/feature_list.h File base/feature_list.h (right): https://codereview.chromium.org/2546653002/diff/1/base/feature_list.h#newcode96 base/feature_list.h:96: void InitializeFromSharedMemory(SharedPersistentMemoryAllocator* allocator); Write a docstring for this. Also maybe typedef SharedPersistentMemoryAllocator to FeatureAllocator? https://codereview.chromium.org/2546653002/diff/1/base/feature_list.h#newcode130 base/feature_list.h:130: void AddFeaturesToAllocator(SharedPersistentMemoryAllocator* allocator); Write docstring. https://codereview.chromium.org/2546653002/diff/1/base/feature_list.h#newcode188 base/feature_list.h:188: friend class FieldTrialList; Do we actually need this? https://codereview.chromium.org/2546653002/diff/1/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2546653002/diff/1/base/metrics/field_trial.h#... base/metrics/field_trial.h:599: static void AddTrialToAllocatorWhileLocked(FieldTrial* field_trial); Let's rename this back to AddToAllocatorWhileLocked. We can refactor this in another CL.
Address self-comments. https://codereview.chromium.org/2546653002/diff/1/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2546653002/diff/1/base/feature_list.cc#newcode65 base/feature_list.cc:65: On 2016/12/01 02:39:48, lawrencewu wrote: > What we probably wanna do here is try calling InitializeFromSharedMemory() first > here, and then fallback to using enable_features and disable_features. But we > can't do that here, since we don't have access to > kUseSharedMemoryForFieldTrials...maybe fallback in > InitializeFieldTrialAndFeatureList()? Moved the fallback code into a new function CreateFeaturesFromCommandLine(). https://codereview.chromium.org/2546653002/diff/1/base/feature_list.h File base/feature_list.h (right): https://codereview.chromium.org/2546653002/diff/1/base/feature_list.h#newcode96 base/feature_list.h:96: void InitializeFromSharedMemory(SharedPersistentMemoryAllocator* allocator); On 2016/12/01 02:39:48, lawrencewu wrote: > Write a docstring for this. Also maybe typedef SharedPersistentMemoryAllocator > to FeatureAllocator? We also need to enforce disabled features over enabled features, since that's the original behaviour. https://codereview.chromium.org/2546653002/diff/1/base/feature_list.h#newcode130 base/feature_list.h:130: void AddFeaturesToAllocator(SharedPersistentMemoryAllocator* allocator); On 2016/12/01 02:39:48, lawrencewu wrote: > Write docstring. Done. https://codereview.chromium.org/2546653002/diff/1/base/feature_list.h#newcode188 base/feature_list.h:188: friend class FieldTrialList; On 2016/12/01 02:39:48, lawrencewu wrote: > Do we actually need this? Don't think so -- removed. https://codereview.chromium.org/2546653002/diff/1/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2546653002/diff/1/base/metrics/field_trial.h#... base/metrics/field_trial.h:599: static void AddTrialToAllocatorWhileLocked(FieldTrial* field_trial); On 2016/12/01 02:39:48, lawrencewu wrote: > Let's rename this back to AddToAllocatorWhileLocked. We can refactor this in > another CL. Done.
Description was changed from ========== Store and retrieve features from shared memory BUG=661224 ========== to ========== Store and retrieve features from shared memory If using shared memory, stores base::Features in the field trial allocator (under a different type defined in FeatureList) when spawning the first new process. When a child process is created, it will first try to get initialized from shared memory. If that doesn't work, it'll fallback to the command line. BUG=661224 ==========
lawrencewu@chromium.org changed reviewers: + asvitkine@chromium.org
Description was changed from ========== Store and retrieve features from shared memory If using shared memory, stores base::Features in the field trial allocator (under a different type defined in FeatureList) when spawning the first new process. When a child process is created, it will first try to get initialized from shared memory. If that doesn't work, it'll fallback to the command line. BUG=661224 ========== to ========== Store and retrieve features from shared memory If using shared memory, stores base::Features in the field trial allocator (under a different type defined in FeatureList) when spawning the first new process. When a child process is created, it will first try to get initialized from shared memory. If that doesn't work, it'll fallback to the command line. BUG=661224 ==========
lawrencewu@chromium.org changed reviewers: + bcwhite@chromium.org
https://codereview.chromium.org/2546653002/diff/20001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2546653002/diff/20001/base/feature_list.cc#ne... base/feature_list.cc:33: struct FeatureEntry { Write a docstring for this struct. https://codereview.chromium.org/2546653002/diff/20001/base/feature_list.cc#ne... base/feature_list.cc:89: void FeatureList::InitializeFromSharedMemory( write a test that stores and retrieves features from shared memory https://codereview.chromium.org/2546653002/diff/20001/base/feature_list.cc#ne... base/feature_list.cc:180: // should we add a ref to each feature? remove this comment
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
address self-comments https://codereview.chromium.org/2546653002/diff/20001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2546653002/diff/20001/base/feature_list.cc#ne... base/feature_list.cc:33: struct FeatureEntry { On 2016/12/01 18:50:38, lawrencewu wrote: > Write a docstring for this struct. Done. https://codereview.chromium.org/2546653002/diff/20001/base/feature_list.cc#ne... base/feature_list.cc:89: void FeatureList::InitializeFromSharedMemory( On 2016/12/01 18:50:38, lawrencewu wrote: > write a test that stores and retrieves features from shared memory Done. https://codereview.chromium.org/2546653002/diff/20001/base/feature_list.cc#ne... base/feature_list.cc:180: // should we add a ref to each feature? On 2016/12/01 18:50:38, lawrencewu wrote: > remove this comment Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
asvitkine@, bcwhite@: PTAL, thanks.
Some initial style comments. Will take a look more in a bit. https://codereview.chromium.org/2546653002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2546653002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:186: void AddFeatureAndFieldTrialFlags(CommandLine* cmd_line, Nit: Non-const params should be last. https://codereview.chromium.org/2546653002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:760: std::unique_ptr<FeatureList>& feature_list) { Don't pass non const refs. You can pass as a FeatureList* https://codereview.chromium.org/2546653002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:762: if (!kUseSharedMemoryForFieldTrials || !global_->field_trial_allocator_.get()) Nit: {}'s
https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:40: // Specifies whether a feature override enables or disables the future. Same future -> feature https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:44: // Size of the pickled structure, NOT the total size of this entry. Nit: Why not name it pickle_size then? https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:47: bool GetFeatureAndTrialName(StringPiece* feature_name, Add a comment and document that it's only valid to call this on a FeatureEntry that's in shared memory. https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:60: return true; Seems you can just simplify this to not have the if since you return true below anyway. https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:111: entry->GetFeatureAndTrialName(&feature_name, &trial_name); Check return value? https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:174: return; Seems like a bad thing to silently fail on. https://codereview.chromium.org/2546653002/diff/60001/base/feature_list_unitt... File base/feature_list_unittest.cc (right): https://codereview.chromium.org/2546653002/diff/60001/base/feature_list_unitt... base/feature_list_unittest.cc:472: TEST_F(FeatureListTest, StoreAndRetrieveFeaturesFromSharedMemory) { Can you add testing for OVERRIDE_USE_DEFAULT state? This would test to make sure the field trial gets associated (and would be activated if the feature is queried) without forcing its state one way or another.
https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:40: // Specifies whether a feature override enables or disables the future. Same future => feature https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:59: if (!pickle_iter.ReadStringPiece(trial_name)) Just remove the if() and keep the comment as to why you're ignoring the return code from pickle_iter. https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.h File base/feature_list.h (right): https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.h#new... base/feature_list.h:99: void InitializeFromSharedMemory(SharedPersistentMemoryAllocator* allocator); Better to accept the base PersistentMemoryAllocator class unless you're actually going to access methods specific to the management of shared memory. Here and below.
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Address comments. https://codereview.chromium.org/2546653002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2546653002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:186: void AddFeatureAndFieldTrialFlags(CommandLine* cmd_line, On 2016/12/01 19:39:22, Alexei Svitkine (slow) wrote: > Nit: Non-const params should be last. Done. https://codereview.chromium.org/2546653002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:760: std::unique_ptr<FeatureList>& feature_list) { On 2016/12/01 19:39:22, Alexei Svitkine (slow) wrote: > Don't pass non const refs. > > You can pass as a FeatureList* Done. https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:40: // Specifies whether a feature override enables or disables the future. Same On 2016/12/01 20:05:52, Alexei Svitkine (slow) wrote: > future -> feature Done. https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:44: // Size of the pickled structure, NOT the total size of this entry. On 2016/12/01 20:05:52, Alexei Svitkine (slow) wrote: > Nit: Why not name it pickle_size then? Done. https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:47: bool GetFeatureAndTrialName(StringPiece* feature_name, On 2016/12/01 20:05:52, Alexei Svitkine (slow) wrote: > Add a comment and document that it's only valid to call this on a FeatureEntry > that's in shared memory. Done. https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:59: if (!pickle_iter.ReadStringPiece(trial_name)) On 2016/12/01 20:15:15, bcwhite wrote: > Just remove the if() and keep the comment as to why you're ignoring the return > code from pickle_iter. The function has warn_unused_result so I think I have to do something with the return value. https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:60: return true; On 2016/12/01 20:05:52, Alexei Svitkine (slow) wrote: > Seems you can just simplify this to not have the if since you return true below > anyway. The function has warn_unused_result so I think I have to do something with the return value. https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:111: entry->GetFeatureAndTrialName(&feature_name, &trial_name); On 2016/12/01 20:05:52, Alexei Svitkine (slow) wrote: > Check return value? Not sure if we should return or continue on failure. I put a continue. https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:174: return; On 2016/12/01 20:05:52, Alexei Svitkine (slow) wrote: > Seems like a bad thing to silently fail on. We do the same thing in field_trial.cc. Maybe TerminateOutOfMemory here and CHECK(false) for NACL? https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.h File base/feature_list.h (right): https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.h#new... base/feature_list.h:99: void InitializeFromSharedMemory(SharedPersistentMemoryAllocator* allocator); On 2016/12/01 20:15:15, bcwhite wrote: > Better to accept the base PersistentMemoryAllocator class unless you're actually > going to access methods specific to the management of shared memory. Here and > below. Done. https://codereview.chromium.org/2546653002/diff/60001/base/feature_list_unitt... File base/feature_list_unittest.cc (right): https://codereview.chromium.org/2546653002/diff/60001/base/feature_list_unitt... base/feature_list_unittest.cc:472: TEST_F(FeatureListTest, StoreAndRetrieveFeaturesFromSharedMemory) { On 2016/12/01 20:05:52, Alexei Svitkine wrote: > Can you add testing for OVERRIDE_USE_DEFAULT state? > > This would test to make sure the field trial gets associated (and would be > activated if the feature is queried) without forcing its state one way or > another. Done.
https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2546653002/diff/60001/base/feature_list.cc#ne... base/feature_list.cc:59: if (!pickle_iter.ReadStringPiece(trial_name)) On 2016/12/02 19:24:46, lawrencewu wrote: > On 2016/12/01 20:15:15, bcwhite wrote: > > Just remove the if() and keep the comment as to why you're ignoring the return > > code from pickle_iter. > > The function has warn_unused_result so I think I have to do something with the > return value. // Need to return true because... so just ignore the return value. auto sink = pickle_iter... If that isn't sufficient, you can add another line: (void)sink;
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2546653002/diff/160001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2546653002/diff/160001/base/metrics/field_tri... base/metrics/field_trial.cc:895: return AddFeatureAndFieldTrialFlags(enable_features_switch, The function return value is void - so don't return the result here since it's confusing - and some compilers won't like that as well. https://codereview.chromium.org/2546653002/diff/160001/base/metrics/field_tri... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2546653002/diff/160001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1161: std::unique_ptr<FeatureList> feature_list(new FeatureList); This doesn't actually register the instance. (It doesn't work same way as FieldTrialList.) Can you use ScopedFeatureList here?
Address comments. https://codereview.chromium.org/2546653002/diff/160001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2546653002/diff/160001/base/metrics/field_tri... base/metrics/field_trial.cc:895: return AddFeatureAndFieldTrialFlags(enable_features_switch, On 2016/12/02 20:14:28, Alexei Svitkine wrote: > The function return value is void - so don't return the result here since it's > confusing - and some compilers won't like that as well. Done. https://codereview.chromium.org/2546653002/diff/160001/base/metrics/field_tri... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2546653002/diff/160001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1161: std::unique_ptr<FeatureList> feature_list(new FeatureList); On 2016/12/02 20:14:28, Alexei Svitkine wrote: > This doesn't actually register the instance. (It doesn't work same way as > FieldTrialList.) > > Can you use ScopedFeatureList here? Done. This also turned out to be the cause of those iossim crashes (the singleton was null).
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
lawrencewu@chromium.org changed reviewers: + piman@chromium.org
piman@: mind taking a look at content?
First time I've heard about this new feature - can you describe how the shared memory is used, and how it's safe (secure) to use across processes? I found a design doc (https://docs.google.com/document/d/1CTFOpOpbqnOTzo_gCuSyqDur0AeY2X_LFayylN_oH...) but I didn't see explained how the shared memory is used. In particular it talks about dynamically modifying the data in memory and I'm not clear how that is done (I commented in that doc). Also I'm not clear how different renderers are isolated from each other, so that a compromized one cannot modify the data for other ones. Are you using separate SHM segments for each one? Did you have security folks look over this new feature?
On 2016/12/02 21:20:36, piman wrote: > First time I've heard about this new feature - can you describe how the shared > memory is used, and how it's safe (secure) to use across processes? I found a > design doc > (https://docs.google.com/document/d/1CTFOpOpbqnOTzo_gCuSyqDur0AeY2X_LFayylN_oH...) > but I didn't see explained how the shared memory is used. In particular it talks > about dynamically modifying the data in memory and I'm not clear how that is > done (I commented in that doc). Also I'm not clear how different renderers are > isolated from each other, so that a compromized one cannot modify the data for > other ones. Are you using separate SHM segments for each one? > > Did you have security folks look over this new feature? The handle is passed read-only to the renderers, so even if they are compromised they won't be able to modify anything in shared memory. The same shm segment is being passed to the child processes. This is the same mechanism used by Mojo. Security has been brought up already. You can take a look at https://docs.google.com/document/d/1CTFOpOpbqnOTzo_gCuSyqDur0AeY2X_LFayylN_oH... for more context.
On 2016/12/02 21:28:42, lawrencewu wrote: > On 2016/12/02 21:20:36, piman wrote: > > First time I've heard about this new feature - can you describe how the shared > > memory is used, and how it's safe (secure) to use across processes? I found a > > design doc > > > (https://docs.google.com/document/d/1CTFOpOpbqnOTzo_gCuSyqDur0AeY2X_LFayylN_oH...) > > but I didn't see explained how the shared memory is used. In particular it > talks > > about dynamically modifying the data in memory and I'm not clear how that is > > done (I commented in that doc). Also I'm not clear how different renderers are > > isolated from each other, so that a compromized one cannot modify the data for > > other ones. Are you using separate SHM segments for each one? > > > > Did you have security folks look over this new feature? > > The handle is passed read-only to the renderers, so even if they are compromised > they won't be able to modify anything in shared memory. The same shm segment > is being passed to the child processes. This is the same mechanism used by Mojo. > > Security has been brought up already. You can take a look at > https://docs.google.com/document/d/1CTFOpOpbqnOTzo_gCuSyqDur0AeY2X_LFayylN_oH... > for more context. That's for Windows. I'm currently working on the Mac implementation for this, and will update the design doc accordingly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2546653002/diff/260001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2546653002/diff/260001/base/feature_list.cc#n... base/feature_list.cc:42: uint32_t override_state; Do you intend to modify this field (or other contents in shared memory) after allocator->MakeIterable(ref) is called? If so, this needs to be atomic, or some form of higher level mutual exclusion needs to exist, otherwise the code in InitializeFromSharedMemory is unsafe. https://codereview.chromium.org/2546653002/diff/260001/base/feature_list.cc#n... base/feature_list.cc:51: char* src = reinterpret_cast<char*>(const_cast<FeatureEntry*>(this)) + nit: why const_cast? The memory is read-only, so we should probably keep everything const to be explicit. So src should be a const char* etc. https://codereview.chromium.org/2546653002/diff/260001/base/feature_list.cc#n... base/feature_list.cc:62: (void)sink; nit: ALLOW_UNUSED_LOCAL(sink)
Address comments. https://codereview.chromium.org/2546653002/diff/260001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2546653002/diff/260001/base/feature_list.cc#n... base/feature_list.cc:42: uint32_t override_state; On 2016/12/02 22:29:26, piman wrote: > Do you intend to modify this field (or other contents in shared memory) after > allocator->MakeIterable(ref) is called? If so, this needs to be atomic, or some > form of higher level mutual exclusion needs to exist, otherwise the code in > InitializeFromSharedMemory is unsafe. I'm pretty sure we don't need to modify any fields after MakeIterable is called. cc: asvitkine, can you confirm this? https://codereview.chromium.org/2546653002/diff/260001/base/feature_list.cc#n... base/feature_list.cc:51: char* src = reinterpret_cast<char*>(const_cast<FeatureEntry*>(this)) + On 2016/12/02 22:29:26, piman wrote: > nit: why const_cast? The memory is read-only, so we should probably keep > everything const to be explicit. So src should be a const char* etc. Done. https://codereview.chromium.org/2546653002/diff/260001/base/feature_list.cc#n... base/feature_list.cc:62: (void)sink; On 2016/12/02 22:29:26, piman wrote: > nit: ALLOW_UNUSED_LOCAL(sink) Done.
It can be modified post making it iterable but the design is such that there is no problem if a reader sees an old while. Basically the field can go from 0 to 1 only. If a child process sees a stale 0 value, it just means that it might send an extra IPC to browser process if the trial is gets activated in the child that will essentially be noop. On Dec 2, 2016 5:51 PM, <lawrencewu@chromium.org> wrote: > Address comments. > > > https://codereview.chromium.org/2546653002/diff/260001/ > base/feature_list.cc > File base/feature_list.cc (right): > > https://codereview.chromium.org/2546653002/diff/260001/ > base/feature_list.cc#newcode42 > base/feature_list.cc:42: uint32_t override_state; > On 2016/12/02 22:29:26, piman wrote: > > Do you intend to modify this field (or other contents in shared > memory) after > > allocator->MakeIterable(ref) is called? If so, this needs to be > atomic, or some > > form of higher level mutual exclusion needs to exist, otherwise the > code in > > InitializeFromSharedMemory is unsafe. > > I'm pretty sure we don't need to modify any fields after MakeIterable is > called. > cc: asvitkine, can you confirm this? > > https://codereview.chromium.org/2546653002/diff/260001/ > base/feature_list.cc#newcode51 > base/feature_list.cc:51: char* src = > reinterpret_cast<char*>(const_cast<FeatureEntry*>(this)) + > On 2016/12/02 22:29:26, piman wrote: > > nit: why const_cast? The memory is read-only, so we should probably > keep > > everything const to be explicit. So src should be a const char* etc. > > Done. > > https://codereview.chromium.org/2546653002/diff/260001/ > base/feature_list.cc#newcode62 > base/feature_list.cc:62: (void)sink; > On 2016/12/02 22:29:26, piman wrote: > > nit: ALLOW_UNUSED_LOCAL(sink) > > Done. > > https://codereview.chromium.org/2546653002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM with the comment below. https://codereview.chromium.org/2546653002/diff/260001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2546653002/diff/260001/base/feature_list.cc#n... base/feature_list.cc:42: uint32_t override_state; On 2016/12/02 22:51:48, lawrencewu wrote: > On 2016/12/02 22:29:26, piman wrote: > > Do you intend to modify this field (or other contents in shared memory) after > > allocator->MakeIterable(ref) is called? If so, this needs to be atomic, or > some > > form of higher level mutual exclusion needs to exist, otherwise the code in > > InitializeFromSharedMemory is unsafe. > > I'm pretty sure we don't need to modify any fields after MakeIterable is called. > cc: asvitkine, can you confirm this? Either way, please add comments somewhere as to the safety protocol for accessing this shared data. Assuming you never change this, it is safe because MakeIterable is a "release" atomic operation, and PersistentMemoryAllocator::Iterator::GetNextOfType is an "acquire" operation (so memory writes before MakeIterable happen-before memory reads after GetNextOfType). Also please mention somewhere that the only process that has write access to this is the (trusted) browser process.
On Fri, Dec 2, 2016 at 3:01 PM, Alexei Svitkine <asvitkine@chromium.org> wrote: > It can be modified post making it iterable but the design is such that > there is no problem if a reader sees an old while. Basically the field can > go from 0 to 1 only. If a child process sees a stale 0 value, it just means > that it might send > an extra IPC to browser process if the trial is gets activated in the > child that will essentially be noop. > This is undefined behavior though. The field is not atomic, it's not legal to read it without a happens-before relationship. In particular you can see values different than 0 or 1. > On Dec 2, 2016 5:51 PM, <lawrencewu@chromium.org> wrote: > >> Address comments. >> >> >> https://codereview.chromium.org/2546653002/diff/260001/base/ >> feature_list.cc >> File base/feature_list.cc (right): >> >> https://codereview.chromium.org/2546653002/diff/260001/base/ >> feature_list.cc#newcode42 >> base/feature_list.cc:42: uint32_t override_state; >> On 2016/12/02 22:29:26, piman wrote: >> > Do you intend to modify this field (or other contents in shared >> memory) after >> > allocator->MakeIterable(ref) is called? If so, this needs to be >> atomic, or some >> > form of higher level mutual exclusion needs to exist, otherwise the >> code in >> > InitializeFromSharedMemory is unsafe. >> >> I'm pretty sure we don't need to modify any fields after MakeIterable is >> called. >> cc: asvitkine, can you confirm this? >> >> https://codereview.chromium.org/2546653002/diff/260001/base/ >> feature_list.cc#newcode51 >> base/feature_list.cc:51: char* src = >> reinterpret_cast<char*>(const_cast<FeatureEntry*>(this)) + >> On 2016/12/02 22:29:26, piman wrote: >> > nit: why const_cast? The memory is read-only, so we should probably >> keep >> > everything const to be explicit. So src should be a const char* etc. >> >> Done. >> >> https://codereview.chromium.org/2546653002/diff/260001/base/ >> feature_list.cc#newcode62 >> base/feature_list.cc:62: (void)sink; >> On 2016/12/02 22:29:26, piman wrote: >> > nit: ALLOW_UNUSED_LOCAL(sink) >> >> Done. >> >> https://codereview.chromium.org/2546653002/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I believe we just check for non 0, so even then its fine. On Dec 2, 2016 6:05 PM, "Antoine Labour" <piman@chromium.org> wrote: > > > On Fri, Dec 2, 2016 at 3:01 PM, Alexei Svitkine <asvitkine@chromium.org> > wrote: > >> It can be modified post making it iterable but the design is such that >> there is no problem if a reader sees an old while. Basically the field can >> go from 0 to 1 only. If a child process sees a stale 0 value, it just means >> that it might send >> an extra IPC to browser process if the trial is gets activated in the >> child that will essentially be noop. >> > > This is undefined behavior though. The field is not atomic, it's not legal > to read it without a happens-before relationship. In particular you can see > values different than 0 or 1. > > >> On Dec 2, 2016 5:51 PM, <lawrencewu@chromium.org> wrote: >> >>> Address comments. >>> >>> >>> https://codereview.chromium.org/2546653002/diff/260001/base/ >>> feature_list.cc >>> File base/feature_list.cc (right): >>> >>> https://codereview.chromium.org/2546653002/diff/260001/base/ >>> feature_list.cc#newcode42 >>> base/feature_list.cc:42: uint32_t override_state; >>> On 2016/12/02 22:29:26, piman wrote: >>> > Do you intend to modify this field (or other contents in shared >>> memory) after >>> > allocator->MakeIterable(ref) is called? If so, this needs to be >>> atomic, or some >>> > form of higher level mutual exclusion needs to exist, otherwise the >>> code in >>> > InitializeFromSharedMemory is unsafe. >>> >>> I'm pretty sure we don't need to modify any fields after MakeIterable is >>> called. >>> cc: asvitkine, can you confirm this? >>> >>> https://codereview.chromium.org/2546653002/diff/260001/base/ >>> feature_list.cc#newcode51 >>> base/feature_list.cc:51: char* src = >>> reinterpret_cast<char*>(const_cast<FeatureEntry*>(this)) + >>> On 2016/12/02 22:29:26, piman wrote: >>> > nit: why const_cast? The memory is read-only, so we should probably >>> keep >>> > everything const to be explicit. So src should be a const char* etc. >>> >>> Done. >>> >>> https://codereview.chromium.org/2546653002/diff/260001/base/ >>> feature_list.cc#newcode62 >>> base/feature_list.cc:62: (void)sink; >>> On 2016/12/02 22:29:26, piman wrote: >>> > nit: ALLOW_UNUSED_LOCAL(sink) >>> >>> Done. >>> >>> https://codereview.chromium.org/2546653002/ >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Dec 2, 2016 at 3:26 PM, Alexei Svitkine <asvitkine@chromium.org> wrote: > I believe we just check for non 0, so even then its fine. > Please, no, it's not - see e.g. https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-coul... Antoine > > On Dec 2, 2016 6:05 PM, "Antoine Labour" <piman@chromium.org> wrote: > >> >> >> On Fri, Dec 2, 2016 at 3:01 PM, Alexei Svitkine <asvitkine@chromium.org> >> wrote: >> >>> It can be modified post making it iterable but the design is such that >>> there is no problem if a reader sees an old while. Basically the field can >>> go from 0 to 1 only. If a child process sees a stale 0 value, it just means >>> that it might send >>> an extra IPC to browser process if the trial is gets activated in the >>> child that will essentially be noop. >>> >> >> This is undefined behavior though. The field is not atomic, it's not >> legal to read it without a happens-before relationship. In particular you >> can see values different than 0 or 1. >> >> >>> On Dec 2, 2016 5:51 PM, <lawrencewu@chromium.org> wrote: >>> >>>> Address comments. >>>> >>>> >>>> https://codereview.chromium.org/2546653002/diff/260001/base/ >>>> feature_list.cc >>>> File base/feature_list.cc (right): >>>> >>>> https://codereview.chromium.org/2546653002/diff/260001/base/ >>>> feature_list.cc#newcode42 >>>> base/feature_list.cc:42: uint32_t override_state; >>>> On 2016/12/02 22:29:26, piman wrote: >>>> > Do you intend to modify this field (or other contents in shared >>>> memory) after >>>> > allocator->MakeIterable(ref) is called? If so, this needs to be >>>> atomic, or some >>>> > form of higher level mutual exclusion needs to exist, otherwise the >>>> code in >>>> > InitializeFromSharedMemory is unsafe. >>>> >>>> I'm pretty sure we don't need to modify any fields after MakeIterable is >>>> called. >>>> cc: asvitkine, can you confirm this? >>>> >>>> https://codereview.chromium.org/2546653002/diff/260001/base/ >>>> feature_list.cc#newcode51 >>>> base/feature_list.cc:51: char* src = >>>> reinterpret_cast<char*>(const_cast<FeatureEntry*>(this)) + >>>> On 2016/12/02 22:29:26, piman wrote: >>>> > nit: why const_cast? The memory is read-only, so we should probably >>>> keep >>>> > everything const to be explicit. So src should be a const char* etc. >>>> >>>> Done. >>>> >>>> https://codereview.chromium.org/2546653002/diff/260001/base/ >>>> feature_list.cc#newcode62 >>>> base/feature_list.cc:62: (void)sink; >>>> On 2016/12/02 22:29:26, piman wrote: >>>> > nit: ALLOW_UNUSED_LOCAL(sink) >>>> >>>> Done. >>>> >>>> https://codereview.chromium.org/2546653002/ >>>> >>> >> -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
All right, we can address that in a separate CL since this one is not changing that part. On Dec 2, 2016 6:58 PM, "Antoine Labour" <piman@chromium.org> wrote: > > > On Fri, Dec 2, 2016 at 3:26 PM, Alexei Svitkine <asvitkine@chromium.org> > wrote: > >> I believe we just check for non 0, so even then its fine. >> > > Please, no, it's not - see e.g. https://software.intel.com/en- > us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong > > Antoine > > >> >> On Dec 2, 2016 6:05 PM, "Antoine Labour" <piman@chromium.org> wrote: >> >>> >>> >>> On Fri, Dec 2, 2016 at 3:01 PM, Alexei Svitkine <asvitkine@chromium.org> >>> wrote: >>> >>>> It can be modified post making it iterable but the design is such that >>>> there is no problem if a reader sees an old while. Basically the field can >>>> go from 0 to 1 only. If a child process sees a stale 0 value, it just means >>>> that it might send >>>> an extra IPC to browser process if the trial is gets activated in the >>>> child that will essentially be noop. >>>> >>> >>> This is undefined behavior though. The field is not atomic, it's not >>> legal to read it without a happens-before relationship. In particular you >>> can see values different than 0 or 1. >>> >>> >>>> On Dec 2, 2016 5:51 PM, <lawrencewu@chromium.org> wrote: >>>> >>>>> Address comments. >>>>> >>>>> >>>>> https://codereview.chromium.org/2546653002/diff/260001/base/ >>>>> feature_list.cc >>>>> File base/feature_list.cc (right): >>>>> >>>>> https://codereview.chromium.org/2546653002/diff/260001/base/ >>>>> feature_list.cc#newcode42 >>>>> base/feature_list.cc:42: uint32_t override_state; >>>>> On 2016/12/02 22:29:26, piman wrote: >>>>> > Do you intend to modify this field (or other contents in shared >>>>> memory) after >>>>> > allocator->MakeIterable(ref) is called? If so, this needs to be >>>>> atomic, or some >>>>> > form of higher level mutual exclusion needs to exist, otherwise the >>>>> code in >>>>> > InitializeFromSharedMemory is unsafe. >>>>> >>>>> I'm pretty sure we don't need to modify any fields after MakeIterable >>>>> is >>>>> called. >>>>> cc: asvitkine, can you confirm this? >>>>> >>>>> https://codereview.chromium.org/2546653002/diff/260001/base/ >>>>> feature_list.cc#newcode51 >>>>> base/feature_list.cc:51: char* src = >>>>> reinterpret_cast<char*>(const_cast<FeatureEntry*>(this)) + >>>>> On 2016/12/02 22:29:26, piman wrote: >>>>> > nit: why const_cast? The memory is read-only, so we should probably >>>>> keep >>>>> > everything const to be explicit. So src should be a const char* etc. >>>>> >>>>> Done. >>>>> >>>>> https://codereview.chromium.org/2546653002/diff/260001/base/ >>>>> feature_list.cc#newcode62 >>>>> base/feature_list.cc:62: (void)sink; >>>>> On 2016/12/02 22:29:26, piman wrote: >>>>> > nit: ALLOW_UNUSED_LOCAL(sink) >>>>> >>>>> Done. >>>>> >>>>> https://codereview.chromium.org/2546653002/ >>>>> >>>> >>> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by lawrencewu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2546653002/#ps300001 (title: "add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1480950212755290,
"parent_rev": "d84df127d4ce7eba6370d850385bb6249f38b527", "commit_rev":
"6f43e87c220da73e4739d708a89100a9c7daad25"}
Message was sent while issue was closed.
Description was changed from ========== Store and retrieve features from shared memory If using shared memory, stores base::Features in the field trial allocator (under a different type defined in FeatureList) when spawning the first new process. When a child process is created, it will first try to get initialized from shared memory. If that doesn't work, it'll fallback to the command line. BUG=661224 ========== to ========== Store and retrieve features from shared memory If using shared memory, stores base::Features in the field trial allocator (under a different type defined in FeatureList) when spawning the first new process. When a child process is created, it will first try to get initialized from shared memory. If that doesn't work, it'll fallback to the command line. BUG=661224 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Store and retrieve features from shared memory If using shared memory, stores base::Features in the field trial allocator (under a different type defined in FeatureList) when spawning the first new process. When a child process is created, it will first try to get initialized from shared memory. If that doesn't work, it'll fallback to the command line. BUG=661224 ========== to ========== Store and retrieve features from shared memory If using shared memory, stores base::Features in the field trial allocator (under a different type defined in FeatureList) when spawning the first new process. When a child process is created, it will first try to get initialized from shared memory. If that doesn't work, it'll fallback to the command line. BUG=661224 Committed: https://crrev.com/5e03cd3432b1f7904a11a94766956e8871795d9f Cr-Commit-Position: refs/heads/master@{#436311} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/5e03cd3432b1f7904a11a94766956e8871795d9f Cr-Commit-Position: refs/heads/master@{#436311} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
