Closed Bug 1381134 Opened 4 years ago Closed 3 years ago
Null deref crash [@ Get
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
There's no attached test-case, mind uploading it? Thanks!
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: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=946ed22cad04431c75ab5093989dfedf1bae5a3e&tochange=d1d47ba19ce9d46222030d491f9fe28dbf80be12 Which I assume is pointing at bug 1144096 as the culprit.
Given the regression range in comment 5, ni? mats.
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
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)
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+
Oh, please do add the crashtest(s) though.
Right, will do.
Pushed by firstname.lastname@example.org: 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.
Thank you Mats!
You need to log in before you can comment on or make changes to this bug.