Don't keep pointers to table sections when told to recalculate sections.

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

[mstensho (USE GERRIT), 2017-01-17 13:01:14]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 13:01:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 14:54:31]

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

[commit-bot: I haz the power, 2017-01-17 14:54:32]

Dry run: 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_...)

[mstensho (USE GERRIT), 2017-01-17 15:24:51]

mstensho@opera.com changed reviewers:
+ dgrogan@chromium.org, eae@chromium.org

[eae, 2017-01-17 15:58:33]

This is great, thank you.

[eae, 2017-01-17 15:58:39]

LGTM

[dgrogan, 2017-01-17 16:00:22]

LGTM 2

We get an assertion failure in debug but a harmless (not use-afer-free) crash in
release, right?

[mstensho (USE GERRIT), 2017-01-17 16:31:16]

On 2017/01/17 16:00:22, dgrogan wrote:
> LGTM 2
> 
> We get an assertion failure in debug but a harmless (not use-afer-free) crash
in
> release, right?

No crash, just an assertion failure in debug.

[eae, 2017-01-17 17:13:51]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2017-01-17 17:14:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 19:46:44]

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

[commit-bot: I haz the power, 2017-01-17 19:46:45]

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_...)

[mstensho (USE GERRIT), 2017-01-17 20:02:40]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2017-01-17 20:03:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 22:31:27]

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

[commit-bot: I haz the power, 2017-01-17 22:31:28]

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_...)

[mstensho (USE GERRIT), 2017-01-18 08:54:20]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2017-01-18 08:54:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 11:17:17]

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

[commit-bot: I haz the power, 2017-01-18 11:17:18]

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_...)

[mstensho (USE GERRIT), 2017-01-18 13:13:43]

On 2017/01/18 11:17:18, commit-bot: I haz the power wrote:
> 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_...)

OK, looks like my new assertions fail here (and only here - no layout tests
failing):

===========

telemetry_perf_unittests (retry summary) telemetry_perf_unittests (retry
summary)

failures:
benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_desktop.browse:news:nytimes
benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_desktop.browse:news:reddit

===========

I have no idea how to reproduce this locally.

[dgrogan, 2017-01-18 16:46:15]

I don't know how to reproduce locally either, but +wangxianzhu probably does :)

[dgrogan, 2017-01-18 16:46:15]

I don't know how to reproduce locally either, but +wangxianzhu probably does :)

[Xianzhu, 2017-01-18 17:05:37]

On 2017/01/18 16:46:15, dgrogan wrote:
> I don't know how to reproduce locally either, but +wangxianzhu probably does
:)

Based on the stack trace, this is related to frame throttling:
FrameView::updateViewportIntersectionForSubtree() is called when there are dirty
layouts. I'll file a bug add related people for discussion.

[mstensho (USE GERRIT), 2017-01-18 19:39:52]

On 2017/01/18 17:05:37, Xianzhu wrote:
> On 2017/01/18 16:46:15, dgrogan wrote:
> > I don't know how to reproduce locally either, but +wangxianzhu probably does
> :)
> 
> Based on the stack trace, this is related to frame throttling:
> FrameView::updateViewportIntersectionForSubtree() is called when there are
dirty
> layouts. I'll file a bug add related people for discussion.

Thanks!

[Xianzhu, 2017-01-18 22:45:53]

It seems that we need some time to fix the dirty layout issues. Should we land
the setNeedsSectionRecalc() change first, and add the DCHECKs after we fix the
dirty layout issue?

[mstensho (USE GERRIT), 2017-01-19 10:00:16]

On 2017/01/18 22:45:53, Xianzhu wrote:
> It seems that we need some time to fix the dirty layout issues. Should we land
> the setNeedsSectionRecalc() change first, and add the DCHECKs after we fix the
> dirty layout issue?

Works for me. Or we can wait with landing this until that is fixed. There's no
rush, because I have a different CL that fixes this bug completely
(crash/assert-wise):

https://codereview.chromium.org/2635143003/

So I removed the dependency on bug 682307.

[Xianzhu, 2017-01-19 17:09:21]

Can anyone who can access the bug add me to its cc list?

[Xianzhu, 2017-01-19 17:10:00]

Description was changed from

==========
Don't keep pointers to table sections when told to recalculate sections.

They may have been deleted.

Assert in header(), footer() and firstBody() in LayoutTable that we're not
waiting for a section recalc.

This fixes the use-after-free issue with bug 680224, but now we get a (albeit
way more harmless) assertion failure instead.

BUG=680224
==========

to

==========
Don't keep pointers to table sections when told to recalculate sections.

They may have been deleted.

Assert in header(), footer() and firstBody() in LayoutTable that we're not
waiting for a section recalc.

This fixes the use-after-free issue with bug 680224, but now we get a (albeit
way more harmless) assertion failure instead.

BUG=680224
==========

[mstensho (USE GERRIT), 2017-01-19 17:38:58]

On 2017/01/19 17:09:21, Xianzhu wrote:
> Can anyone who can access the bug add me to its cc list?

Ah, dgrogan did it some minutes ago. :) Sorry, I kind of thought you had some
special privileges, since you added the blocker, but now I realize that you did
it from the other bug.

[eae, 2017-01-23 18:18:53]

On 2017/01/19 17:38:58, mstensho wrote:
> On 2017/01/19 17:09:21, Xianzhu wrote:
> > Can anyone who can access the bug add me to its cc list?
> 
> Ah, dgrogan did it some minutes ago. :) Sorry, I kind of thought you had some
> special privileges, since you added the blocker, but now I realize that you
did
> it from the other bug.

Ping? We'd like to ghet this in asap!

[mstensho (USE GERRIT), 2017-01-23 18:22:52]

On 2017/01/23 18:18:53, eae wrote:
> On 2017/01/19 17:38:58, mstensho wrote:
> > On 2017/01/19 17:09:21, Xianzhu wrote:
> > > Can anyone who can access the bug add me to its cc list?
> > 
> > Ah, dgrogan did it some minutes ago. :) Sorry, I kind of thought you had
some
> > special privileges, since you added the blocker, but now I realize that you
> did
> > it from the other bug.
> 
> Ping? We'd like to ghet this in asap!

Note that landing this isn't essential to fixing the bug anymore, as long as
https://codereview.chromium.org/2635143003/ lands.

[mstensho (USE GERRIT), 2017-02-16 10:45:39]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 10:46:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 13:18:34]

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

[commit-bot: I haz the power, 2017-02-16 13:18:36]

Dry run: 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_...)

[mstensho (USE GERRIT), 2017-02-16 13:47:56]

On 2017/01/18 22:45:53, Xianzhu wrote:
> It seems that we need some time to fix the dirty layout issues. Should we land
> the setNeedsSectionRecalc() change first, and add the DCHECKs after we fix the
> dirty layout issue?

I just confirmed that the DCHECKs are still failing. Maybe it's time to just
land my CL without the DCHECKs?

[Xianzhu, 2017-02-16 16:45:35]

On 2017/02/16 13:47:56, mstensho wrote:
> On 2017/01/18 22:45:53, Xianzhu wrote:
> > It seems that we need some time to fix the dirty layout issues. Should we
land
> > the setNeedsSectionRecalc() change first, and add the DCHECKs after we fix
the
> > dirty layout issue?
> 
> I just confirmed that the DCHECKs are still failing. Maybe it's time to just
> land my CL without the DCHECKs?

SGTM.

[mstensho (USE GERRIT), 2017-02-16 21:35:30]

Description was changed from

==========
Don't keep pointers to table sections when told to recalculate sections.

They may have been deleted.

Assert in header(), footer() and firstBody() in LayoutTable that we're not
waiting for a section recalc.

This fixes the use-after-free issue with bug 680224, but now we get a (albeit
way more harmless) assertion failure instead.

BUG=680224
==========

to

==========
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
==========

[mstensho (USE GERRIT), 2017-02-16 21:37:57]

On 2017/02/16 16:45:35, Xianzhu wrote:
> On 2017/02/16 13:47:56, mstensho wrote:
> > On 2017/01/18 22:45:53, Xianzhu wrote:
> > > It seems that we need some time to fix the dirty layout issues. Should we
> land
> > > the setNeedsSectionRecalc() change first, and add the DCHECKs after we fix
> the
> > > dirty layout issue?
> > 
> > I just confirmed that the DCHECKs are still failing. Maybe it's time to just
> > land my CL without the DCHECKs?
> 
> SGTM.

Okay, I'll try that. Actually, we probably only have to remove the DCHECK in
header(), and keep the ones in firstBody() and footer(). I'll see how it goes.

[mstensho (USE GERRIT), 2017-02-16 21:39:20]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2017-02-16 21:39:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgrogan@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2636153002/#ps60001
(title: "Disable one DCHECK that causes failures on the linux trybot.")

[commit-bot: I haz the power, 2017-02-16 21:40:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 00:07:50]

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

[commit-bot: I haz the power, 2017-02-17 00:07:51]

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_...)

[mstensho (USE GERRIT), 2017-02-17 06:28:21]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2017-02-17 06:28:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 07:10:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487312901490000,
"parent_rev": "6b7e091709d04f852f7572556c8dc6c1e8790fbb", "commit_rev":
"25993aa7c80d68de1a53bff157d02c8bc8ec3458"}

[commit-bot: I haz the power, 2017-02-17 07:11:10]

Description was changed from

==========
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
==========

to

==========
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/+/25993aa7c80d68de1a53bff157d0...
==========

[commit-bot: I haz the power, 2017-02-17 07:11:12]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/25993aa7c80d68de1a53bff157d0...