Fix CFI: Invalid cast in PaymentRequestTest

Fix CFI: Invalid cast in PaymentRequestTest

Do not reset PaymentDetails.items and PaymentDetails.shippingOptions to
avoid double-deletion. Avoid setting these properties instead. This
affects only unit tests.

BUG=600808
Committed: https://crrev.com/59dd5d45f05cab02ef9410b505cbae3dfd5d4ed4
Cr-Commit-Position: refs/heads/master@{#385478}

[please use gerrit instead, 2016-04-05 23:25:13]

Description was changed from

==========
Fix CFI: Invalid cast in PaymentRequestTest

Do not reset PaymentDetails.items and PaymentDetails.shippingOptions to
avoid double-deletion. Avoid setting these properties instead.

BUG=600808
==========

to

==========
Fix CFI: Invalid cast in PaymentRequestTest

Do not reset PaymentDetails.items and PaymentDetails.shippingOptions to
avoid double-deletion. Avoid setting these properties instead. This
affects only unit tests.

BUG=600808
==========

[please use gerrit instead, 2016-04-06 00:22:47]

rouslan@chromium.org changed reviewers:
+ krasin@chromium.org, mek@google.com

[please use gerrit instead, 2016-04-06 00:22:48]

mek@, krasin@: ptal.

[krasin, 2016-04-06 00:35:03]

Assuming the patch passes the tests under CFI, I have no objections against the
CL. Have you verified that locally?

[please use gerrit instead, 2016-04-06 00:47:32]

On 2016/04/06 00:35:03, krasin wrote:
> Assuming the patch passes the tests under CFI, I have no objections against
the
> CL. Have you verified that locally?

Verified locally and also check out linux_chromium_cfi_rel_ng try bot ^^

[krasin, 2016-04-06 00:53:08]

Thanks!

A somewhat tangential question: can you please run 'git cl format' to get the
formatting in these files right?
See https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md

[please use gerrit instead, 2016-04-06 00:54:53]

On 2016/04/06 00:53:08, krasin wrote:
> A somewhat tangential question: can you please run 'git cl format' to get the
> formatting in these files right?

Done, but that did not change any files. Do you have a specific concern?

[krasin, 2016-04-06 01:06:47]

I feel that some of the files have unusually long lines which would have been
eliminated by the formatter.

I've tried 'git cl format --full', but the results were even less expected.
Anyway, that is not relevant to this particular fix, so LGTM.

Thank you for the fix.

[krasin, 2016-04-06 01:07:08]

The CQ bit was checked by krasin@google.com

[commit-bot: I haz the power, 2016-04-06 01:07:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860303002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1860303002/1

[please use gerrit instead, 2016-04-06 01:12:34]

On 2016/04/06 01:06:47, krasin wrote:
> I feel that some of the files have unusually long lines which would have been
> eliminated by the formatter.

Code in third_party/WebKit uses a slightly different style. For example, there's
no line length limit, indents are 4 spaces, and function opening braces are
placed on a new line.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
points to:
http://clang.llvm.org/docs/ClangFormatStyleOptions.html#configurable-format-s...
, which points to:
https://webkit.org/code-style-guidelines/

[krasin, 2016-04-06 01:18:45]

Thank you for the explanation. That totally makes sense now. :)

[commit-bot: I haz the power, 2016-04-06 07:08:26]

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

[commit-bot: I haz the power, 2016-04-06 07:08:27]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[krasin, 2016-04-06 15:57:02]

The CQ bit was checked by krasin@google.com

[commit-bot: I haz the power, 2016-04-06 15:57:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860303002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1860303002/1

[commit-bot: I haz the power, 2016-04-06 16:43:21]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-06 16:44:26]

Description was changed from

==========
Fix CFI: Invalid cast in PaymentRequestTest

Do not reset PaymentDetails.items and PaymentDetails.shippingOptions to
avoid double-deletion. Avoid setting these properties instead. This
affects only unit tests.

BUG=600808
==========

to

==========
Fix CFI: Invalid cast in PaymentRequestTest

Do not reset PaymentDetails.items and PaymentDetails.shippingOptions to
avoid double-deletion. Avoid setting these properties instead. This
affects only unit tests.

BUG=600808

Committed: https://crrev.com/59dd5d45f05cab02ef9410b505cbae3dfd5d4ed4
Cr-Commit-Position: refs/heads/master@{#385478}
==========

[commit-bot: I haz the power, 2016-04-06 16:44:27]

Patchset 1 (id:??) landed as
https://crrev.com/59dd5d45f05cab02ef9410b505cbae3dfd5d4ed4
Cr-Commit-Position: refs/heads/master@{#385478}

[Marijn Kruisselbrink, 2016-04-06 17:04:30]

patch LGTM, although I don't understand what exactly was broken. Did this
accidentally hit some oilpan/v8/bindings bug or something? As in, isn't one of
the reasons to use garbage collection to make sure you don't have to worry as
much about memory management/double deletion?
If there are edge cases like this were our garbage collectors can cause double
deletions, I hope these edge cases are clearly documented/well understood
somewhere? Or am I completely misunderstanding what was going on in this bug?

[krasin, 2016-04-06 17:12:00]

On 2016/04/06 17:04:30, Marijn Kruisselbrink wrote:
> patch LGTM, although I don't understand what exactly was broken. Did this
> accidentally hit some oilpan/v8/bindings bug or something? As in, isn't one of
> the reasons to use garbage collection to make sure you don't have to worry as
> much about memory management/double deletion?
> If there are edge cases like this were our garbage collectors can cause double
> deletions, I hope these edge cases are clearly documented/well understood
> somewhere? Or am I completely misunderstanding what was going on in this bug?

It's my understanding that there was a double delete (GC + manual delete)
somewhere. CFI complained about accessing already deleted C++ object from within
GC. This fix unfortunately does not provide a full explanation of what's
happened. I fully expect that similar issue with the same code will appear in
the nearest future.

[Marijn Kruisselbrink, 2016-04-06 17:23:29]

On 2016/04/06 at 17:12:00, krasin wrote:
> On 2016/04/06 17:04:30, Marijn Kruisselbrink wrote:
> > patch LGTM, although I don't understand what exactly was broken. Did this
> > accidentally hit some oilpan/v8/bindings bug or something? As in, isn't one
of
> > the reasons to use garbage collection to make sure you don't have to worry
as
> > much about memory management/double deletion?
> > If there are edge cases like this were our garbage collectors can cause
double
> > deletions, I hope these edge cases are clearly documented/well understood
> > somewhere? Or am I completely misunderstanding what was going on in this
bug?
> 
> It's my understanding that there was a double delete (GC + manual delete)
somewhere. CFI complained about accessing already deleted C++ object from within
GC. This fix unfortunately does not provide a full explanation of what's
happened. I fully expect that similar issue with the same code will appear in
the nearest future.

So it sounds like there is some kind of code pattern that can cause something
that should be under GC control to be manually deleted? Where is the
investigation happening about what exactly the problematic code pattern is, and
if there is some way such a pattern can get statically detected (or maybe the GC
could be fixed to detect that something is already deleted). Just changing this
code to no longer hit the problematic case, without actually understanding what
the problem is seems like the wrong thing to do. As for all we know there are
other places much harder to debug where we hit similar problems. And it's still
not clear to me if this is actually a bug in the GC or not.

[krasin, 2016-04-06 17:25:22]

I suppose it's the question to the author of the code and the fix.
My contribution was the bug report: https://crbug.com/600808.

[please use gerrit instead, 2016-04-06 17:40:07]

I was using code like this in my tests:

ShippingOption shippingOption;
PaymentDetails details;
details.setShippingOptions(HeapVector<ShippingOption>(1, shippingOption));
details.setShippingOptions(HeapVector<ShippingOption>()); // <--- Causes the
bug.

It appears that the assignment operator for HeapVector is deleting its own
elements instead of letting the garbage collector do its job. I can write a
patch that reproduces the problem and file a bug against the garbage collector.
Sounds like a plan?

[haraken, 2016-04-07 01:57:49]

On 2016/04/06 17:40:07, Rouslan wrote:
> I was using code like this in my tests:
> 
> ShippingOption shippingOption;
> PaymentDetails details;
> details.setShippingOptions(HeapVector<ShippingOption>(1, shippingOption));
> details.setShippingOptions(HeapVector<ShippingOption>()); // <--- Causes the
> bug.
> 
> It appears that the assignment operator for HeapVector is deleting its own
> elements instead of letting the garbage collector do its job. I can write a
> patch that reproduces the problem and file a bug against the garbage
collector.
> Sounds like a plan?

I don't think the above code causes double-free, but we definitely must fix the
underlying problem here.

Please file a bug ccing oilpan-reviews@.

[please use gerrit instead, 2016-04-07 02:13:47]

Filed http://crbug.com/601193