1618501 - Crash when displaying a large table if layers.force-active=true

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Zach Hoffman [:zrhoffman]]

Attachment: testcase.html

mozilla::layers::APZCTreeManager::UpdateHitTestingTreeImpl<mozilla::layers::LayerMetricsWrapper> calls mRootNode->Dump(" "), which calls mLastChild->Dump(nsPrintfCString("%s ", aPrefix).get()), which recursively calls mPrevSibling->Dump(aPrefix) until unsafe memory is accessed.

Comment 1

[Zach Hoffman [:zrhoffman]]

Attachment: gdb backtrace

Attached backtrace.

Comment 2

[Zach Hoffman [:zrhoffman]]

Tested in a trunk build from today (515608:7f41334e1044), verified in latest nightly, both on 64-bit Arch Linux.

Comment 3

[:Gijs (he/him)]

:kats, from a quick look at the stack this looks like it got tripped by APZ (mozilla::layers::APZCTreeManager::UpdateHitTestingTreeImpl<mozilla::layers::LayerMetricsWrapper>(), though I suppose it's quite possible for the actual bug to be elsewhere - could you take a look and/or redirect as appropriate?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Thanks, I'll investigate. Regression from bug 1616591 since prior to that the code in question was compiled out by default.

Comment 5

[:Gijs (he/him)]

Thanks! Updating release flags based on that. :-)

Comment 6

[Andrew McCreight [:mccr8]]

This just looks like stack exhaustion, so I'm unhiding it. (Tyson said the fuzzers are hitting this a lot, too.)

Comment 7

[Kartikaya Gupta (email:[e-mail])]

So there's two problems here: one is that we're going into the logging code at all even when logging is not enabled. The other is that the logging code is poorly written (by me) and recurses across siblings as well as when descending to children. So for layer trees that are really wide, such as in this example, this results in stack exhaustion.

I have patches to fix both of these things, will put them up once my try results are back. https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=8cf8ac8e1ada0bcbfcb43c3cd143035001079352

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1618501 - Don't go into the hit-testing tree dumping codepath if we're not actually dumping. r?botond

This avoids doing a full recursive walk of the tree for no purpose unless
the logging is actually enabled.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1618501 - Rewrite the dumping code to not recurse so much. r?botond

The HTTN dumping code recursed not only when going down the tree, but
also when traversing siblings. For "wide" trees this could result in
stack exhaustion. This patch rewrites that function to only recurse
when going down a level and uses iteration to walk the siblings at a
given level.

Depends on D64657

Comment 10

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=03beedc8f87efefa2a5bffde50832bbce7dbe16e is the better try push

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e3c1d399649
Don't go into the hit-testing tree dumping codepath if we're not actually dumping. r=botond
https://hg.mozilla.org/integration/autoland/rev/6cb26395019b
Rewrite the dumping code to not recurse so much. r=botond

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/8e3c1d399649
https://hg.mozilla.org/mozilla-central/rev/6cb26395019b

Comment 13

[Daniel Veditz [:dveditz]]

Unfortunately this kind of denial of service is not covered by our security bug bounty program.