1520798 - Crash in mozilla::layout::ScrollAnchorContainer::InvalidateAnchor

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-b7dfce6e-aa59-41ab-8b3c-15ec90190117.

Seen while looking at nightly crash stats - crashes started in 20190117095319: https://bit.ly/2RvtI3q. 9 crashes/2 installs so far.

Code was touched in Bug 1305957. ni on rhunt

Top 10 frames of crashing thread:

0 xul.dll mozilla::layout::ScrollAnchorContainer::InvalidateAnchor layout/generic/ScrollAnchorContainer.cpp:224
1 xul.dll nsFrame::DidSetComputedStyle layout/generic/nsFrame.cpp:1101
2 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2772
3 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2818
4 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2818
5 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2818
6 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2818
7 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2818
8 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2818
9 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2818

Comment 1

[Daniel Holbert [:dholbert]]

We seem to be crashing on a null-deref, when checking the value of a member variable ("mAnchorNode"), inside of this call:

ScrollAnchorContainer::FindFor(this)->InvalidateAnchor();

I suspect this indicates that "FindFor" is returning null here.

Comment 2

[Daniel Holbert [:dholbert]]

stack-level 0 is for "if (mAnchorNode)" here:
https://hg.mozilla.org/mozilla-central/annotate/24f969298ec5c1a7cbc444bfc8f3d77b07f46bbe/layout/generic/ScrollAnchorContainer.cpp#l224

...and stack-level 1 is for "ScrollAnchorContainer::FindFor(this)->InvalidateAnchor();" here:
https://hg.mozilla.org/mozilla-central/annotate/24f969298ec5c1a7cbc444bfc8f3d77b07f46bbe/layout/generic/nsFrame.cpp#l1101

Comment 3

[Daniel Holbert [:dholbert]]

And ScrollAnchorContainer::FindFor() does absolutely have some cases where it returns null, so yeah, I think we need to be null-checking its return value before we dereference it.

(sorry for not catching this in review)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1520798: Null-check the result of ScrollAnchorContainer::FindFor, in DidSetComputedStyle. r?rhunt

Comment 5

[Daniel Holbert [:dholbert]]

I'm currently trying to come up with a testcase that triggers the crash, BTW.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5270d03a6d8
Null-check the result of ScrollAnchorContainer::FindFor, in DidSetComputedStyle. r=rhunt

Comment 7

[Daniel Holbert [:dholbert]]

I included a XUL testcase to trigger the "no scrollarea for viewport" scenario, but I suspect the users hitting this crash aren't using XUL to trigger it...

It'd be great to (privately) see some of the URLs that triggered the issue, to perhaps construct a non-XUL regression test.

Comment 8

[Daniel Holbert [:dholbert]]

(ni=marcia in case she can help with comment 7)

Comment 9

[Marcia Knous [:marcia]]

Here are the other URLs I see in these crashes:

• file:///tmp/scroll-test.xul
• https://svenssonshop.com/product-category/shoes/
• https://www.google.com/search?client=firefox-b-d&q=urcc

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Marcia Knous [:marcia - needinfo? me] from comment #9)

Here are the other URLs I see in these crashes:

• file:///tmp/scroll-test.xul

That one is me. :D

Comment 11

[Daniel Holbert [:dholbert]]

OK - for HTML, the necessary scenario for the crash is for the mutation to happen inside of a "position:fixed" element.

By default, the position:fixed element is parented on the viewport frame, without an ancestor scrollport. So ScrollAnchorContainer::FindFor on its descendant ends up returning nullptr, because nsLayoutUtils::GetNearestScrollableFrame returns nullptr.

(The nsLayoutUtils function accepts "SCROLLABLE_FIXEDPOS_FINDS_ROOT" as an optional flag to avoid returning null in this scenario, but we don't (& probably shouldn't) pass it that flag, so that clause doesn't help.)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1 (warning, may crash your content process)

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b5270d03a6d8

Comment 14

[Ryan Hunt [:rhunt]]

(In reply to Daniel Holbert [:dholbert] from comment #11)

OK - for HTML, the necessary scenario for the crash is for the mutation to
happen inside of a "position:fixed" element.

By default, the position:fixed element is parented on the viewport frame,
without an ancestor scrollport. So ScrollAnchorContainer::FindFor on its
descendant ends up returning nullptr, because
nsLayoutUtils::GetNearestScrollableFrame returns nullptr.

(The nsLayoutUtils function accepts "SCROLLABLE_FIXEDPOS_FINDS_ROOT" as an
optional flag to avoid returning null in this scenario, but we don't (&
probably shouldn't) pass it that flag, so that clause doesn't help.)

Thanks for looking into this! That makes total sense.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2ed817b7523
followup: add HTML crashtest for this bug (on top of earlier XUL crashtest). no review, testonly

Comment 16

[Daniel Holbert [:dholbert]]

Landed that^ crashtest followup with the HTML testcase that I attached earlier.

I verified that it passes in a build with this bug's fix, and fails (crashes) in the harness in a build with the fix reverted.

Comment 17

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/b2ed817b7523