crash near null in [@ nsContentUtils::ContentIsDescendantOf]

RESOLVED FIXED in Firefox 62

Status

()

P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: tsmith, Assigned: emilio)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {crash, regression, testcase})

28 Branch
mozilla62
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox-esr60 disabled, firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 disabled, firefox61 disabled, firefox62 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

a year ago
==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

a year ago
Created attachment 8925059 [details]
testcase.html
(Reporter)

Comment 2

a year ago
Created attachment 8925060 [details]
prefs.js
(Assignee)

Comment 3

a year ago
Looks pretty similar to bug 1414100...
Blocks: 1405937
See Also: → bug 1414100
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-firefox58: affected → fix-optional
status-firefox-esr52: --- → wontfix
Priority: -- → P3
Version: 58 Branch → 28 Branch
(Assignee)

Comment 5

a year 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.
(Assignee)

Updated

a year ago
Depends on: 1427635
(Assignee)

Updated

9 months ago
Flags: needinfo?(emilio)
(Assignee)

Updated

9 months ago
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Comment hidden (mozreview-request)

Comment 9

9 months 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

9 months 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

9 months 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
Keywords: regression
(Assignee)

Comment 12

9 months ago
(This is not a regression, as far as I can tell)
Keywords: regression
(Assignee)

Comment 13

9 months 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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/06ef43a833ce
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
status-firefox59: ? → wontfix
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.