Fix deadlock issue with AllStatesToString

Fix deadlock issue with AllStatesToString

There was a deadlock happening when the thread calling AllStatesToString would grab the same lock twice. This fixes that by creating a version of GetState that's lockfree, GetStateWhileLocked, so AllStatesToString can call that instead.

BUG=660150
Committed: https://crrev.com/73c431498b20128b49e07540f6c8b4e81d20f500
Cr-Commit-Position: refs/heads/master@{#428844}

[lawrencewu, 2016-10-27 14:29:44]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 14:30:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 14:54:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 14:54:25]

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...)

[lawrencewu, 2016-10-27 20:49:39]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[lawrencewu, 2016-10-27 20:50:00]

fix tests and fix deadlock issue

[lawrencewu, 2016-10-27 20:50:10]

lawrencewu@chromium.org changed reviewers:
+ asvitkine@chromium.org

[commit-bot: I haz the power, 2016-10-27 20:50:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2016-10-27 20:54:23]

Please split the CL in two multiple parts - one that fixes the lock and the
other one that turns it on.

This way, if the one that turns it on needs to be reverted, we don't have
to reland the lock stuff.

On Thu, Oct 27, 2016 at 4:50 PM, <lawrencewu@chromium.org> wrote:

> https://codereview.chromium.org/2453933004/
>

-- 
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.

[lawrencewu, 2016-10-27 20:58:28]

On 2016/10/27 20:54:23, Alexei Svitkine (slow) wrote:
> Please split the CL in two multiple parts - one that fixes the lock and the
> other one that turns it on.
> 
> This way, if the one that turns it on needs to be reverted, we don't have
> to reland the lock stuff.
> 
> On Thu, Oct 27, 2016 at 4:50 PM, <mailto:lawrencewu@chromium.org> wrote:
> 
> > https://codereview.chromium.org/2453933004/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

Sure. I'm going to turn this one into the one that fixes the lock, then.

[lawrencewu, 2016-10-27 20:59:50]

Description was changed from

==========
Turn on flag for using shared memory for field trials on Windows

This CL switches on the ability to use shared memory in field trials.
For more context, see https://codereview.chromium.org/2412113002/ and
https://codereview.chromium.org/2365273004/.

BUG=131632
==========

to

==========
Fix deadlock issue with AllStatesToString

There was a deadlock happening when the thread calling AllStatesToString would
grab the same lock twice. This fixes that by creating a version of GetState
that's lockfree, GetStateWhileLocked, so AllStatesToString can call that
instead.

BUG=660150
==========

[commit-bot: I haz the power, 2016-10-27 21:18:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 21:18:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexei Svitkine (slow), 2016-10-27 21:40:43]

https://codereview.chromium.org/2453933004/diff/40001/base/metrics/field_tria...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2453933004/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.cc:372: FinalizeGroupChoice(true);
How about making the call that takes the param FinalizeGroupChoiceImpl() and
keeping FinalizeGroupChoice() take no param?

Then, you can make FinalizeGroupChoice() call FinalizeGroupChoiceImpl(false) so
you don't have to change all the callers.

[lawrencewu, 2016-10-28 13:41:36]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 13:41:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 14:07:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 14:07:31]

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...)

[lawrencewu, 2016-10-28 14:24:24]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 14:24:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lawrencewu, 2016-10-28 15:20:52]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 15:21:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 17:16:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 17:16:57]

Dry run: This issue passed the CQ dry run.

[lawrencewu, 2016-10-28 17:20:21]

Address comments. Also fix an edge case where global_ wasn't instantiated in
browsertests, so now CopyFieldTrialStateToFlags will fall back to adding
--force-field-trials flag, which will do nothing (which is the right thing).

https://codereview.chromium.org/2453933004/diff/40001/base/metrics/field_tria...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2453933004/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.cc:372: FinalizeGroupChoice(true);
On 2016/10/27 21:40:43, Alexei Svitkine (slow) wrote:
> How about making the call that takes the param FinalizeGroupChoiceImpl() and
> keeping FinalizeGroupChoice() take no param?
> 
> Then, you can make FinalizeGroupChoice() call FinalizeGroupChoiceImpl(false)
so
> you don't have to change all the callers.

Done.

[Alexei Svitkine (slow), 2016-10-28 18:57:34]

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
base/metrics/field_trial.cc:688: if (global_ &&
global_->readonly_allocator_handle_)
There's already an early return on line 684.

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
base/metrics/field_trial.cc:705: if (!global_ ||
!global_->readonly_allocator_handle_) {
Can we instead fix the browser test to have a FieldTrialList instance instead of
adding this check?

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
File base/metrics/field_trial_unittest.cc (right):

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
base/metrics/field_trial_unittest.cc:1149:
EXPECT_TRUE(cmd_line.HasSwitch("field-trial-handle") ||
Please reference the flag via it's switches:: constant.

[lawrencewu, 2016-10-30 21:21:06]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-30 21:21:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-30 21:31:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-30 21:31:33]

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...)

[lawrencewu, 2016-10-31 13:45:07]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 13:45:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 14:38:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 14:38:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lawrencewu, 2016-10-31 15:37:16]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 15:37:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 16:05:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 16:05:34]

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...)

[lawrencewu, 2016-10-31 17:51:36]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 17:51:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 18:32:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 18:32:45]

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...)

[lawrencewu, 2016-10-31 18:59:25]

Address comments. The failure on ios-simulator looks unrelated, probably flaky.

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
base/metrics/field_trial.cc:688: if (global_ &&
global_->readonly_allocator_handle_)
On 2016/10/28 18:57:34, Alexei Svitkine (slow) wrote:
> There's already an early return on line 684.

Fixed.

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
base/metrics/field_trial.cc:705: if (!global_ ||
!global_->readonly_allocator_handle_) {
On 2016/10/28 18:57:34, Alexei Svitkine (slow) wrote:
> Can we instead fix the browser test to have a FieldTrialList instance instead
of
> adding this check?

As we discussed, I moved the check for global to the top and wrote a comment
about this.

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
File base/metrics/field_trial_unittest.cc (right):

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
base/metrics/field_trial_unittest.cc:1149:
EXPECT_TRUE(cmd_line.HasSwitch("field-trial-handle") ||
On 2016/10/28 18:57:34, Alexei Svitkine (slow) wrote:
> Please reference the flag via it's switches:: constant.

The flag is in content_switches.cc so I don't think we can use it from base.

[Alexei Svitkine (slow), 2016-10-31 19:01:40]

lgtm

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
File base/metrics/field_trial_unittest.cc (right):

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
base/metrics/field_trial_unittest.cc:1149:
EXPECT_TRUE(cmd_line.HasSwitch("field-trial-handle") ||
On 2016/10/31 18:59:25, lawrencewu wrote:
> On 2016/10/28 18:57:34, Alexei Svitkine (slow) wrote:
> > Please reference the flag via it's switches:: constant.
> 
> The flag is in content_switches.cc so I don't think we can use it from base.

Ah, I see - you're specifying it manually on line 1146. I suggest making a char
constant within the test for it and re-using it in both places. You can also
name it something else so it's not easily confused with the real flag - e.g.
test-field-trial-handle.

[Alexei Svitkine (slow), 2016-10-31 19:03:58]

https://codereview.chromium.org/2453933004/diff/210001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2453933004/diff/210001/base/metrics/field_tri...
base/metrics/field_trial.cc:703: // guarantee this.
Actually, I would make this more explicit.

e.g.

Ideally, having the global would be guaranteed. However, content browser tests
currently don't create a FieldTrialList because they don't run
ChromeBrowserMainParts code where it's done for Chrome.

[lawrencewu, 2016-10-31 19:20:27]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 19:21:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lawrencewu, 2016-10-31 21:09:09]

Address comments.

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
File base/metrics/field_trial_unittest.cc (right):

https://codereview.chromium.org/2453933004/diff/140001/base/metrics/field_tri...
base/metrics/field_trial_unittest.cc:1149:
EXPECT_TRUE(cmd_line.HasSwitch("field-trial-handle") ||
On 2016/10/31 19:01:40, Alexei Svitkine (slow) wrote:
> On 2016/10/31 18:59:25, lawrencewu wrote:
> > On 2016/10/28 18:57:34, Alexei Svitkine (slow) wrote:
> > > Please reference the flag via it's switches:: constant.
> > 
> > The flag is in content_switches.cc so I don't think we can use it from base.
> 
> Ah, I see - you're specifying it manually on line 1146. I suggest making a
char
> constant within the test for it and re-using it in both places. You can also
> name it something else so it's not easily confused with the real flag - e.g.
> test-field-trial-handle.

Done.

https://codereview.chromium.org/2453933004/diff/210001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2453933004/diff/210001/base/metrics/field_tri...
base/metrics/field_trial.cc:703: // guarantee this.
On 2016/10/31 19:03:58, Alexei Svitkine (slow) wrote:
> Actually, I would make this more explicit.
> 
> e.g.
> 
> Ideally, having the global would be guaranteed. However, content browser tests
> currently don't create a FieldTrialList because they don't run
> ChromeBrowserMainParts code where it's done for Chrome.

Done.

[Alexei Svitkine (slow), 2016-10-31 21:13:45]

lgtm

https://codereview.chromium.org/2453933004/diff/230001/base/metrics/field_tri...
File base/metrics/field_trial_unittest.cc (right):

https://codereview.chromium.org/2453933004/diff/230001/base/metrics/field_tri...
base/metrics/field_trial_unittest.cc:1145: const char* field_trial_handle =
"test-field-trial-handle";
Nit: const char kFieldTrialHandleSwitch[] =

[lawrencewu, 2016-10-31 21:15:41]

The CQ bit was checked by lawrencewu@chromium.org

[lawrencewu, 2016-10-31 21:15:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2453933004/#ps250001
(title: "fix nit")

[commit-bot: I haz the power, 2016-10-31 21:15:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 22:33:30]

Description was changed from

==========
Fix deadlock issue with AllStatesToString

There was a deadlock happening when the thread calling AllStatesToString would
grab the same lock twice. This fixes that by creating a version of GetState
that's lockfree, GetStateWhileLocked, so AllStatesToString can call that
instead.

BUG=660150
==========

to

==========
Fix deadlock issue with AllStatesToString

There was a deadlock happening when the thread calling AllStatesToString would
grab the same lock twice. This fixes that by creating a version of GetState
that's lockfree, GetStateWhileLocked, so AllStatesToString can call that
instead.

BUG=660150
==========

[commit-bot: I haz the power, 2016-10-31 22:33:31]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2016-10-31 22:35:12]

Description was changed from

==========
Fix deadlock issue with AllStatesToString

There was a deadlock happening when the thread calling AllStatesToString would
grab the same lock twice. This fixes that by creating a version of GetState
that's lockfree, GetStateWhileLocked, so AllStatesToString can call that
instead.

BUG=660150
==========

to

==========
Fix deadlock issue with AllStatesToString

There was a deadlock happening when the thread calling AllStatesToString would
grab the same lock twice. This fixes that by creating a version of GetState
that's lockfree, GetStateWhileLocked, so AllStatesToString can call that
instead.

BUG=660150

Committed: https://crrev.com/73c431498b20128b49e07540f6c8b4e81d20f500
Cr-Commit-Position: refs/heads/master@{#428844}
==========

[commit-bot: I haz the power, 2016-10-31 22:35:13]

Patchset 14 (id:??) landed as
https://crrev.com/73c431498b20128b49e07540f6c8b4e81d20f500
Cr-Commit-Position: refs/heads/master@{#428844}