1381134 - Null deref crash [@ GetParentNode]

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Schwartzentruber (:truber)]

The attached testcase causes a null-deref in m-c rev 67cd1ee26f26.


==32513==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000028 (pc 0x7f6d282b331b bp 0x7ffdd1a32a10 sp 0x7ffdd1a32a10 T0)
==32513==The signal is caused by a READ memory access.
==32513==Hint: address points to the zero page.
    #0 0x7f6d282b331a in GetParentNode /home/worker/workspace/build/src/dom/base/nsINode.h:920:12
    #1 0x7f6d282b331a in nsContentUtils::ContentIsDescendantOf(nsINode const*, nsINode const*) /home/worker/workspace/build/src/dom/base/nsContentUtils.cpp:2566
    #2 0x7f6d2c7f2792 in nsCounterList::SetScope(nsCounterNode*) /home/worker/workspace/build/src/layout/base/nsCounterManager.cpp:151:10
    #3 0x7f6d2c7f2a5a in nsCounterList::RecalcAll() /home/worker/workspace/build/src/layout/base/nsCounterManager.cpp:169:5
    #4 0x7f6d2c7d777c in RecalcAll /home/worker/workspace/build/src/layout/base/nsCounterManager.cpp:252:13
    #5 0x7f6d2c7d777c in nsCSSFrameConstructor::RecalcQuotesAndCounters() /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:9069
    #6 0x7f6d2c726e45 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/PresShell.cpp:4228:26
    #7 0x7f6d2c69c7f5 in FlushPendingNotifications /home/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:587:5
    #8 0x7f6d2c69c7f5 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1923

Comment 1

[Emilio Cobos Álvarez [:emilio]]

There's no attached test-case, mind uploading it?

Thanks!

Comment 2

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

Yes, sorry!

Comment 3

[Emilio Cobos Álvarez [:emilio]]

So I took a look at this, and it's kinda nasty... So the column set frame is removing its next in flows.

Why we mutate the frame tree when we're doing reflow will remain a mistery to me, I guess...

Of course removing frames shouldn't affect the shape of the DOM, but it does (since we hold pseudo-elements off the container frame, which is kinda nasty on its own and I'd like to change).

Anyway, so we have a pseudo-frame which happens to be a ::after pseudo, and its parent is obviously null, because the container frame has been destroyed already, and deleting it's GenConPseudoProperty() unbinds the pseudo-content.

Since we have a null parent, we crash.

Short-term fix is probably adding a null-check. But there's some really messy stuff here. I wonder if we could stop mutating the frame tree from reflow...

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Mutating frame tree during reflow is very normal, because we can only know whether and where should we split a frame and create continuation only during reflow in many cases.

In general, when before a continuation is removed, its previous continuation should pull all of the children into its own child list, and we should reparent them at that time.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This testcase crashes with or without Stylo enabled. Adding a meta refresh to the testcase can make it reproduce more reliably.

Adjusting the test to use -moz-columns, I get the regression range below:
INFO: Last good revision: 946ed22cad04431c75ab5093989dfedf1bae5a3e (2016-03-12)
INFO: First bad revision: d1d47ba19ce9d46222030d491f9fe28dbf80be12 (2016-03-13)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=946ed22cad04431c75ab5093989dfedf1bae5a3e&tochange=d1d47ba19ce9d46222030d491f9fe28dbf80be12

Which I assume is pointing at bug 1144096 as the culprit.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Given the regression range in comment 5, ni? mats.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Testcase #2

The crash doesn't seem Grid-related per se.  Here's a slightly modified
version of the first test that crashes in a similar way but without
using display:grid.  It's more related to display:contents and I think
the first assertion indicates where it goes wrong:
ASSERTION: property should only be set on first continuation/ib-sibling: 'nsLayoutUtils::IsFirstContinuationOrIBSplitSibling(aOwnerFrame)', file layout/base/nsCSSFrameConstructor.cpp, line 6290

(Simply forcing |aOwnerFrame| to be the first continuation there seems
to fix the crash, fwiw.)

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix

This should fix it for now.  (I agree with your code comment though --
perhaps that's something we can do as part of bug 1400618?)

This patch also fixes bug 1251800, bug 1145931 and bug 1350372.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8917426 [details] [diff] [review]
fix

Review of attachment 8917426 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks for looking into it, I really never got around to do it properly.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Oh, please do add the crashtest(s) though.

Comment 11

[Mats Palmgren (inactive)]

Right, will do.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/981c4d1c7c12
Ensure we're using the correct frame for the :after/:before references.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa88f9fbee71
part 2 - Crashtests.

Comment 13

[Mats Palmgren (inactive)]

(I also pushed crashtests for bug 1145931 and bug 1350372.)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/981c4d1c7c12
https://hg.mozilla.org/mozilla-central/rev/fa88f9fbee71

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Thank you Mats!