Closed Bug 1461749 Opened 6 years ago Closed 6 years ago

Crash [@ GetBoolFlag]

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Version:
59 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- disabled
firefox60 --- disabled
firefox61 --- disabled
firefox62 --- fixed

People

(Reporter: jkratzer, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Attachments

(3 files)

trigger.html
657 bytes, text/html
Details
prefs.js
11.34 KB, application/javascript
Details
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
MatsPalmgren_bugz
: review+
Details
Attached file trigger.html — 
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
Flags: in-testsuite?
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.
Flags: needinfo?(overholt)
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)
Attached file prefs.js — 
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.
Flags: needinfo?(jkratzer)
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? :-)
Flags: needinfo?(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
Blocks: shadowdom-layout
Flags: needinfo?(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 ecoal95@gmail.com:
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.
https://hg.mozilla.org/mozilla-central/rev/5024a6a9176a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
status-firefox60: --- → disabled
status-firefox61: --- → disabled
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → disabled
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: