Issue 1661233003: Revert of Fix leak PaintLayerReflectionInfo - Code Review

Chromium Code Reviews

chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out

(710)

My Issues | Starred Open | Closed | All

Issue 1661233003: Revert of Fix leak PaintLayerReflectionInfo (Closed)

Created:
4 years, 10 months ago by pdr.

Modified:
4 years, 10 months ago

Reviewers:
Xianzhu

CC:
blink-reviews, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, gavinp+loader_chromium.org, Nate Chapin, loading-reviews+fetch_chromium.org, slimming-paint-reviews_chromium.org, tyoshino+watch_chromium.org, Yoav Weiss, pcc

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Revert of Fix leak PaintLayerReflectionInfo (patchset #3 id:40001 of https://codereview.chromium.org/1657883002/ ) Reason for revert: Caused crashes on CrWinClang: https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds/4122/steps/webkit_unit_tests/logs/stdio Original issue's description: > Fix leak PaintLayerReflectionInfo > > 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/8718e5b803192df3cd4cec8907330443104063ee > Cr-Commit-Position: refs/heads/master@{#372985} TBR=wangxianzhu@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=582717 Committed: https://crrev.com/304935d656ea2151cf306e834ce93a59ed4d0276 Cr-Commit-Position: refs/heads/master@{#373458}

Patch Set 1 #

Created: 4 years, 10 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+6 lines, -42 lines)			Patch
	M	third_party/WebKit/Source/core/core.gypi	View		1 chunk	+0 lines, -1 line	0 comments	Download
	M	third_party/WebKit/Source/core/fetch/ImageResource.h	View		1 chunk	+0 lines, -2 lines	0 comments	Download
	M	third_party/WebKit/Source/core/paint/PaintLayer.cpp	View		2 chunks	+4 lines, -0 lines	0 comments	Download
	M	third_party/WebKit/Source/core/paint/PaintLayerReflectionInfo.h	View		1 chunk	+1 line, -1 line	0 comments	Download
	M	third_party/WebKit/Source/core/paint/PaintLayerReflectionInfo.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
	D	third_party/WebKit/Source/core/paint/PaintLayerTest.cpp	View		1 chunk	+0 lines, -37 lines	0 comments	Download

Messages

Total messages: 10 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

commit-bot: I haz the power

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

4 years, 10 months ago (2016-02-04 02:37:08 UTC) #2

commit-bot: I haz the power

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

4 years, 10 months ago (2016-02-04 03:03:39 UTC) #3

commit-bot: I haz the power

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

4 years, 10 months ago (2016-02-04 03:34:20 UTC) #4

commit-bot: I haz the power

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

4 years, 10 months ago (2016-02-04 04:04:42 UTC) #5

commit-bot: I haz the power

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

4 years, 10 months ago (2016-02-04 04:32:50 UTC) #6

commit-bot: I haz the power

Description was changed from ========== Revert of Fix leak PaintLayerReflectionInfo (patchset #3 id:40001 of https://codereview.chromium.org/1657883002/ ...

4 years, 10 months ago (2016-02-04 04:38:13 UTC) #7

Message was sent while issue was closed.

Description was changed from

==========
Revert of Fix leak PaintLayerReflectionInfo (patchset #3 id:40001 of
https://codereview.chromium.org/1657883002/ )

Reason for revert:
Caused crashes on CrWinClang:
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds...

Original issue's description:
> Fix leak PaintLayerReflectionInfo
> 
> 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/8718e5b803192df3cd4cec8907330443104063ee
> Cr-Commit-Position: refs/heads/master@{#372985}

TBR=wangxianzhu@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=582717
==========

to

==========
Revert of Fix leak PaintLayerReflectionInfo (patchset #3 id:40001 of
https://codereview.chromium.org/1657883002/ )

Reason for revert:
Caused crashes on CrWinClang:
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds...

Original issue's description:
> Fix leak PaintLayerReflectionInfo
> 
> 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/8718e5b803192df3cd4cec8907330443104063ee
> Cr-Commit-Position: refs/heads/master@{#372985}

TBR=wangxianzhu@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=582717
==========

commit-bot: I haz the power

Description was changed from ========== Revert of Fix leak PaintLayerReflectionInfo (patchset #3 id:40001 of https://codereview.chromium.org/1657883002/ ...

4 years, 10 months ago (2016-02-04 04:39:26 UTC) #9

Message was sent while issue was closed.

Description was changed from

==========
Revert of Fix leak PaintLayerReflectionInfo (patchset #3 id:40001 of
https://codereview.chromium.org/1657883002/ )

Reason for revert:
Caused crashes on CrWinClang:
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds...

Original issue's description:
> Fix leak PaintLayerReflectionInfo
> 
> 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/8718e5b803192df3cd4cec8907330443104063ee
> Cr-Commit-Position: refs/heads/master@{#372985}

TBR=wangxianzhu@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=582717
==========

to

==========
Revert of Fix leak PaintLayerReflectionInfo (patchset #3 id:40001 of
https://codereview.chromium.org/1657883002/ )

Reason for revert:
Caused crashes on CrWinClang:
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds...

Original issue's description:
> Fix leak PaintLayerReflectionInfo
>
> 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/8718e5b803192df3cd4cec8907330443104063ee
> Cr-Commit-Position: refs/heads/master@{#372985}

TBR=wangxianzhu@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=582717

Committed: https://crrev.com/304935d656ea2151cf306e834ce93a59ed4d0276
Cr-Commit-Position: refs/heads/master@{#373458}
==========

commit-bot: I haz the power

4 years, 10 months ago (2016-02-04 04:39:27 UTC) #10

Message was sent while issue was closed.

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

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Powered by Google App Engine

This is Rietveld 408576698