Fix the failed case ValueStoreTest.DotsInKeyNamesWithDicts for Android

Fix the failed case ValueStoreTest.DotsInKeyNamesWithDicts for Android

Json parser uses a stack-based input copy string and the parsed result
'Value' references the internal pointer of the input copy string. To
make the references valid, 'Value' has one member std::string to swap
its content with the input copy string. On other platforms, the member
std::string can get the internal pointer of the input copy string.
However, on Android, std::string just swaps the content of two strings
instead of swapping their internal pointers. This makes the pointer
reference of value invalid.

Fix this by forcing options |= JSON_DETACHABLE_CHILDREN for Android so
disable this optimization.

BUG=139434
TEST=run_tests.py -s unit_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152944

[yongsheng, 2012-08-15 09:48:57]

This issue is very tricky.
The main cause is the different behavior of std::string::swap on Android.
On other platforms, str1.swap(str2) means the internal ptr of str1 is swapped
with the internal ptr of str2.
str1: { c_str: p1("a")}, str2: {c_str: p2("b")}
-->
str1: { c_str: p2("b")}, str2: {c_str: p1("a")}

On Android, str1.swap(str2) means the content of ptr of str1 is copied to the
content of internal ptr of str2.
str1: { c_str: p1("a")}, str2: {c_str: p2("b")}
-->
str1: { c_str: p1("b")}, str2: {c_str: p2("a")}
This is because android NDK has different implementation for  std::string::swap.

Actually, from the developer document of stl, it doesn't guarantee the first
behavior:
"Swaps the contents of the string with those of string object str, such that
after the call to this member function, the contents of this string are those
which were in str before the call, and the contents of str are those which were
in this string."
So I think the usage of swapping strings in json parser may get issues.

I only make changes for Android for I don't want to affect other platforms.

Owners, please help review this patch. Thanks very much.

[Robert Sesek, 2012-08-15 16:48:16]

Thanks for investigating this -- it does look tricky.

I wonder if, instead of doing this, it's just easier to turn off the hidden root
optimizations by forcing options |= JSON_DETACHABLE_CHILDREN on Android. When
writing this new parser, I measured that using scoped_ptr<T> is expensive at
this level, due to the extra pointer indirection. Also, if the Android stl is
performing a copy in std::string::swap, there's no appreciable advantage of
using hidden roots.

[yongsheng, 2012-08-16 01:22:32]

On 2012/08/15 16:48:16, rsesek wrote:
> Thanks for investigating this -- it does look tricky.
> 
> I wonder if, instead of doing this, it's just easier to turn off the hidden
root
> optimizations by forcing options |= JSON_DETACHABLE_CHILDREN on Android. When
> writing this new parser, I measured that using scoped_ptr<T> is expensive at
> this level, due to the extra pointer indirection. Also, if the Android stl is
> performing a copy in std::string::swap, there's no appreciable advantage of
> using hidden roots.
reasonable for the performance concern. I'll change this in the json parser.

[yongsheng, 2012-08-16 03:01:20]

The ValueStoreTest related tests are all passed.
The 3 failures in android_test are not related to this issue:
'VariationsHelperTest.DisableAfterInitialization',
'VariationsHelperTest.AssociateGoogleVariationID',
'VariationsHelperTest.NoAssociation'

[yongsheng, 2012-08-16 07:06:27]

@brettw, since you are the owner of 'base/', could you please help review it?
thanks.

[brettw, 2012-08-19 22:40:17]

This optimization seems scary and I wonder how necessary it is. LGTM for this
patch, though.

[yongsheng, 2012-08-20 01:22:25]

On 2012/08/19 22:40:17, brettw wrote:
> This optimization seems scary and I wonder how necessary it is. LGTM for this
> patch, though.
Thanks for review. yes, because it depends on the behavior of std::string on
different platforms.
This patch is to disable this optimization for Android.
@nileshagrawal1 and @rsesek, any other comments?

[nilesh, 2012-08-20 16:26:16]

LGTM

[yongsheng, 2012-08-21 01:45:03]

On 2012/08/20 16:26:16, nileshagrawal1 wrote:
> LGTM
thanks.
@rsesek, if you don't have comments, i'll commit it. :)

[Robert Sesek, 2012-08-21 13:30:35]

LGTM

On 2012/08/19 22:40:17, brettw wrote:
> This optimization seems scary and I wonder how necessary it is. LGTM for this
> patch, though.

The comment at the top of that block says that it avoids about 2/3rds of string
copies on a typical input.

[yongsheng, 2012-08-22 01:21:08]

thank you all.

[commit-bot: I haz the power, 2012-08-22 01:21:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10831322/2002

[commit-bot: I haz the power, 2012-08-22 02:53:23]

Try job failure for 10831322-2002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-22 05:28:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10831322/2002

[commit-bot: I haz the power, 2012-08-22 05:47:30]

Try job failure for 10831322-2002 on win for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-08-23 01:15:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10831322/2002

[commit-bot: I haz the power, 2012-08-23 03:42:39]

Change committed as 152944

[nilesh, 2012-08-23 17:33:41]

https://chromiumcodereview.appspot.com/10831322/diff/2002/build/android/gtest...
File build/android/gtest_filter/unit_tests_disabled (left):

https://chromiumcodereview.appspot.com/10831322/diff/2002/build/android/gtest...
build/android/gtest_filter/unit_tests_disabled:47:
LeveldbValueStore/ValueStoreTest.DotsInKeyNamesWithDicts/0
Looks like the test is still failing on the bots. Can you please take a look.
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Linux%20Android%...

[nilesh, 2012-08-23 20:21:32]

https://chromiumcodereview.appspot.com/10831322/diff/2002/build/android/gtest...
File build/android/gtest_filter/unit_tests_disabled (left):

https://chromiumcodereview.appspot.com/10831322/diff/2002/build/android/gtest...
build/android/gtest_filter/unit_tests_disabled:47:
LeveldbValueStore/ValueStoreTest.DotsInKeyNamesWithDicts/0
On 2012/08/23 17:33:42, nileshagrawal1 wrote:
> Looks like the test is still failing on the bots. Can you please take a look.
>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%2520Linux%2520Andr...

Nevermind, this is an issue with the android tester bots.

[yongsheng, 2012-08-24 01:04:38]

On 2012/08/23 20:21:32, nileshagrawal1 wrote:
>
https://chromiumcodereview.appspot.com/10831322/diff/2002/build/android/gtest...
> File build/android/gtest_filter/unit_tests_disabled (left):
> 
>
https://chromiumcodereview.appspot.com/10831322/diff/2002/build/android/gtest...
> build/android/gtest_filter/unit_tests_disabled:47:
> LeveldbValueStore/ValueStoreTest.DotsInKeyNamesWithDicts/0
> On 2012/08/23 17:33:42, nileshagrawal1 wrote:
> > Looks like the test is still failing on the bots. Can you please take a
look.
> >
>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%252520Linux%252520...
> 
> Nevermind, this is an issue with the android tester bots.
ok, got it. thanks.