Botond Ballo [:botond] Reporter
	Description • 10 years ago

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)

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 10 years ago

This isn't by chance using web components, is it?

	Botond Ballo [:botond] Reporter
	Comment 2 • 10 years ago

(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).

	:garvan
	Comment 3 • 10 years ago

I don't know anything helpful (or who would be involved in that setting).

	Botond Ballo [:botond] Reporter
	Comment 4 • 10 years ago

CC'ing Francisco, he might be able to answer Boris' question in comment 1.

	Cameron McCormack (:heycam) Assignee
	Comment 5 • 10 years ago

There's a web component registered for the <gaia-switch> element which is the thing you tap.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 10 years ago

I'm totally willing to believe web components screw this stuff up.  :(

	Cameron McCormack (:heycam) Assignee
	Comment 7 • 10 years ago

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).

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 9 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 14 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=1e59d16f3e8b

	Cameron McCormack (:heycam) Assignee
	Comment 15 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 10 years ago

Comment on attachment 8435604 [details] [diff] [review]
Part 3: Crashtest.

r=me

	William Chen [:wchen]
	Comment 19 • 10 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 10 years ago

Could we make the getter on ShadowRoot be called Host then (since it's never null) and have it return Element*?

	William Chen [:wchen]
	Comment 21 • 10 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 10 years ago

> 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.

	Cameron McCormack (:heycam) Assignee
	Comment 23 • 10 years ago

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...

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 10 years ago

> does that mean it's possible for a non-ShadowRoot DocumentFragment to have a shadow tree
> attached to it

It's not possible.

	Cameron McCormack (:heycam) Assignee
	Comment 25 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/33ce82daa572
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6cd0caaf182
https://hg.mozilla.org/integration/mozilla-inbound/rev/a98e6f80344c

	Ed Morley [:emorley]
	Comment 26 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/33ce82daa572
https://hg.mozilla.org/mozilla-central/rev/b6cd0caaf182
https://hg.mozilla.org/mozilla-central/rev/a98e6f80344c