Closed Bug 1381134 Opened 4 years ago Closed 3 years ago

Null deref crash [@ GetParentNode]


(Core :: Layout, defect, P2)

36 Branch



Tracking Status
firefox-esr52 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed


(Reporter: truber, Assigned: mats)



(Keywords: crash, csectype-nullptr, testcase)


(3 files)

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
Flags: in-testsuite?
Component: DOM → Layout
There's no attached test-case, mind uploading it?

Flags: needinfo?(jschwartzentruber)
Attached file testcase.html
Yes, sorry!
Flags: needinfo?(jschwartzentruber)
Priority: -- → P2
Flags: needinfo?(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...
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.
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:

Which I assume is pointing at bug 1144096 as the culprit.
Has Regression Range: --- → yes
Version: Trunk → 48 Branch
Given the regression range in comment 5, ni? mats.
Flags: needinfo?(mats)
Attached file 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.)
Assignee: nobody → mats
Flags: needinfo?(mats)
Attached patch fixSplinter Review
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.
Attachment #8917426 - Flags: review?(emilio)
OS: Unspecified → All
Hardware: Unspecified → All
Version: 48 Branch → 36 Branch
Comment on attachment 8917426 [details] [diff] [review]

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

r=me, thanks for looking into it, I really never got around to do it properly.
Attachment #8917426 - Flags: review?(emilio) → review+
Flags: needinfo?(emilio)
Oh, please do add the crashtest(s) though.
Right, will do.
Pushed by
Ensure we're using the correct frame for the :after/:before references.  r=emilio
part 2 - Crashtests.
(I also pushed crashtests for bug 1145931 and bug 1350372.)
Flags: in-testsuite? → in-testsuite+
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Thank you Mats!
You need to log in before you can comment on or make changes to this bug.