986730 - Move all anonymous content into the XBL scope

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

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)

Comment 1

[Bobby Holley (:bholley)]

(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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 3

[Bobby Holley (:bholley)]

(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.

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix up a bunch of tests that assume that they can touch AC in content. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Prevent a NAC-parented plugin from trying to touch its reflector. v1

jwatt indicated that he thinks we shouldn't support this use case, and thinks
this is the right approach.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Run the FeedWriter sandbox with an expanded principal. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 - Rename NODE_IS_IN_ANONYMOUS_SUBTREE to NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE, since that's what it is. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 5 - Add a dynamic accessor to determine whether a node is in an anonymous subtree. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 6 - Put all anonymous content into the XBL scope. v1

Comment 12

[John Schoenick [:johns]]

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

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 15

[Bobby Holley (:bholley)]

(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 16

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 17

[Bobby Holley (:bholley)]

Mano, are you interested in looking at this? Blake's review alone might also be sufficient.

Comment 18

[Bobby Holley (:bholley)]

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

Comment 19

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #18)
> https://tbpl.mozilla.org/?tree=Try&rev=5aa27163c49d

Green, modulo browserchromepocalypse.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/74a617a50287
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f8219dddacf7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/48a316da2fb7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a0a5af9959de
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/73af60e34c5b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c4773f208050
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ec8a5ce1d620

Comment 20

[Asaf Romano (gone)]

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.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8219dddacf7
https://hg.mozilla.org/mozilla-central/rev/48a316da2fb7
https://hg.mozilla.org/mozilla-central/rev/a0a5af9959de
https://hg.mozilla.org/mozilla-central/rev/73af60e34c5b
https://hg.mozilla.org/mozilla-central/rev/c4773f208050
https://hg.mozilla.org/mozilla-central/rev/ec8a5ce1d620

Comment 22

[u279076]

Marking as [qa-] but please let me know if this needs any QA attention before we release Firefox 31.