Closed
Bug 1414303
Opened 7 years ago
Closed 7 years ago
crash near null in [@ nsContentUtils::ContentIsDescendantOf]
Categories
(Core :: Layout, defect, P3)
Tracking
()
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?
Reporter | ||
Comment 1•7 years ago
|
||
Reporter | ||
Comment 2•7 years ago
|
||
Comment 4•7 years ago
|
||
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
status-firefox56:
--- → wontfix
status-firefox57:
--- → wontfix
status-firefox-esr52:
--- → wontfix
Priority: -- → P3
Version: 58 Branch → 28 Branch
Assignee | ||
Comment 5•7 years ago
|
||
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.
Comment 6•7 years ago
|
||
status-firefox59:
--- → ?
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(emilio)
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 10•7 years ago
|
||
(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.
Comment 11•7 years ago
|
||
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
Updated•7 years ago
|
Keywords: regression
Assignee | ||
Comment 12•7 years ago
|
||
(This is not a regression, as far as I can tell)
Keywords: regression
Assignee | ||
Comment 13•7 years ago
|
||
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
Comment 14•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•7 years ago
|
status-firefox60:
--- → disabled
status-firefox61:
--- → disabled
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.
Description
•