Closed Bug 1461749 Opened 5 years ago Closed 5 years ago
Crash [@ Get
657 bytes, text/html
Bug 1461749: Fix slow path for finding the next sibling frame for appending in presence of Shadow DOM.
59 bytes, text/x-review-board-request
Testcase found while fuzzing mozilla-central rev cf3ee14023483cbbb57129479537c713e22c1980. ==10111==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000001c (pc 0x7f2c228f7856 bp 0x7ffdaea40050 sp 0x7ffdaea40040 T0) ==10111==The signal is caused by a READ memory access. ==10111==Hint: address points to the zero page. #0 0x7f2c228f7855 in GetBoolFlag /builds/worker/workspace/build/src/dom/base/nsINode.h:1644:12 #1 0x7f2c228f7855 in GetParent /builds/worker/workspace/build/src/dom/base/nsINode.h:999 #2 0x7f2c228f7855 in mozilla::dom::ExplicitChildIterator::Seek(nsIContent const*) /builds/worker/workspace/build/src/dom/base/ChildIterator.cpp:177 #3 0x7f2c2795d231 in nsCSSFrameConstructor::ContentAppended(nsIContent*, nsCSSFrameConstructor::InsertionKind) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:7152:10 #4 0x7f2c278eddaf in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:1402:27 #5 0x7f2c278fb105 in mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:3001:9 #6 0x7f2c278b2e1d in ProcessPendingRestyles /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:3078:3 #7 0x7f2c278b2e1d in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4315 #8 0x7f2c27842f76 in FlushPendingNotifications /builds/worker/workspace/build/src/layout/base/nsIPresShell.h:591:5 #9 0x7f2c27842f76 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1925 #10 0x7f2c27852380 in TickDriver /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:337:13 #11 0x7f2c27852380 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:307 #12 0x7f2c27851f46 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:329:5 #13 0x7f2c27854cbe in RunRefreshDrivers /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:770:5 #14 0x7f2c27854cbe in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:683 #15 0x7f2c278548be in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:584:9 #16 0x7f2c280fabff in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /builds/worker/workspace/build/src/layout/ipc/VsyncChild.cpp:68:16 #17 0x7f2c20e82a6d in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:167:20 #18 0x7f2c20d46e2d in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1988:28 #19 0x7f2c2085ffce in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2136:25 #20 0x7f2c2085cf96 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2066:17 #21 0x7f2c2085e74c in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1912:5 #22 0x7f2c2085eda8 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1945:15 #23 0x7f2c1f96dba3 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1090:14 #24 0x7f2c1f989770 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:519:10 #25 0x7f2c20867c6a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21 #26 0x7f2c207bb959 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10 #27 0x7f2c207bb959 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319 #28 0x7f2c207bb959 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299 #29 0x7f2c272f5d5a in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27 #30 0x7f2c2b55d49b in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:893:22 #31 0x7f2c207bb959 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10 #32 0x7f2c207bb959 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319 #33 0x7f2c207bb959 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299 #34 0x7f2c2b55ce60 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:719:34 #35 0x4f50dc in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30 #36 0x4f50dc in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:282 #37 0x7f2c3f5f582f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
Seems like a nullptr crash at https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/dom/base/ChildIterator.cpp#177, seems like we don't have a null check there, but the underlying issue may be more involved than a simple check... Over to Andrew to find an owner.
Priority: -- → P1
Apologies for the random bug but at least it's got a nice STR :)
Flags: needinfo?(overholt) → needinfo?(hsivonen)
No crash in a non-ASAN build.
I can't repro in a normal debug build or in an ASAN debug build on Linux. Are there steps to reproduce that I should be aware of beyond loading the attachment here?
Flags: needinfo?(hsivonen) → needinfo?(jkratzer)
This crashes for me on both asan and opt builds under Ubuntu 16.04. I've attached the prefs file here. Let me know if you still have trouble reproducing it.
Thanks. I was able to repro with the attached prefs (with the proxy-related prefs removed). The problem is that https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7152 tries to seek with a null last child. Over to layout, because I don't know what the right fix is in case of generated content without any non-generated content. Specifically, if the line iter.Seek(insertion.mContainer->GetLastChild()); is skipped in case of GetLastChild() returning nullptr, the line nextSibling = FindNextSibling(iter, unused); runs into an assertion in ExplicitChildIterator::Get().
Component: DOM → Layout
Frame constructor and generated content => Emilio? :-)
Oh, and display: contents, fantabulous. We shouldn't have generated content with no generated content with no generated content... I'll take a look.
It even has Shadow DOM!
Assignee: nobody → emilio
Comment on attachment 8980000 [details] Bug 1461749: Fix slow path for finding the next sibling frame for appending in presence of Shadow DOM. https://reviewboard.mozilla.org/r/246180/#review252328 Seems reasonable. Did you forget to "hg add" the test file? It's missing in MozReview... BTW, do we still have reftests for DOM changes in trees with shadow DOM, display:contents, ::before, ::after, etc? I wrote a test for that when I originally implemented display:contents: https://hg.mozilla.org/mozilla-central/file/387293/layout/reftests/css-display/display-contents-shadow-dom-1.html but I can't seem to find it there anymore... do you know if it was renamed or something? We really need an extensive reftest that checks that the frames for the inserted node ends up in the right place...
Attachment #8980000 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #11) > Comment on attachment 8980000 [details] > Bug 1461749: Fix slow path for finding the next sibling frame for appending > in presence of Shadow DOM. > > https://reviewboard.mozilla.org/r/246180/#review252328 > > Seems reasonable. > Did you forget to "hg add" the test file? It's missing in MozReview... > > BTW, do we still have reftests for DOM changes in trees with shadow DOM, > display:contents, ::before, ::after, etc? > I wrote a test for that when I originally implemented display:contents: > https://hg.mozilla.org/mozilla-central/file/387293/layout/reftests/css- > display/display-contents-shadow-dom-1.html > but I can't seem to find it there anymore... do you know if it was renamed > or something? Yeah, it was disabled when <content> in bug 1418002, and I think I removed it later since it was effectively dead. Though the ref is still there... It may be worth to resurrect it and rewrite it to use <slot> instead, though the model with <slot>s is fairly different...
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/5024a6a9176a Fix slow path for finding the next sibling frame for appending in presence of Shadow DOM. r=mats
Oh, I remember why I removed it instead of trying to convert it. All the shadow hosts in the test-case were fieldsets, which are not even a valid shadow host anymore.
The file I linked above have no fieldsets afaict. In any case, it would be good to have a reftest like that to test the various paths here...
Oh, sorry, for some reason I looked at layout/reftests/forms/legend/shadow-dom.html, which was also disabled and removed at the same time. I agree... I'll repurpose bug 1421540 for that.
You need to log in before you can comment on or make changes to this bug.