Use a std::string to store return value from FieldTrialList::FindFullName()

Use a std::string to store return value from FieldTrialList::FindFullName()
to prevent memory from being freed.

R=mef@chromium.org
BUG=413400
Committed: https://crrev.com/2d076284582761510b76429f1718797b6c776b83
Cr-Commit-Position: refs/heads/master@{#294660}

[Randy Smith (Not in Mondays), 2014-09-11 22:02:04]

Misha: Could you review this?  It's reasonably high priority; it's blocking
enabling SDCH on HTTPS (see linked issue and http://crbug.com/357769 for
context).

[mef, 2014-09-12 13:28:33]

I wonder if it is possible to detect this pattern programmatically (at compile
or run time) as I imagine it could happen elsewhere and corrupt memory in the
process. 

I would expect that destructor of std::string in the debug build should
invalidate the memory, but it doesn't seem to be happening...

https://codereview.chromium.org/566763002/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/566763002/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:1389: if (sdch_trial_group ==
kEnabledAllGroupName) {
I wonder if exact comparison is the right thing here. Could the trial groupname
be something like 'EnabledAll1234'? 

Would it make sense to just explicitly store return of
base::FieldTrialList::FindFullName(kSdchFieldTrialName) in the std::string
sdch_trial_group_name and make base::StringPiece sdch_trial_group based on it?

[Randy Smith (Not in Mondays), 2014-09-12 17:49:42]

Well, on Windows the memory is scribbled on as part of deallcoation.  More
invalidation than that would probably require virtual memory tricks.  But I
agree with you ASAN should catch it--I'm curious about where the conversation on
the bug goes. 

WDYTN?

https://codereview.chromium.org/566763002/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/566763002/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:1389: if (sdch_trial_group ==
kEnabledAllGroupName) {
On 2014/09/12 13:28:33, mef wrote:
> I wonder if exact comparison is the right thing here. Could the trial
groupname
> be something like 'EnabledAll1234'? 
> 
> Would it make sense to just explicitly store return of
> base::FieldTrialList::FindFullName(kSdchFieldTrialName) in the std::string
> sdch_trial_group_name and make base::StringPiece sdch_trial_group based on it?

Yeah ... I looked through the code and it looked like it would either be the
actual group name or a number, but I'm planning to try to merge this to M38, and
for that, it should be as close to the original code as possible.  Done.

[mef, 2014-09-12 17:54:03]

lgtm

[Randy Smith (Not in Mondays), 2014-09-12 17:55:54]

rdsmith@chromium.org changed reviewers:
+ sky@chromium.org

[Randy Smith (Not in Mondays), 2014-09-12 17:55:55]

Thanks!

Scott: Willing to stamp?

[sky, 2014-09-12 18:14:50]

LGTM

https://codereview.chromium.org/566763002/diff/20001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/566763002/diff/20001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:1387: std::string sdch_trial_group_string
=
How about a comment so this doesn't happen again.

[Randy Smith (Not in Mondays), 2014-09-12 19:35:55]

Thanks!

https://codereview.chromium.org/566763002/diff/20001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/566763002/diff/20001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:1387: std::string sdch_trial_group_string
=
On 2014/09/12 18:14:49, sky wrote:
> How about a comment so this doesn't happen again.

Done.

[Randy Smith (Not in Mondays), 2014-09-12 19:36:03]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-09-12 19:38:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/566763002/40001

[commit-bot: I haz the power, 2014-09-12 21:24:00]

Committed patchset #3 (id:40001) as 30f9b54f427103fbf25d8a06bf01fe368db37671

[commit-bot: I haz the power, 2014-09-12 21:28:41]

Patchset 3 (id:??) landed as
https://crrev.com/2d076284582761510b76429f1718797b6c776b83
Cr-Commit-Position: refs/heads/master@{#294660}

[sky, 2014-09-16 22:06:27]

LGTM