Closed Bug 1013743 Opened 6 years ago Closed 5 years ago

MutationObserver sees XBL anonymous content for <marquee>

Categories

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

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jruderman, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(4 files, 1 obsolete file)

Attached file testcase
No description provided.
Hmm, has this regressed.
I don't know if that's related, but bholley did change marquee a few days ago in bug 1005552.
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?
FWIW, I would guess that this regressed either in bug 825392 or bug 986730.
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.
(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.
Attached patch v1 (obsolete) — Splinter Review
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
Assignee: nobody → bugs
Flags: needinfo?(bugs)
Attached patch v2Splinter Review
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.
Attachment #8554048 - Attachment is obsolete: true
Flags: needinfo?(bugs)
Attachment #8554175 - Flags: review?(jonas)
Comment on attachment 8554175 [details] [diff] [review]
v2

William, would you be ok to review this. Jonas doesn't seem to have time atm.
Attachment #8554175 - Flags: review?(jonas) → review?(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.
Attachment #8554175 - Flags: review?(wchen) → review+
I was thinking that, but then realized one needs to go through all those nsIMutationObserver callback
cases always anyway, so perhaps not worth it.
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.
Flags: needinfo?(kgrandon)
Flags: needinfo?(stas)
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
(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.
Flags: needinfo?(stas) → needinfo?(gandalf)
(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
Flags: needinfo?(kgrandon)
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.)
Depends on: 1130436
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.
(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?
Flags: needinfo?(gandalf)
Attached patch up-to-date (v3)Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=636a2ef17862

(hopefully inbound wasn't broken when I pushed that)
https://hg.mozilla.org/mozilla-central/rev/25db40a4dd8e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1160110
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.