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

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:
5 months, 1 week ago by mstensho
Modified:
4 months, 1 week 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
5 months, 1 week ago (2017-01-17 15:24:52 UTC) #6
eae
This is great, thank you.
5 months, 1 week ago (2017-01-17 15:58:33 UTC) #7
eae
LGTM
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 months, 1 week 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
5 months, 1 week 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)
5 months, 1 week 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
5 months, 1 week 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)
5 months, 1 week 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
5 months, 1 week 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)
5 months, 1 week 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 ...
5 months, 1 week ago (2017-01-18 13:13:43 UTC) #23
dgrogan
I don't know how to reproduce locally either, but +wangxianzhu probably does :)
5 months, 1 week ago (2017-01-18 16:46:15 UTC) #24
dgrogan
I don't know how to reproduce locally either, but +wangxianzhu probably does :)
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 months, 1 week ago (2017-01-19 10:00:16 UTC) #29
Xianzhu
Can anyone who can access the bug add me to its cc list?
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 months 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 ...
5 months 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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
4 months, 1 week 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)
4 months, 1 week 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
4 months, 1 week ago (2017-02-17 06:28:53 UTC) #49
commit-bot: I haz the power
4 months, 1 week 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 cb946e318