Reland Fix leak PaintLayerReflectionInfo

Reland Fix leak PaintLayerReflectionInfo

NOT FOR COMMIT, YET

Originally https://codereview.chromium.org/1657883002/
Patch by wangxianzhu@

Previously PaintLayerReflectionInfo required destroy() to be called
before destruction, but we missed the call in
PaintLayer::updateReflectionInfo(), causing leak of the reflection
layout object and the layer for the reflection.

Delete PaintLayerReflectionInfo::destroy(). Use destructor instead.

BUG=582717
TEST=PaintLayerTest.ReflectionLeak

Committed: https://crrev.com/6137b72d44c0391a39e2a8f9cad9efb4706880e9
Cr-Commit-Position: refs/heads/master@{#374556}

[Stephen Chennney, 2016-02-08 21:15:06]

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

[Stephen Chennney, 2016-02-08 21:17:24]

schenney@chromium.org changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org

[Stephen Chennney, 2016-02-08 21:17:25]

I'm trying out the exact same patch for this issue to see if the issue
reproduces on the trybots.

I couldn't repro the WinClang crash locally, but it's very hard to know if I
have exactly the same setup. I know I built with clang and I know I ran the
test. But maybe it's a 32/64 bit issue or something.

We'll wait and see.

[commit-bot: I haz the power, 2016-02-08 21:19:45]

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

[chrishtr, 2016-02-08 21:24:40]

What is different about the test now vs Xianzhu's original patch?

[Stephen Chennney, 2016-02-08 21:30:13]

On 2016/02/08 21:24:40, chrishtr wrote:
> What is different about the test now vs Xianzhu's original patch?

Nothing at all. Clang on Windows is still not the default option, and there is
active development. The fact that the test only failed on Win Clang suggests the
compiler's behavior might be the source of the crash, so I want to try it again
on the bots before I spend more time trying to reproduce it locally.

I'm specifically trying all the win clang bots, which are not normally checked
as part of the CQ.

[Stephen Chennney, 2016-02-08 21:31:15]

Description was changed from

==========
Reland Fix leak PaintLayerReflectionInfo

Originally https://codereview.chromium.org/1657883002/
Patch by wangxianzhu@

Previously PaintLayerReflectionInfo required destroy() to be called
before destruction, but we missed the call in
PaintLayer::updateReflectionInfo(), causing leak of the reflection
layout object and the layer for the reflection.

Delete PaintLayerReflectionInfo::destroy(). Use destructor instead.

BUG=582717
TEST=PaintLayerTest.ReflectionLeak
==========

to

==========
Reland Fix leak PaintLayerReflectionInfo

NOT FOR COMMIT, YET

Originally https://codereview.chromium.org/1657883002/
Patch by wangxianzhu@

Previously PaintLayerReflectionInfo required destroy() to be called
before destruction, but we missed the call in
PaintLayer::updateReflectionInfo(), causing leak of the reflection
layout object and the layer for the reflection.

Delete PaintLayerReflectionInfo::destroy(). Use destructor instead.

BUG=582717
TEST=PaintLayerTest.ReflectionLeak
==========

[chrishtr, 2016-02-08 22:18:52]

lgtm

[commit-bot: I haz the power, 2016-02-08 23:08:50]

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

[commit-bot: I haz the power, 2016-02-08 23:08:50]

Dry run: This issue passed the CQ dry run.

[Stephen Chennney, 2016-02-09 21:12:35]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2016-02-09 21:12:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1681663003/#ps20001
(title: "Rebased")

[Stephen Chennney, 2016-02-09 21:13:41]

Try landing this again too see if my failure to reproduce is real. If it still
fails on one bot I think the clang team might need to look at it. Or we need
input from the infra team on how to reproduce the bot failure.

[pdr., 2016-02-09 21:14:46]

pdr@chromium.org changed reviewers:
+ rnk@chromium.org

[pdr., 2016-02-09 21:15:37]

@rnk, this looks like it's now passing the win clang bot that caused this to be
rolled out (https://code.google.com/p/chromium/issues/detail?id=582717#c16).
Just ccing you in case you notice this happen again.

[commit-bot: I haz the power, 2016-02-09 21:16:30]

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

[Reid Kleckner, 2016-02-09 21:23:06]

On 2016/02/09 21:15:37, pdr wrote:
> @rnk, this looks like it's now passing the win clang bot that caused this to
be
> rolled out (https://code.google.com/p/chromium/issues/detail?id=582717#c16).
> Just ccing you in case you notice this happen again.

Sounds good, if it fails again I'll try to repro locally.

[Reid Kleckner, 2016-02-09 21:23:24]

rnk@chromium.org changed reviewers:
+ hans@chromium.org

[Reid Kleckner, 2016-02-09 21:23:43]

+Hans, this weeks clang sheriff

[hans, 2016-02-09 21:26:39]

On 2016/02/09 21:23:43, Reid Kleckner wrote:
> +Hans, this weeks clang sheriff

Thanks, I'll keep an eye out for it.

[Stephen Chennney, 2016-02-09 21:55:28]

On 2016/02/09 21:26:39, hans wrote:
> On 2016/02/09 21:23:43, Reid Kleckner wrote:
> > +Hans, this weeks clang sheriff
> 
> Thanks, I'll keep an eye out for it.

It's passing the win clang try bots but they don't seem to run any tests, which
seems like a lack of coverage. My only evidence that this won't fail again is
the fact that I couldn't repro locally.

[commit-bot: I haz the power, 2016-02-10 00:47:51]

Description was changed from

==========
Reland Fix leak PaintLayerReflectionInfo

NOT FOR COMMIT, YET

Originally https://codereview.chromium.org/1657883002/
Patch by wangxianzhu@

Previously PaintLayerReflectionInfo required destroy() to be called
before destruction, but we missed the call in
PaintLayer::updateReflectionInfo(), causing leak of the reflection
layout object and the layer for the reflection.

Delete PaintLayerReflectionInfo::destroy(). Use destructor instead.

BUG=582717
TEST=PaintLayerTest.ReflectionLeak
==========

to

==========
Reland Fix leak PaintLayerReflectionInfo

NOT FOR COMMIT, YET

Originally https://codereview.chromium.org/1657883002/
Patch by wangxianzhu@

Previously PaintLayerReflectionInfo required destroy() to be called
before destruction, but we missed the call in
PaintLayer::updateReflectionInfo(), causing leak of the reflection
layout object and the layer for the reflection.

Delete PaintLayerReflectionInfo::destroy(). Use destructor instead.

BUG=582717
TEST=PaintLayerTest.ReflectionLeak
==========

[commit-bot: I haz the power, 2016-02-10 00:47:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-10 00:48:37]

Description was changed from

==========
Reland Fix leak PaintLayerReflectionInfo

NOT FOR COMMIT, YET

Originally https://codereview.chromium.org/1657883002/
Patch by wangxianzhu@

Previously PaintLayerReflectionInfo required destroy() to be called
before destruction, but we missed the call in
PaintLayer::updateReflectionInfo(), causing leak of the reflection
layout object and the layer for the reflection.

Delete PaintLayerReflectionInfo::destroy(). Use destructor instead.

BUG=582717
TEST=PaintLayerTest.ReflectionLeak
==========

to

==========
Reland Fix leak PaintLayerReflectionInfo

NOT FOR COMMIT, YET

Originally https://codereview.chromium.org/1657883002/
Patch by wangxianzhu@

Previously PaintLayerReflectionInfo required destroy() to be called
before destruction, but we missed the call in
PaintLayer::updateReflectionInfo(), causing leak of the reflection
layout object and the layer for the reflection.

Delete PaintLayerReflectionInfo::destroy(). Use destructor instead.

BUG=582717
TEST=PaintLayerTest.ReflectionLeak

Committed: https://crrev.com/6137b72d44c0391a39e2a8f9cad9efb4706880e9
Cr-Commit-Position: refs/heads/master@{#374556}
==========

[commit-bot: I haz the power, 2016-02-10 00:48:38]

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

[blundell, 2016-02-10 10:09:32]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1685073002/ by blundell@chromium.org.

The reason for reverting is: I suspect this broke webkit_unit_tests on Mac Asan
(https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests...).
Will reland if not the culprit.

Failure snippet:

[ RUN      ] All/ParameterizedWebFrameTest.CompositedSelectionBoundsCleared/1
=================================================================
==37555==ERROR: AddressSanitizer: heap-use-after-free on address 0x610000001b50
at pc 0x00010b1599b5 bp 0x7fff5bb669d0 sp 0x7fff5bb669c8
READ of size 8 at 0x610000001b50 thread T0
    #0 0x10b1599b4 in blink::PaintLayerScrollableArea::scrollbarsCanBeActive()
const (in webkit_unit_tests) + 420
    #1 0x105dd885b in blink::ScrollAnimatorMac::updateScrollerStyle() (in
webkit_unit_tests) + 283

.