Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2636153002: Don't keep pointers to table sections when told to recalculate sections. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 months ago by mstensho
Modified:
6 months ago
Reviewers:
dgrogan, eae
CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't keep pointers to table sections when told to recalculate sections. They may have been deleted. We should ideally assert in header(), footer() and firstBody() in LayoutTable that we're not waiting for a section recalc, but that's currently failing in one trybot; see crbug.com/693212 This would have fixed the original use-after-free issue with bug 680224, but before this CL landed, I found another fix that attacked the root cause instead. Still no point in keeping potentially dead pointers around, though. BUG=680224 Review-Url: https://codereview.chromium.org/2636153002 Cr-Commit-Position: refs/heads/master@{#451257} Committed: https://chromium.googlesource.com/chromium/src/+/25993aa7c80d68de1a53bff157d02c8bc8ec3458

Patch Set 1 #

Patch Set 2 : rebase master #

Patch Set 3 : rebase master #

Patch Set 4 : Disable one DCHECK that causes failures on the linux trybot. #

Messages

Total messages: 52 (23 generated)
mstensho
7 months ago (2017-01-17 15:24:52 UTC) #6
eae
This is great, thank you.
7 months ago (2017-01-17 15:58:33 UTC) #7
eae
LGTM
7 months ago (2017-01-17 15:58:39 UTC) #8
dgrogan
LGTM 2 We get an assertion failure in debug but a harmless (not use-afer-free) crash ...
7 months ago (2017-01-17 16:00:22 UTC) #9
mstensho
On 2017/01/17 16:00:22, dgrogan wrote: > LGTM 2 > > We get an assertion failure ...
7 months ago (2017-01-17 16:31:16 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2636153002/1
7 months ago (2017-01-17 17:14:07 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/372212)
7 months ago (2017-01-17 19:46:45 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2636153002/1
7 months ago (2017-01-17 20:03:29 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/372299)
7 months ago (2017-01-17 22:31:28 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2636153002/1
7 months ago (2017-01-18 08:54:32 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/372845)
7 months ago (2017-01-18 11:17:18 UTC) #22
mstensho
On 2017/01/18 11:17:18, commit-bot: I haz the power wrote: > Try jobs failed on following ...
7 months ago (2017-01-18 13:13:43 UTC) #23
dgrogan
I don't know how to reproduce locally either, but +wangxianzhu probably does :)
7 months ago (2017-01-18 16:46:15 UTC) #24
dgrogan
I don't know how to reproduce locally either, but +wangxianzhu probably does :)
7 months ago (2017-01-18 16:46:15 UTC) #25
Xianzhu
On 2017/01/18 16:46:15, dgrogan wrote: > I don't know how to reproduce locally either, but ...
7 months ago (2017-01-18 17:05:37 UTC) #26
mstensho
On 2017/01/18 17:05:37, Xianzhu wrote: > On 2017/01/18 16:46:15, dgrogan wrote: > > I don't ...
7 months ago (2017-01-18 19:39:52 UTC) #27
Xianzhu
It seems that we need some time to fix the dirty layout issues. Should we ...
7 months ago (2017-01-18 22:45:53 UTC) #28
mstensho
On 2017/01/18 22:45:53, Xianzhu wrote: > It seems that we need some time to fix ...
7 months ago (2017-01-19 10:00:16 UTC) #29
Xianzhu
Can anyone who can access the bug add me to its cc list?
7 months ago (2017-01-19 17:09:21 UTC) #30
mstensho
On 2017/01/19 17:09:21, Xianzhu wrote: > Can anyone who can access the bug add me ...
7 months ago (2017-01-19 17:38:58 UTC) #32
eae
On 2017/01/19 17:38:58, mstensho wrote: > On 2017/01/19 17:09:21, Xianzhu wrote: > > Can anyone ...
6 months, 3 weeks ago (2017-01-23 18:18:53 UTC) #33
mstensho
On 2017/01/23 18:18:53, eae wrote: > On 2017/01/19 17:38:58, mstensho wrote: > > On 2017/01/19 ...
6 months, 3 weeks ago (2017-01-23 18:22:52 UTC) #34
mstensho
On 2017/01/18 22:45:53, Xianzhu wrote: > It seems that we need some time to fix ...
6 months ago (2017-02-16 13:47:56 UTC) #39
Xianzhu
On 2017/02/16 13:47:56, mstensho wrote: > On 2017/01/18 22:45:53, Xianzhu wrote: > > It seems ...
6 months ago (2017-02-16 16:45:35 UTC) #40
mstensho
On 2017/02/16 16:45:35, Xianzhu wrote: > On 2017/02/16 13:47:56, mstensho wrote: > > On 2017/01/18 ...
6 months ago (2017-02-16 21:37:57 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2636153002/60001
6 months ago (2017-02-16 21:40:50 UTC) #45
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/368319)
6 months ago (2017-02-17 00:07:51 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2636153002/60001
6 months ago (2017-02-17 06:28:53 UTC) #49
commit-bot: I haz the power
6 months ago (2017-02-17 07:11:12 UTC) #52
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/25993aa7c80d68de1a53bff157d0...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b