Closed
Bug 1013743
Opened 7 years ago
Closed 6 years ago
MutationObserver sees XBL anonymous content for <marquee>
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jruderman, Assigned: smaug)
References
Details
(Keywords: testcase)
Attachments
(4 files, 1 obsolete file)
639 bytes,
text/html
|
Details | |
14.05 KB,
patch
|
wchen
:
review+
|
Details | Diff | Splinter Review |
4.48 KB,
image/png
|
Details | |
14.27 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
Hmm, has this regressed.
Comment 2•7 years ago
|
||
I don't know if that's related, but bholley did change marquee a few days ago in bug 1005552.
Comment 3•7 years ago
|
||
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•7 years ago
|
||
FWIW, I would guess that this regressed either in bug 825392 or bug 986730.
Assignee | ||
Comment 5•7 years ago
|
||
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•7 years ago
|
||
(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.
Assignee | ||
Comment 7•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(bugs)
Assignee | ||
Comment 8•6 years ago
|
||
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)
Assignee | ||
Comment 9•6 years ago
|
||
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 10•6 years ago
|
||
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+
Assignee | ||
Comment 11•6 years ago
|
||
I was thinking that, but then realized one needs to go through all those nsIMutationObserver callback cases always anyway, so perhaps not worth it.
Assignee | ||
Comment 12•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad2c849f0be
Assignee | ||
Comment 13•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(stas)
Comment 14•6 years ago
|
||
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•6 years ago
|
||
(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)
Comment 16•6 years ago
|
||
Comment 17•6 years ago
|
||
(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)
Assignee | ||
Comment 18•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
(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)
Assignee | ||
Comment 21•6 years ago
|
||
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: 6 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•2 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•