Closed Bug 1414303 Opened 7 years ago Closed 7 years ago

crash near null in [@ nsContentUtils::ContentIsDescendantOf]

Categories

(Core :: Layout, defect, P3)

28 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- disabled
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- disabled
firefox61 --- disabled
firefox62 --- fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase)

Attachments

(3 files)

==12762==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000028 (pc 0x7f0fdc86cb8b bp 0x7ffc69336cb0 sp 0x7ffc69336cb0 T0) ==12762==The signal is caused by a READ memory access. ==12762==Hint: address points to the zero page. #0 0x7f0fdc86cb8a in GetParentNode /src/dom/base/nsINode.h:929:12 #1 0x7f0fdc86cb8a in nsContentUtils::ContentIsDescendantOf(nsINode const*, nsINode const*) /src/dom/base/nsContentUtils.cpp:2646 #2 0x7f0fe0e7d292 in nsCounterList::SetScope(nsCounterNode*) /src/layout/base/nsCounterManager.cpp:152:10 #3 0x7f0fe0e7d56a in nsCounterList::RecalcAll() /src/layout/base/nsCounterManager.cpp:170:5 #4 0x7f0fe0e61ebc in RecalcAll /src/layout/base/nsCounterManager.cpp:253:13 #5 0x7f0fe0e61ebc in nsCSSFrameConstructor::RecalcQuotesAndCounters() /src/layout/base/nsCSSFrameConstructor.cpp:9087 #6 0x7f0fe0dab1d8 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /src/layout/base/PresShell.cpp:4221:26 #7 0x7f0fe0d1ee62 in FlushPendingNotifications /src/obj-firefox/dist/include/nsIPresShell.h:581:5 #8 0x7f0fe0d1ee62 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:1921 #9 0x7f0fe0d2ca6b in TickDriver /src/layout/base/nsRefreshDriver.cpp:336:13 #10 0x7f0fe0d2ca6b in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /src/layout/base/nsRefreshDriver.cpp:306 #11 0x7f0fe0d2c754 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:327:5 #12 0x7f0fe0d2ecbb in RunRefreshDrivers /src/layout/base/nsRefreshDriver.cpp:769:5 #13 0x7f0fe0d2ecbb in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:682 #14 0x7f0fe0d2a467 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() /src/layout/base/nsRefreshDriver.cpp:528:20 #15 0x7f0fd9e7f086 in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1037:14 #16 0x7f0fd9e99548 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:513:10 #17 0x7f0fdac6bb21 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21 #18 0x7f0fdabcc24b in RunInternal /src/ipc/chromium/src/base/message_loop.cc:326:10 #19 0x7f0fdabcc24b in RunHandler /src/ipc/chromium/src/base/message_loop.cc:319 #20 0x7f0fdabcc24b in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299 #21 0x7f0fe063189f in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27 #22 0x7f0fe497b751 in nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30 #23 0x7f0fe4b7259b in XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4675:22 #24 0x7f0fe4b74165 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4837:8 #25 0x7f0fe4b75516 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4932:21 #26 0x4ec4ec in do_main /src/browser/app/nsBrowserApp.cpp:231:22 #27 0x4ec4ec in main /src/browser/app/nsBrowserApp.cpp:304 #28 0x7f0ff7be482f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291 #29 0x41dbc8 in _start (firefox+0x41dbc8)
Flags: in-testsuite?
Attached file testcase.html (deleted) —
Attached file prefs.js (deleted) —
INFO: Last good revision: 2581b84e0ca1 (2013-12-02) INFO: First bad revision: 8648aa476eef (2013-12-03) INFO: Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2581b84e0ca1&tochange=8648aa476eef
Blocks: 806506
Has Regression Range: --- → yes
Priority: -- → P3
Version: 58 Branch → 28 Branch
So this is because counters are using the light tree, while they should probably use the frame tree. Indeed there's an XXX comment related to this: https://searchfox.org/mozilla-central/rev/51cd1093c5c2113e7b041e8cc5c9bf2186abda13/layout/base/nsGenConList.cpp#102 Unfortunately it's not a matter of just switching there, because we compute counters before inserting the frames in the frame tree. That's fixable though.
Depends on: 1427635
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Comment on attachment 8975539 [details] Bug 1414303: Make nsLayoutUtils::CompareTreePosition handle Shadow DOM in a sensible way. https://reviewboard.mozilla.org/r/243790/#review249880 ::: commit-message-6cecf:1 (Diff revision 1) > +Bug 1414303: Make nsLayoutUtils::CompareTreePosition handle Shadow DOM in a sensible way. r?xidorn Maybe add a comment to the declaration of this method mentioning that it works on flattened tree... maybe doesn't matter a lot. Your call. ::: commit-message-6cecf:3 (Diff revision 1) > +We probably need more fixes for counters and Shadow DOM. The spec only mentions > +"document order", and this is the most reasonable thing to do accounting for > +shadow DOM in that regard... Intuitively I thought that counters from host shouldn't be inherited into the shadow tree... but the spec says CSS uses flattened element tree for all purposes after Selectors, so sounds like counters should be inherited... I don't know... probably worth a spec issue to clarify at least.
Attachment #8975539 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9) > Comment on attachment 8975539 [details] > Bug 1414303: Make nsLayoutUtils::CompareTreePosition handle Shadow DOM in a > sensible way. > > https://reviewboard.mozilla.org/r/243790/#review249880 > > ::: commit-message-6cecf:1 > (Diff revision 1) > > +Bug 1414303: Make nsLayoutUtils::CompareTreePosition handle Shadow DOM in a sensible way. r?xidorn > > Maybe add a comment to the declaration of this method mentioning that it > works on flattened tree... maybe doesn't matter a lot. Your call. It doesn't work on the flattened tree, fwiw, since it doesn't follow <slot>s. It follows some sort of "composed document" order, which was closer to what the spec said. > ::: commit-message-6cecf:3 > (Diff revision 1) > > +We probably need more fixes for counters and Shadow DOM. The spec only mentions > > +"document order", and this is the most reasonable thing to do accounting for > > +shadow DOM in that regard... > > Intuitively I thought that counters from host shouldn't be inherited into > the shadow tree... but the spec says CSS uses flattened element tree for all > purposes after Selectors, so sounds like counters should be inherited... > > I don't know... probably worth a spec issue to clarify at least. Yeah, wouldn't surprise me if each browser just did whatever it was easier with his architecture... I filed https://github.com/w3c/csswg-drafts/issues/2679 to make the spec clear about what is it expected to happen.
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/06ef43a833ce Make nsLayoutUtils::CompareTreePosition handle Shadow DOM in a sensible way. r=xidorn
(This is not a regression, as far as I can tell)
Keywords: regression
After talking with Sylvestre, it can be considered a regression in the sense of "it never crashed before, it crashes now with Shadow DOM". I don't really want to argue about that so fine :P
Keywords: regression
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: