Assertion failure in AncestorFilter::AssertHasAllAncestors()

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: botond, Assigned: heycam)

Tracking

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

STR:
  1. Build B2G with today's gecko in debug mode.
  2. Run the First Time Experience.
  3. On the Geolocation screen, disable geolocation.

The tap to disable geolocation triggers the following assertion failure:

Program received signal SIGSEGV, Segmentation fault.
0xb57643cc in AncestorFilter::AssertHasAllAncestors (this=0xbedd41a0, aElement=<optimized out>) at ../../../../refactoring/layout/style/nsCSSRuleProcessor.cpp:3620
3620        MOZ_ASSERT(mElements.Contains(cur));
(gdb) i s
#0  0xb57643cc in AncestorFilter::AssertHasAllAncestors (this=0xbedd41a0, aElement=<optimized out>) at ../../../../refactoring/layout/style/nsCSSRuleProcessor.cpp:3620
#1  0xb5766e2a in RuleHash::EnumerateAllRules (this=0xb2eda140, aElement=0xb210d2e0, aData=0xbedd3c34, aNodeContext=...) at ../../../../refactoring/layout/style/nsCSSRuleProcessor.cpp:770
#2  0xb5767dac in nsCSSRuleProcessor::RulesMatching (this=<optimized out>, aData=0xbedd3c34) at ../../../../refactoring/layout/style/nsCSSRuleProcessor.cpp:2480
#3  0xb57ce6e4 in EnumRulesMatching<ElementRuleProcessorData> (aProcessor=<optimized out>, aData=<optimized out>) at ../../../../refactoring/layout/style/nsStyleSet.cpp:671
#4  0xb57d1180 in nsStyleSet::FileRules (this=0xb2125180, aCollectorFunc=0xb57ce6dd <EnumRulesMatching<ElementRuleProcessorData>(nsIStyleRuleProcessor*, void*)>, aData=0xbedd3c34, aElement=0xb210d2e0, aRuleWalker=0xbedd3c28)
    at ../../../../refactoring/layout/style/nsStyleSet.cpp:976
#5  0xb57d1ae2 in nsStyleSet::ResolveStyleFor (this=0xb2125180, aElement=0xb210d2e0, aParentContext=0xb1741338, aTreeMatchContext=...) at ../../../../refactoring/layout/style/nsStyleSet.cpp:1206
#6  0xb5805552 in mozilla::ElementRestyler::RestyleSelf (this=0xbedd3da4, aSelf=0xb16c7928, aRestyleHint=eRestyle_Subtree) at ../../../../refactoring/layout/base/RestyleManager.cpp:2462
#7  0xb5805978 in mozilla::ElementRestyler::Restyle (this=0xbedd3da4, aRestyleHint=eRestyle_Subtree) at ../../../../refactoring/layout/base/RestyleManager.cpp:2306
#8  0xb5806098 in mozilla::ElementRestyler::RestyleContentChildren (this=0xbedd3eec, aParent=<optimized out>, aChildRestyleHint=eRestyle_Subtree) at ../../../../refactoring/layout/base/RestyleManager.cpp:2844
#9  0xb5806160 in mozilla::ElementRestyler::RestyleChildren (this=0xbedd3eec, aChildRestyleHint=eRestyle_Subtree) at ../../../../refactoring/layout/base/RestyleManager.cpp:2577
#10 0xb5805994 in mozilla::ElementRestyler::Restyle (this=0xbedd3eec, aRestyleHint=eRestyle_Subtree) at ../../../../refactoring/layout/base/RestyleManager.cpp:2310
#11 0xb5806064 in mozilla::ElementRestyler::RestyleContentChildren (this=0xbedd3ff8, aParent=<optimized out>, aChildRestyleHint=0) at ../../../../refactoring/layout/base/RestyleManager.cpp:2833
#12 0xb5806160 in mozilla::ElementRestyler::RestyleChildren (this=0xbedd3ff8, aChildRestyleHint=0) at ../../../../refactoring/layout/base/RestyleManager.cpp:2577
#13 0xb5805994 in mozilla::ElementRestyler::Restyle (this=0xbedd3ff8, aRestyleHint=eRestyle_Self) at ../../../../refactoring/layout/base/RestyleManager.cpp:2310
#14 0xb5805ada in mozilla::RestyleManager::ComputeStyleChangeFor (this=0xb2f14f00, aFrame=<optimized out>, aChangeList=0xbedd41fc, aMinChange=0, aRestyleTracker=..., aRestyleDescendants=false)
    at ../../../../refactoring/layout/base/RestyleManager.cpp:2958
#15 0xb5805efe in mozilla::RestyleManager::RestyleElement (this=0xb2f14f00, aElement=0xb1c941d0, aPrimaryFrame=0xb1c86eb8, aMinHint=0, aRestyleTracker=..., aRestyleDescendants=false)
    at ../../../../refactoring/layout/base/RestyleManager.cpp:866
#16 0xb58062b0 in mozilla::RestyleTracker::ProcessOneRestyle (this=0xb2f14f28, aElement=0xb1c941d0, aRestyleHint=eRestyle_Self, aChangeHint=0) at ../../../../refactoring/layout/base/RestyleTracker.cpp:123
#17 0xb5806916 in mozilla::RestyleTracker::DoProcessRestyles (this=0xb2f14f28) at ../../../../refactoring/layout/base/RestyleTracker.cpp:205
#18 0xb5805d66 in ProcessRestyles (this=0xb2f14f28) at ../../../../refactoring/layout/base/RestyleTracker.h:273
#19 mozilla::RestyleManager::ProcessPendingRestyles (this=0xb2f14f00) at ../../../../refactoring/layout/base/RestyleManager.cpp:1442
#20 0xb57ef9a4 in PresShell::FlushPendingNotifications (this=0xb2f5a680, aFlush=...) at ../../../../refactoring/layout/base/nsPresShell.cpp:4052
#21 0xb57f5ea4 in nsRefreshDriver::Tick (this=0xb2f150a0, aNowEpoch=<optimized out>, aNowTime=...) at ../../../../refactoring/layout/base/nsRefreshDriver.cpp:1162
#22 0xb57f65e0 in mozilla::RefreshDriverTimer::Tick (this=0xb28ad540) at ../../../../refactoring/layout/base/nsRefreshDriver.cpp:162
#23 0xb4a1a896 in nsTimerImpl::Fire (this=0xb2336f10) at ../../../../refactoring/xpcom/threads/nsTimerImpl.cpp:608
#24 0xb4a1aa20 in nsTimerEvent::Run (this=0xb28d6650) at ../../../../refactoring/xpcom/threads/nsTimerImpl.cpp:701
#25 0xb4a17e12 in ProcessNextEvent (aResult=0xbedd4d17, aMayWait=false, this=0xb3a47880) at ../../../../refactoring/xpcom/threads/nsThread.cpp:766
#26 nsThread::ProcessNextEvent (this=0xb3a47880, aMayWait=<optimized out>, aResult=0xbedd4d17) at ../../../../refactoring/xpcom/threads/nsThread.cpp:685
#27 0xb49cec9c in NS_ProcessNextEvent (thread=0xb3a47880, mayWait=<optimized out>) at ../../../../refactoring/xpcom/glue/nsThreadUtils.cpp:263
#28 0xb4bd3ce8 in mozilla::ipc::MessagePump::Run (this=0xb3a01b80, aDelegate=0xbedd4e70) at ../../../../refactoring/ipc/glue/MessagePump.cpp:95
#29 0xb4bc0aaa in MessageLoop::RunInternal (this=0xbedd4e70) at ../../../../refactoring/ipc/chromium/src/base/message_loop.cc:229
#30 0xb4bc0ac2 in RunHandler (this=0xbedd4e70) at ../../../../refactoring/ipc/chromium/src/base/message_loop.cc:222
#31 MessageLoop::Run (this=0xbedd4e70) at ../../../../refactoring/ipc/chromium/src/base/message_loop.cc:196
#32 0xb51f2cde in nsBaseAppShell::Run (this=0xb2e6c880) at ../../../../refactoring/widget/xpwidgets/nsBaseAppShell.cpp:164
#33 0xb5ac5eea in XRE_RunAppShell () at ../../../../refactoring/toolkit/xre/nsEmbedFunctions.cpp:690
#34 0xb4bd3dfe in mozilla::ipc::MessagePumpForChildProcess::Run (this=0xb3a01b80, aDelegate=0xbedd4e70) at ../../../../refactoring/ipc/glue/MessagePump.cpp:253
#35 0xb4bc0aaa in MessageLoop::RunInternal (this=0xbedd4e70) at ../../../../refactoring/ipc/chromium/src/base/message_loop.cc:229
#36 0xb4bc0ac2 in RunHandler (this=0xbedd4e70) at ../../../../refactoring/ipc/chromium/src/base/message_loop.cc:222
#37 MessageLoop::Run (this=0xbedd4e70) at ../../../../refactoring/ipc/chromium/src/base/message_loop.cc:196
#38 0xb5ac5db0 in XRE_InitChildProcess (aArgc=<optimized out>, aArgv=<optimized out>, aProcess=<optimized out>) at ../../../../refactoring/toolkit/xre/nsEmbedFunctions.cpp:527
#39 0x00008862 in main (argc=7, argv=0xbedd5964) at ../../../../refactoring/ipc/app/MozillaRuntimeMain.cpp:149
(gdb)
This isn't by chance using web components, is it?
(In reply to Boris Zbarsky [:bz] from comment #1)
> This isn't by chance using web components, is it?

CC'ing Garvan, he might know (or know who might know).
I don't know anything helpful (or who would be involved in that setting).
CC'ing Francisco, he might be able to answer Boris' question in comment 1.
There's a web component registered for the <gaia-switch> element which is the thing you tap.
I'm totally willing to believe web components screw this stuff up.  :(
In the FTU app's index.html, we have:

          <ul>
            <li>
              <gaia-switch id="geolocation-switch" name="geolocation.enabled" checked>
                <label data-l10n-id="geolocationCheckbox"></label>
              </gaia-switch>
            </li>
          </ul>

We're restyling the <label> in the document.  But the ancestor filter contains a <label> (and its child <span>) from a shadow tree (the ancestor filter's <label> element has a ShadowRoot parent).
So TreeMatchContext::InitAncestors uses GetParentNode() to walk up the tree.  So if we started the restyle somewhere in the anon content, we only put anon things in there (because for shadow DOM, unlike XBL, .parentNode doesn't cross back into the light DOM), but then we walked down the _frame_ tree during the restyle and into light content.

All of which is to say we need to teach InitAncestors (and probably InitStyleScopes) about going from kids of a ShadowRoot to the shadow host.
But will putting those flattened tree ancestors in the AncestorFilter affect the correctness of selector matching?  Normally you don't want to consider ancestors in the light tree when determining whether a selector matches a node in the shadow tree.
AncestorFilter is an optimization.  We ask it whether we're guaranteed that we have no ancestors with a given class/id/etc.

So correctness requires that any actual ancestors be in the ancestor filter.  But you could stick whatever else you want in there; it would just mean that sometimes we end up not being to optimize out the SelectorMatchesTree tree-walk when we actually could have.
Status: NEW → ASSIGNED
QA Contact: cam
Assignee: nobody → cam
QA Contact: cam
Attachment #8435604 - Flags: review?(bzbarsky)
One thing that still doesn't work is using a <style scoped> as a child of a ShadowRoot, since we don't stick the ElementIsStyleScopeRoot flag on the ShadowRoot.  Filed bug 1021572 for that.
Blocks: 1003294
Comment on attachment 8435602 [details] [diff] [review]
Part 1: Make AncestorFilter and the TreeMatchContext style scope list include ancestors from following ShadowRoots up to the light tree.

>+    if (host && host->IsElement()) {

It is possible to have a shadowroot with no host, or with a non-element host?

r=me
Attachment #8435602 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(wchen)
Comment on attachment 8435603 [details] [diff] [review]
Part 2: Set content bits on elements in shadow trees indicating whether they are in a style scope from the light tree.

Maybe make those methods protected class statics on Element so you don't have to duplicate them?

r=me
Attachment #8435603 - Flags: review?(bzbarsky) → review+
Comment on attachment 8435604 [details] [diff] [review]
Part 3: Crashtest.

r=me
Attachment #8435604 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #16)
> It is possible to have a shadowroot with no host, or with a non-element host?
ShadowRoot should always have a non-null element host.
Flags: needinfo?(wchen)
Could we make the getter on ShadowRoot be called Host then (since it's never null) and have it return Element*?
(In reply to Boris Zbarsky [:bz] from comment #20)
> Could we make the getter on ShadowRoot be called Host then (since it's never
> null) and have it return Element*?

GetHost() lives on document fragment (it's defined in the DOM spec) and most of the time a DocumentFragment will not have a host. We certainly could add Host() to ShadowRoot if we wanted to.
> GetHost() lives on document fragment (it's defined in the DOM spec)

Ah, I see.  Can we at least make it return Element* if that's what it always has?

And yeah, a Host() on ShadowRoot might be nice.
wchen, does that mean it's possible for a non-ShadowRoot DocumentFragment to have a shadow tree attached to it?  Can a shadow tree be attached to another ShadowRoot?  At the moment I'm only looking at shadow trees on Elements, but given the getter lives on DocumentFragment...
> does that mean it's possible for a non-ShadowRoot DocumentFragment to have a shadow tree
> attached to it

It's not possible.
You need to log in before you can comment on or make changes to this bug.