Don't expose the user agent shadow tree

Don't expose the user agent shadow tree

Don't expose the user agent shadow tree used for the cloned <use> tree. Add one test for the attribute event listener code path (onclick) and one for addEventListener API. To make sure the shadow tree is not exposed, make sure we always wrap non-shadow tree SVG Elements.

BUG=313438
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176065

[rwlbuis, 2014-06-05 23:46:34]

Is this what needs to be implemented?

http://www.w3.org/TR/2013/WD-shadow-dom-20130514/#events

If so, is there is code for this already in Blink?
My quick fix patch is simpler, hopefully not too simple. It fixes the testcase
but I wonder if there is more API that needs to be adjusted. And I wonder if
still proto could be used to call the parentNode in Node...

[esprehn, 2014-06-05 23:58:55]

This is not correct, you can't just redefine what parentNode means in svg.
You just broke using svg elements in user shadow dom. You still leak the
root too, there's lots of other ways to get there besides the parentNode.

Not lgtm

You need to either not add those event listeners at all, of add a proxy
event listener that dispatches it again from the right spot.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[rwlbuis, 2014-06-06 02:41:10]

On 2014/06/05 23:58:55, esprehn wrote:
> This is not correct, you can't just redefine what parentNode means in svg.
> You just broke using svg elements in user shadow dom. You still leak the
> root too, there's lots of other ways to get there besides the parentNode.
> 
> Not lgtm
> 
> You need to either not add those event listeners at all, of add a proxy
> event listener that dispatches it again from the right spot.

I tried not adding them but it is hard. I tried testing for
!correspondingElement()
in SVGElement::parseAttribute but it is not set yet at that point.
The latest CL sets the event target to the original (corresponding) element
instead of the user
agent shadow tree node. While not according to spec this at least fixes the
security problem
for now and is a short term fix until we figure out how to fix it exactly, IMHO
the SVG2 spec
is not really clear on the exact behavior we should provide.

[esprehn, 2014-06-11 01:10:23]

On 2014/06/06 at 02:41:10, rob.buis wrote:
> On 2014/06/05 23:58:55, esprehn wrote:
> > This is not correct, you can't just redefine what parentNode means in svg.
> > You just broke using svg elements in user shadow dom. You still leak the
> > root too, there's lots of other ways to get there besides the parentNode.
> > 
> > Not lgtm
> > 
> > You need to either not add those event listeners at all, of add a proxy
> > event listener that dispatches it again from the right spot.
> 
> I tried not adding them but it is hard. I tried testing for
!correspondingElement()
> in SVGElement::parseAttribute but it is not set yet at that point.
> The latest CL sets the event target to the original (corresponding) element
instead of the user
> agent shadow tree node. While not according to spec this at least fixes the
security problem
> for now and is a short term fix until we figure out how to fix it exactly,
IMHO the SVG2 spec
> is not really clear on the exact behavior we should provide.

You need to instead put the event listeners on the real elements I think, or
create event proxies or something. We need to fix this soon though.

[rwlbuis, 2014-06-12 21:48:43]

PTAL, 323343004 will fix the target attribute, my patch fixes currentTarget to
always point to the corresponding element, not the element in the shadow tree.
"this" should be set up to be whatever currentTarget points to, so gets fixed
too by this patch.

[esprehn, 2014-06-12 21:52:36]

lgtm, please fix the test before landing.

https://codereview.chromium.org/312423002/diff/80001/LayoutTests/svg/custom/u...
File
LayoutTests/svg/custom/use-event-handler-on-referenced-element-hierarchy.svg
(right):

https://codereview.chromium.org/312423002/diff/80001/LayoutTests/svg/custom/u...
LayoutTests/svg/custom/use-event-handler-on-referenced-element-hierarchy.svg:24:
if (rect.parentNode == "[object SVGGElement]" && rect.parentNode.parentNode ==
"[object SVGDefsElement]")
Can you compare to a real node in the page? getElementById something and then do
== instead of string checks.

[rwlbuis, 2014-06-12 23:05:56]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-06-12 23:06:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob.buis@samsung.com/312423002/100001

[commit-bot: I haz the power, 2014-06-13 00:11:14]

Change committed as 176065