411054 - Audit IsNativeAnonymous()/GetBindingParent() uses

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Right now we have two ways of testing for a "native anonymous" node, depending on what we mean.  IsNativeAnonymous() tests to make sure the node is in fact native anonymous.  We have various callers who don't use that, though, because it used to not work for XUL until bug 410119 got fixed.  We can now simplify those callers.

There are also callers who actually want to know whether a node is in some subtree rooted in a native anonymous node.  This involves walking the bindingParent chain, etc, and is implemented in multiple places (find code, style system at the very least).

We should audit our code that does these to decide which question we're really answering (e.g. there's security manager code that asks the first question when it should really ask the second one), and we should perhaps simplify the answer to the second question (via an nsIContent API).

Thoughts on the latter?

Comment 1

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

Do we actually want to keep the GetBindingParent "hack"? I think yes, that
makes it faster to check if some node is in native anonymous subtree.
Writing now some kind of patch for this...

Comment 2

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

Attachment: WIP

The patch changes the behavior of
(1) nsScriptSecurityManager
(2) nsDocument::ElementFromPoint (discussed shortly with roc about the change)
    IMO, we do want to return element which is just inside XBL.
    When we have XBL2, depending on the document on which the method is called
    anonymous or non-anonymous could be returned.
(3) nsDocument::MutationEventDispatched, the code is just wrong. If mutations
    have happened in native anonymous content (but not on native anonymous 
    element itself) we don't want to take those in account when trying to
    figure out what is the target for DOMSubtreeModified event.
    (This isn't security bug. If mutations have happened only in native 
     anonymous, event isn't propagated.)

Not sure if I want FindFirstNonNativeAnonymous to be in nsIContent or should
it be in nsContentUtils. Though, with other changes, all native-anon handling
is in nsIContent.

Need to run mochitest...

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Do we actually want to keep the GetBindingParent "hack"?

I _think_ so.  It simplifies and speeds up things like nsContentUtils::IsInSameAnonymousTree a lot (which is good, since it's used a good bit), in addition to the performance benefit you mentioned.

For nsDocument::ElementFromPoint I'm not sure we want to return XBL anon content.  See the discussion in the bug that added the code.

Would you mind also changing the Find code that does native anonymous stuff?

Comment 5

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

Need to still figure out how to handle svg:use

Comment 6

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

(In reply to comment #5)
> Need to still figure out how to handle svg:use
And this makes things quite a bit harder. We don't support SVGElementInstance
event targets.

Comment 7

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

Um, P2. I agree, fixing this for 1.9 would be more than great.
/me thinks how to solve the svg:use issue.

Currently the clone of the referenced element in svg:use is set nativeanonymous,
but not any of its descendants. This means accessing the root of the cloned
subtree using JS isn't possible, but accessing other nodes in the subtree is
possible. If this bug gets fixed and handling of svg:use stays the same,
accessing any nodes in the cloned subtree becomes prevented. That is how things
should be, but then we should support SVGElementInstance for event handling.
(and one great thing is that SVG 1.1 specifies SVGElementInstance so that it applies only to SVGElements, which means that event handling for example in 
cloned svg:foreignObjects is unclear. Opera seems to have change that 
limitation so that also other DOMElements, not only SVGElements, are allowed.)
One hackish, but quite simple possibility might be to not mark the root of the
cloned subtree as nativeanonymous. Still setting the binding parent though, so
that events get retargeted (though, that is not what should happen if 
SVGElementInstance was supported). I wonder if that change would break something in styling or somewhere...

The reason to be able access either the cloned subtree or SVGElementInstance 
tree is event handling attributes, which are also cloned when svg:use's referenced element is cloned.

Comment 8

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

Attachment: make cloned elements under svg:use non-native-anonymous

Tor, any comments on this? The cloned nodes under svg:use
won't still show up in the DOM, but they just aren't native anonymous.
This allows onXXX attributes to work properly on all cloned elements.

Comment 9

[tor]

(In reply to comment #8)
> Created an attachment (id=296869) [details]
> make cloned elements under svg:use non-native-anonymous
> 
> Tor, any comments on this? The cloned nodes under svg:use
> won't still show up in the DOM, but they just aren't native anonymous.
> This allows onXXX attributes to work properly on all cloned elements.

I don't have any problems with that - it's been quite a while since that code was added, but I don't think there was any intentional choice between native and non-native anonymous.

Comment 10

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

Comment on attachment 296869 [details] [diff] [review]
make cloned elements under svg:use non-native-anonymous

Jonas, what do you think about this approach.
Using node flags to optimize (lazily) native-anonymousness checks. Though on content elements don't usually have bindingParents. chrome is different.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not a big fan of using multiple flags like this. Is this only to optimize walking up the tree looking for native-anonymous parents? Is there really a measurable perf benefit? Having the same information available in multiple (i.e. both on the node itself and on the parent) places is a pain since it's so easy to get out of sync. This is especially bad if getting out of sync means that you've got a security problem.

Comment 12

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

When running content scripts, IsNativeAnonymous (or IsInNativeAnonymousSubtree with the patch) is called very often.
Currently IsNativeAnonymous is fast - inlined simple flag check. Because the
patch fixes relevant callers to use IsInNativeAnonymousSubtree, those cases
might get quite a bit slower without flags, especially if DOM is deep.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hold on.  The call to IsNativeAnonymousSubtree() would only happen when the caller principal is not system, right?

And what would matter is the depth of the bindingParent tree, not the DOM tree, right?

Unless I'm missing something, chrome should have the system principal, and content never has deeply nested bindings...

Comment 14

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

(In reply to comment #13)
> Hold on.  The call to IsNativeAnonymousSubtree() would only happen when the
> caller principal is not system, right?
Ofc. When running content scripts, principal isn't system

> And what would matter is the depth of the bindingParent tree, not the DOM tree,
> right?
oops, right.

I'll make a patch without those extra flags. If there is a regression with 
that, then I can try to add flags.

Comment 15

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

Attachment: no flags

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 304047 [details] [diff] [review]
no flags

r/sr=me

Comment 17

[Steve England [:stevee]]

It looks like this caused bug 421486.