Move all anonymous content into the XBL scope

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks 1 bug)

unspecified
mozilla31
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 fixed)

Details

(Whiteboard: [qa-])

Attachments

(6 attachments)

Assignee

Description

5 years ago
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)
Assignee

Updated

5 years ago
Blocks: 981257
Assignee

Comment 1

5 years ago
(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.
Assignee

Comment 3

5 years ago
(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.
Assignee

Updated

5 years ago
Depends on: 988655
Assignee

Comment 7

5 years ago
jwatt indicated that he thinks we shouldn't support this use case, and thinks
this is the right approach.
Attachment #8397811 - Flags: review?(jschoenick)
Assignee

Updated

5 years ago
Attachment #8397812 - Flags: review?(mrbkap)
Assignee

Updated

5 years ago
Blocks: 988842
Assignee

Updated

5 years ago
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?
Assignee

Comment 15

5 years ago
(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+
Assignee

Comment 17

5 years ago
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-]
Assignee

Updated

5 years ago
Duplicate of this bug: 948000
Depends on: 998724

Updated

5 years ago
Depends on: 1008615
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.