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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 490473003: Defer call to updateWidgetPositions() outside of RenderLayerScrollableArea. (Closed)

Created:
6 years, 4 months ago by hartmanng
Modified:
6 years, 4 months ago
Reviewers:
Ian Vollick
CC:
blink-layers+watch_chromium.org, blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr., rune+blink, zoltan1
Project:
blink
Visibility:
Public.

Description

Defer call to updateWidgetPositions() outside of RenderLayerScrollableArea. updateWidgetPositions() can destroy the render tree, so it should never be called from inside RenderLayerScrollableArea. Leaving it there allows for the potential of use-after-free bugs. BUG=402407 R=vollick@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180681

Patch Set 1 #

Patch Set 2 : add layout test #

Patch Set 3 : remove release-assert #

Total comments: 5

Patch Set 4 : fix style #

Patch Set 5 : doctype #

Messages

Total messages: 20 (0 generated)
hartmanng
6 years, 4 months ago (2014-08-19 20:37:37 UTC) #1
Ian Vollick
On 2014/08/19 at 20:37:37, hartmanng wrote: > This change seems quite straightforward and reasonable, but ...
6 years, 4 months ago (2014-08-19 21:07:10 UTC) #2
hartmanng
On 2014/08/19 21:07:10, vollick wrote: > On 2014/08/19 at 20:37:37, hartmanng wrote: > > > ...
6 years, 4 months ago (2014-08-20 15:33:10 UTC) #3
Ian Vollick
lgtm. Thanks for the test! https://codereview.chromium.org/490473003/diff/40001/LayoutTests/compositing/overflow/resources/do-not-crash-use-after-free-update-widget-positions-iframe.html File LayoutTests/compositing/overflow/resources/do-not-crash-use-after-free-update-widget-positions-iframe.html (right): https://codereview.chromium.org/490473003/diff/40001/LayoutTests/compositing/overflow/resources/do-not-crash-use-after-free-update-widget-positions-iframe.html#newcode1 LayoutTests/compositing/overflow/resources/do-not-crash-use-after-free-update-widget-positions-iframe.html:1: <html> DOCTYPE, and probably ...
6 years, 4 months ago (2014-08-20 17:42:59 UTC) #4
hartmanng
Thanks! https://codereview.chromium.org/490473003/diff/40001/LayoutTests/compositing/overflow/resources/do-not-crash-use-after-free-update-widget-positions-iframe.html File LayoutTests/compositing/overflow/resources/do-not-crash-use-after-free-update-widget-positions-iframe.html (right): https://codereview.chromium.org/490473003/diff/40001/LayoutTests/compositing/overflow/resources/do-not-crash-use-after-free-update-widget-positions-iframe.html#newcode1 LayoutTests/compositing/overflow/resources/do-not-crash-use-after-free-update-widget-positions-iframe.html:1: <html> On 2014/08/20 17:42:59, vollick wrote: > DOCTYPE, ...
6 years, 4 months ago (2014-08-20 17:59:50 UTC) #5
hartmanng
The CQ bit was checked by hartmanng@chromium.org
6 years, 4 months ago (2014-08-20 17:59:57 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hartmanng@chromium.org/490473003/60001
6 years, 4 months ago (2014-08-20 18:00:31 UTC) #7
hartmanng
The CQ bit was checked by hartmanng@chromium.org
6 years, 4 months ago (2014-08-20 18:01:51 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hartmanng@chromium.org/490473003/80001
6 years, 4 months ago (2014-08-20 18:01:59 UTC) #9
hartmanng
The CQ bit was checked by hartmanng@chromium.org
6 years, 4 months ago (2014-08-20 18:04:11 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hartmanng@chromium.org/490473003/100001
6 years, 4 months ago (2014-08-20 18:04:20 UTC) #11
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink ...
6 years, 4 months ago (2014-08-20 19:16:48 UTC) #12
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-20 19:19:25 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/22138)
6 years, 4 months ago (2014-08-20 19:19:26 UTC) #14
hartmanng
The CQ bit was checked by hartmanng@chromium.org
6 years, 4 months ago (2014-08-20 19:57:41 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hartmanng@chromium.org/490473003/100001
6 years, 4 months ago (2014-08-20 19:58:08 UTC) #16
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink ...
6 years, 4 months ago (2014-08-20 20:01:29 UTC) #17
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-20 20:04:02 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/22155) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/21532)
6 years, 4 months ago (2014-08-20 20:04:02 UTC) #19
hartmanng
6 years, 4 months ago (2014-08-20 20:09:30 UTC) #20
Message was sent while issue was closed. 
Committed patchset #5 manually as 180681 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698