1013743 - MutationObserver sees XBL anonymous content for <marquee>

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Comment 1

[Olli Pettay [:smaug]]

Hmm, has this regressed.

Comment 2

[Andrew McCreight [:mccr8]]

I don't know if that's related, but bholley did change marquee a few days ago in bug 1005552.

Comment 3

[Bobby Holley (:bholley)]

Thankfully, this isn't a security issue because the anonymous content is all in a protected scope, so wrappers will save us. We've almost certainly got a bug in the event re-targeting code, which I don't understand all that well. Smaug, any thoughts?

Comment 4

[Bobby Holley (:bholley)]

FWIW, I would guess that this regressed either in bug 825392 or bug 986730.

Comment 5

[Olli Pettay [:smaug]]

Oh, I think this isn't a regression after all. We just protect from NAC only.
And this has nothing to do with event re-targeting. MutationObservers don't have anything to do
with events.

Need to just figure out how this should work.
We do want MO to work inside Shadow DOM, even in content. and chrome code should be able to
use AC.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #5)
> Need to just figure out how this should work.
> We do want MO to work inside Shadow DOM, even in content.

Shadow DOM is tracked separately from AC.

> and chrome code
> should be able to
> use AC.

Yeah. I think we should do the same thing we do for NAC for AC-in-content, since the security wrappers make it inaccessible anyway.

Comment 7

[Olli Pettay [:smaug]]

Attachment: v1

Prevent XBL stuff to show up in normal mutation observing (except in chrome) and
handle Shadow DOM subtrees.

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

Comment 8

[Olli Pettay [:smaug]]

Attachment: v2

https://tbpl.mozilla.org/?tree=Try&rev=172b98644146

Make mutationobserver limited to the subtree of the (possibly transient) observer, so that changes in shadow DOM aren't propagated.
Limit XBL observing to chrome only.

Comment 9

[Olli Pettay [:smaug]]

Comment on attachment 8554175 [details] [diff] [review]
v2

William, would you be ok to review this. Jonas doesn't seem to have time atm.

Comment 10

[William Chen [:wchen]]

Comment on attachment 8554175 [details] [diff] [review]
v2

Review of attachment 8554175 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMMutationObserver.cpp
@@ +180,5 @@
> +  bool wantsChildList =
> +    ChildList() &&
> +    ((Subtree() && RegisterTarget()->SubtreeRoot() == parent->SubtreeRoot()) ||
> +     parent == Target());
> +  if (!wantsChildList || !IsObservable(aFirstNewContent)) {

This is pretty much the same as the check in nsMutationReceiver::ContentAppended, may be worthwhile to factor this out.

Comment 11

[Olli Pettay [:smaug]]

I was thinking that, but then realized one needs to go through all those nsIMutationObserver callback
cases always anyway, so perhaps not worth it.

Comment 12

[Olli Pettay [:smaug]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad2c849f0be

Comment 13

[Olli Pettay [:smaug]]

Looks like this causes causes localization_test.js to fail.

Kevin, based on https://github.com/mozilla-b2g/gaia/blame/master/apps/verticalhome/test/marionette/localization_test.js you know something about it.
I hope Gaia isn't relying on the broken behavior of MutationObservers where
MutationObserver can get MutationRecords for changes happening in the shadow DOM when 
observer is observing non-shadow-dom.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for Gij failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f5cef4614d

https://treeherder.mozilla.org/logviewer.html#?job_id=6345021&repo=mozilla-inbound

TEST-UNEXPECTED-FAIL | apps/verticalhome/test/marionette/localization_test.js | Vertical - Localization Menu option localization

Comment 15

[Staś Małolepszy :stas (OOO until Nov 4)]

(In reply to Olli Pettay [:smaug] (review overload. No more review requests before Feb 8, please) from comment #13)

> I hope Gaia isn't relying on the broken behavior of MutationObservers where
> MutationObserver can get MutationRecords for changes happening in the shadow
> DOM when observer is observing non-shadow-dom.

I think this might be the case :(  The l10n.js library sets up a single Mutation Observer for all of the DOM and translates elements when they're inserted or their data-l10n-id attribute changes.  IIRC, we don't do anything special for the shadow DOM but I'm needinfoing Zibi who knows this code better than I do.

The screenshot from the failing test is a good illustration of what's happening, I'll attach it in a second.

Comment 16

[Staś Małolepszy :stas (OOO until Nov 4)]

Attachment: Screenshot from the failing Marionette test

Comment 17

[Kevin Grandon :kgrandon]

(In reply to Olli Pettay [:smaug] (review overload. No more review requests before Feb 8, please) from comment #13)
> Looks like this causes causes localization_test.js to fail.
> 
> Kevin, based on
> https://github.com/mozilla-b2g/gaia/blame/master/apps/verticalhome/test/
> marionette/localization_test.js you know something about it.
> I hope Gaia isn't relying on the broken behavior of MutationObservers where
> MutationObserver can get MutationRecords for changes happening in the shadow
> DOM when 
> observer is observing non-shadow-dom.

Hmm, it probably is =/  I suppose we weren't aware of this broken behavior when we implemented this. Sadly if the test is failing, then this will also break the gaia UI, potentially in other places where we use web components as well.

How do we proceed here? Do we need to setup mutation observers for the shadow dom as well? Perhaps l10n.js should expose a method where we can pass in a shadow dom to be observed?

Looks like the current logic is here: https://github.com/mozilla-b2g/gaia/blob/d8d240663284fa40d191edcf612eab1cef73418c/shared/elements/gaia_menu/script.js#L31

Comment 18

[Olli Pettay [:smaug]]

Yeah, sorry that we had this wrong behavior.

> Perhaps l10n.js should expose a method where we can pass in a shadow dom to be observed?
That sounds like a reasonable API
(I think I discussed about something like that with someone on IRC couple of months ago.)

Comment 19

[Kevin Grandon :kgrandon]

Zibi - (In reply to Olli Pettay [:smaug] (review overload. No more review requests before Feb 8, please) from comment #18)
> > Perhaps l10n.js should expose a method where we can pass in a shadow dom to be observed?
> That sounds like a reasonable API
> (I think I discussed about something like that with someone on IRC couple of
> months ago.)

Zibi - let me know what you would like to do. I think having a way to hook into l10n.js and observer the shadow is probably the right thing to do for now. I'm not sure how well this meshes up with the longer term web components effort in gaia, but this seems like the easiest path forward.

I've filed bug 1130436 to track the l10n/gaia pieces of this.

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf] (PTO till Oct 14th)]

(In reply to Kevin Grandon :kgrandon from comment #19)
> Zibi - let me know what you would like to do. I think having a way to hook
> into l10n.js and observer the shadow is probably the right thing to do for
> now. I'm not sure how well this meshes up with the longer term web
> components effort in gaia, but this seems like the easiest path forward.

I believe we should stick to no-l10n in shadow-dom for now. Let's discuss it in the follow up bug?

Comment 21

[Olli Pettay [:smaug]]

Attachment: up-to-date (v3)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=636a2ef17862

(hopefully inbound wasn't broken when I pushed that)

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25db40a4dd8e

Comment 23

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/25db40a4dd8e