Closed Bug 986730 Opened 7 years ago Closed 6 years ago

Move all anonymous content into the XBL scope

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox31 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(6 files)

Here's the result of the discussion I had with smaug:

(1) rename NODE_IS_IN_ANONYMOUS_SUBTREE to NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE
(2) introduce NODE_IS_IN_ANONYMOUS_SUBTREE, with the proper semantics
(3) make ChromeOnlyAccess() return true for NODE_IS_IN_ANONYMOUS_SUBTREE
(4) remove chromeOnlyContent machinery (including nsINode flags)
Blocks: 981257
(In reply to Bobby Holley (:bholley) from comment #0)
> (2) introduce NODE_IS_IN_ANONYMOUS_SUBTREE, with the proper semantics

Looks like we can actually compute this dynamically, assuming it's not too slow..

> (3) make ChromeOnlyAccess() return true for NODE_IS_IN_ANONYMOUS_SUBTREE
> (4) remove chromeOnlyContent machinery (including nsINode flags)

This part is actually pretty complicated. Currently, ChromeOnlyAccess() returns true for NAC and chromeOnlyContent AC, but not regular AC. The chromeOnlyContent stuff was kind of tacked on, and a lot of the logic there is still really specific to NAC (retargeting events and whatnot). Flipping this on for all AC breaks the world in Chrome.

So I'm going to see if I can scale back on this a bit.
(In reply to Bobby Holley (:bholley) from comment #1)
> Looks like we can actually compute this dynamically, assuming it's not too
> slow..
nsIContent::IsInAnonymousSubtree() works.
(In reply to Olli Pettay [:smaug] from comment #2)
> (In reply to Bobby Holley (:bholley) from comment #1)
> > Looks like we can actually compute this dynamically, assuming it's not too
> > slow..
> nsIContent::IsInAnonymousSubtree() works.

Right. But to put it on nsINode we need to do an IsContent()/AsContent() check. I was assuming that would all be very fast, but I figured it was worth checking.
Depends on: 988655
jwatt indicated that he thinks we shouldn't support this use case, and thinks
this is the right approach.
Attachment #8397811 - Flags: review?(jschoenick)
Attachment #8397812 - Flags: review?(mrbkap)
Blocks: 988842
No longer blocks: 981257
Summary: Move all anonymous content into the XBL scope, and remove chromeOnlyContent → Move all anonymous content into the XBL scope
Comment on attachment 8397811 [details] [diff] [review]
Part 2 - Prevent a NAC-parented plugin from trying to touch its reflector. v1

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

::: dom/plugins/test/testplugin/nptest.cpp
@@ +1032,5 @@
>    NPObject* o = nullptr;
> +
> +  // Set a property on NPNVPluginElementNPObject, unless the consumer explicitly
> +  // opted out of this behavior. This happens when the plugin element ends up in an
> +  // anonymous subtree, and the page script isn't allowed to touch the reflector.

The explanation here makes it sound like this is something that occurs automatically. Could probably just remove the second sentence, since the test explains why it sets it
Attachment #8397811 - Flags: review?(jschoenick) → review+
Attachment #8397810 - Flags: review?(mrbkap) → review+
Attachment #8397812 - Flags: review?(mrbkap) → review+
Attachment #8397813 - Flags: review?(bugs) → review+
Comment on attachment 8397814 [details] [diff] [review]
Part 5 - Add a dynamic accessor to determine whether a node is in an anonymous subtree. v1


>   nsIContent* AsContent();
>+  const nsIContent* AsContent() const {
{ goes to its own line
Attachment #8397814 - Flags: review?(bugs) → review+
Comment on attachment 8397815 [details] [diff] [review]
Part 6 - Put all anonymous content into the XBL scope. v1


>   mozilla::dom::ParentObject GetParentObjectInternal(nsINode* aNativeParent) const {
>     mozilla::dom::ParentObject p(aNativeParent);
>-    p.mUseXBLScope = ChromeOnlyAccess();
>+    p.mUseXBLScope = IsInAnonymousSubtree();
Hmm, does this affect chrome code too?
Why would we want to use xblscope there for everything?
(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 8397815 [details] [diff] [review]
> Part 6 - Put all anonymous content into the XBL scope. v1
> 
> 
> >   mozilla::dom::ParentObject GetParentObjectInternal(nsINode* aNativeParent) const {
> >     mozilla::dom::ParentObject p(aNativeParent);
> >-    p.mUseXBLScope = ChromeOnlyAccess();
> >+    p.mUseXBLScope = IsInAnonymousSubtree();
> Hmm, does this affect chrome code too?
> Why would we want to use xblscope there for everything?

Because for chrome,  GetXBLScope(global) === global.
Comment on attachment 8397815 [details] [diff] [review]
Part 6 - Put all anonymous content into the XBL scope. v1

Ahaa, then this need a comment that mUseXBLScope is effectively nop-op in
chrome.
Attachment #8397815 - Flags: review?(bugs) → review+
Mano, are you interested in looking at this? Blake's review alone might also be sufficient.
Flags: needinfo?(mano)
Comment on attachment 8397812 [details] [diff] [review]
Part 3 - Run the FeedWriter sandbox with an expanded principal. v1

Indeed.  I figured a while ago all this mess could go away, not sure if I ever filed it though.
Attachment #8397812 - Flags: review?(mano) → review+
Flags: needinfo?(mano)
Marking as [qa-] but please let me know if this needs any QA attention before we release Firefox 31.
Whiteboard: [qa-]
Depends on: 993423
Duplicate of this bug: 948000
Depends on: 998724
Depends on: 1008615
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.