Closed
Bug 1381134
Opened 7 years ago
Closed 7 years ago
Null deref crash [@ GetParentNode]
Categories
(Core :: Layout, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: truber, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: crash, csectype-nullptr, testcase)
Attachments
(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?
Updated•7 years ago
|
Component: DOM → Layout
Comment 1•7 years ago
|
||
There's no attached test-case, mind uploading it? Thanks!
Flags: needinfo?(jschwartzentruber)
Updated•7 years ago
|
Priority: -- → P2
Updated•7 years ago
|
Flags: needinfo?(emilio)
Comment 3•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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.
Has Regression Range: --- → yes
status-firefox56:
--- → wontfix
status-firefox57:
--- → affected
status-firefox58:
--- → affected
status-firefox-esr52:
--- → wontfix
Version: Trunk → 48 Branch
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Comment 9•7 years ago
|
||
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.
Attachment #8917426 -
Flags: review?(emilio) → review+
Updated•7 years ago
|
Flags: needinfo?(emilio)
Comment 10•7 years ago
|
||
Oh, please do add the crashtest(s) though.
Assignee | ||
Comment 11•7 years ago
|
||
Right, will do.
Updated•7 years ago
|
Comment 12•7 years ago
|
||
Pushed by mpalmgren@mozilla.com: 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.
Assignee | ||
Comment 13•7 years ago
|
||
(I also pushed crashtests for bug 1145931 and bug 1350372.)
Flags: in-testsuite? → in-testsuite+
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/981c4d1c7c12 https://hg.mozilla.org/mozilla-central/rev/fa88f9fbee71
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 15•7 years ago
|
||
Thank you Mats!
You need to log in
before you can comment on or make changes to this bug.
Description
•