Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow tree.
This approach allows a user to access a cloned node in UA's shadow tree if a node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to shrink an event path so
it does not include a node in <use>'s UA shadow tree.

A <use> element itself still receives an event if an event is a composed event.

This CL also reverts the most parts of https://codereview.chromium.org/312423002
because it is no longer valid in the latest SVG spec, and stop copying event
listeners from a referenced node into a cloned node because it is no longer
necessary.

BUG=630870
Committed: https://crrev.com/c4f2b0897923d1fa54fd2b644f6771290e812f4b
Cr-Commit-Position: refs/heads/master@{#409455}

[hayato, 2016-07-27 05:36:13]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 05:36:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-07-27 05:40:27]

Description was changed from

==========
Prevent an event fireing on a cloned node in svg <use>'s UA shadow tree

SVG <use> element is cloning a user-provided subtree into their UA shadow tree.
This approach allows a user to access a node in UA's shadow tree if a node has
an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target)}">
  </g>
  <use href="#a">
</svg>

Thus, we leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, this CL prevents an event from be
fired on a node if it is in <use>'s shadow tree.

BUG=630870
==========

to

==========
Do not fire an event on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into their UA shadow
tree
in its implementation. This approach allows a user to access a cloned node in
UA's
shadow tree if a node has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target)}">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, this CL prevents an event from being
fired on a node if it is in <use>'s shadow tree.

BUG=630870
==========

[hayato, 2016-07-27 05:41:14]

Description was changed from

==========
Do not fire an event on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into their UA shadow
tree
in its implementation. This approach allows a user to access a cloned node in
UA's
shadow tree if a node has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target)}">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, this CL prevents an event from being
fired on a node if it is in <use>'s shadow tree.

BUG=630870
==========

to

==========
Do not fire an event on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target)}">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, this CL prevents an event from being
fired on a node if it is in <use>'s shadow tree.

BUG=630870
==========

[hayato, 2016-07-27 05:42:07]

Description was changed from

==========
Do not fire an event on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target)}">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, this CL prevents an event from being
fired on a node if it is in <use>'s shadow tree.

BUG=630870
==========

to

==========
Do not fire an event on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target)}">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to prevent an event from
being
fired on a node if it is in <use>'s shadow tree.

BUG=630870
==========

[hayato, 2016-07-27 05:49:56]

hayato@chromium.org changed reviewers:
+ fs@opera.com, jww@chromium.org, mkwst@chromium.org

[hayato, 2016-07-27 05:49:57]

PTAL

[hayato, 2016-07-27 05:51:19]

Description was changed from

==========
Do not fire an event on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target)}">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to prevent an event from
being
fired on a node if it is in <use>'s shadow tree.

BUG=630870
==========

to

==========
Do not fire an event on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to prevent an event from
being
fired on a node if it is in <use>'s shadow tree.

BUG=630870
==========

[hayato, 2016-07-27 05:51:42]

Description was changed from

==========
Do not fire an event on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to prevent an event from
being
fired on a node if it is in <use>'s shadow tree.

BUG=630870
==========

to

==========
Do not fire an event on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to prevent an event from
being
fired on a node if it is in <use>'s UA shadow tree.

BUG=630870
==========

[commit-bot: I haz the power, 2016-07-27 06:51:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 06:51:57]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mike West, 2016-07-27 08:37:05]

mkwst@chromium.org changed reviewers:
+ pdr@chromium.org

[Mike West, 2016-07-27 08:37:06]

The test failures look relevant. +pdr who I think knows things about SVG (I
don't).

[fs, 2016-07-27 16:19:49]

Could we treat Event.target in the same way as we do Event.currentTarget? (Not
sure why
https://chromium.googlesource.com/chromium/src/+/c0fcf79e84996c055b61d1bf6a10...
only did one but not the other.) IIUC that's what the newly updated SVG2 section
says - https://svgwg.org/svg2-draft/struct.html#UseEventHandling.

Since the demise of the (explicit) element instances though, I think this field
is open for suggestions as well if you have any.

On 2016/07/27 at 08:37:06, mkwst wrote:
> The test failures look relevant. +pdr who I think knows things about SVG (I
don't).

Yes, at the very least the ones that fails assertions (at EventPath.cpp:250).

[pdr., 2016-07-27 17:41:32]

@mkwst, can you give fs and me access to 630870?

[jww, 2016-07-27 17:45:28]

On 2016/07/27 17:41:32, pdr. wrote:
> @mkwst, can you give fs and me access to 630870?

I've added pdr and fs to 630870.

[pdr., 2016-07-27 19:12:24]

How does the fact that these are under the user agent shadow dom cause this bug?
Could we switch this to a regular shadow tree?

[hayato, 2016-07-28 03:22:10]

On 2016/07/27 at 16:19:49, fs wrote:
> Could we treat Event.target in the same way as we do Event.currentTarget? (Not
sure why
https://chromium.googlesource.com/chromium/src/+/c0fcf79e84996c055b61d1bf6a10...
only did one but not the other.) IIUC that's what the newly updated SVG2 section
says - https://svgwg.org/svg2-draft/struct.html#UseEventHandling.
> 
> Since the demise of the (explicit) element instances though, I think this
field is open for suggestions as well if you have any.

Thanks. I did not realize SVG spec defines how <use> should be implemented. I
just thought it was an implementation detail.
Let me take a look at it, and update the CL accordingly.

[hayato, 2016-07-28 03:26:12]

On 2016/07/27 at 19:12:24, pdr wrote:
> How does the fact that these are under the user agent shadow dom cause this
bug? Could we switch this to a regular shadow tree?

Yeah, that is the root cause we have to figure out. It looks no one understands
why UA shadow tree has such a *privilege*. As of now, I can not think of any
relevant code path.
I am wondering whether this is a problem of UA shadow tree in general, or a
problem <use>'s UA shadow tree. We have to figure out.

[hayato, 2016-07-28 05:30:46]

I have read: https://svgwg.org/svg2-draft/struct.html#UseElement

As far as my understanding is correct, this spec clearly allows an event
dispatching in a shadow tree via event attribute.

That should be updated because we leak a node in an UA shadow tree , as the
exploit makes use of it. An UA shadow tree is designed for: "Never leak a node
inside of a tree".

As another idea, we should change our implementation so that <use> uses a normal
author shadow tree, instead of UA shadow tree. It does not look that SVG spec
defines which kind of a shadow tree should be used.

[hayato, 2016-07-28 05:47:16]

It looks this CL, https://codereview.chromium.org/312423002, is doing the
opposite thing to the spec.

This CL tries to avoid a *leak*, but

- It is incomplete. It does nothing for event.target.
- The spec does not requires this behavior. It looks the spec requires the
event's currentTarget and target should be set to the elements in a shadow tree.

Is the spec recently updated? I am confused. 

Can someone who is familiar with the SVG spec give us advice?

[fs, 2016-07-28 15:32:32]

On 2016/07/28 at 05:47:16, hayato wrote:
> It looks this CL, https://codereview.chromium.org/312423002, is doing the
opposite thing to the spec.
> 
> This CL tries to avoid a *leak*, but
> 
> - It is incomplete. It does nothing for event.target.
> - The spec does not requires this behavior. It looks the spec requires the
event's currentTarget and target should be set to the elements in a shadow tree.
> 
> Is the spec recently updated? I am confused. 
> 
> Can someone who is familiar with the SVG spec give us advice?

Yes, the spec changed just a few days ago. Previously [1] there used to be an
explicit "instance tree" (SVGElementInstance) which would serve as the target
for events. When that was removed from the Blink implementation, I suspect it
wasn't fully considered where events would be delivered too (or UA shadows
weren't considered harmful from that perspective at least. Like in the CL which
added the currentTarget redirect.) Feel free to raise any issues/questions at
https://github.com/w3c/svgwg/.

Anyway, from a spec PoV I think we have quite a it of leeway to do whatever we
want. It could be worthwhile to check what other UAs do.

I'm beginning to ponder whether an alternate way would be to simply drop all
event handlers from the shadow tree.

[1] https://www.w3.org/TR/SVG/struct.html#UseElement

[hayato, 2016-07-29 03:36:46]

Thanks. Yeah, I think we have two choices as a short term fix:

A. Don not call an event listener which is registered at a node in <use>'s UA
shadow trees. 
   In other words, we trim the "event path" so that it does not includes nodes
in the shadow tree.
   Thus, <use> can receive an event (with retargeted correctly).

B. Make <use> use a closed shadow tree, instead of UA shadow tree.
   But I am not sure this would prevent the exploit. I have to test it. We can
still leak a node in a closed shadow tree, anyway.

I prefer A. Let me implement A.

Then, we would file an issue for the SVG's spec accordingly.

[hayato, 2016-07-29 06:10:00]

Rewrite

[hayato, 2016-07-29 06:10:02]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 06:10:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-07-29 06:12:53]

Description was changed from

==========
Do not fire an event on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to prevent an event from
being
fired on a node if it is in <use>'s UA shadow tree.

BUG=630870
==========

to

==========
Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to shrink an event path so
it does not includes nodes in <use>'s UA shadow tree.

This CL also reverts the most parts of https://codereview.chromium.org/312423002
because it is no longer valid in the latest SVG spec.

BUG=630870
==========

[hayato, 2016-07-29 06:13:20]

Description was changed from

==========
Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to shrink an event path so
it does not includes nodes in <use>'s UA shadow tree.

This CL also reverts the most parts of https://codereview.chromium.org/312423002
because it is no longer valid in the latest SVG spec.

BUG=630870
==========

to

==========
Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to shrink an event path so
it does not include a node in <use>'s UA shadow tree.

This CL also reverts the most parts of https://codereview.chromium.org/312423002
because it is no longer valid in the latest SVG spec.

BUG=630870
==========

[hayato, 2016-07-29 06:14:53]

Description was changed from

==========
Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to shrink an event path so
it does not include a node in <use>'s UA shadow tree.

This CL also reverts the most parts of https://codereview.chromium.org/312423002
because it is no longer valid in the latest SVG spec.

BUG=630870
==========

to

==========
Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to shrink an event path so
it does not include a node in <use>'s UA shadow tree.

A <use> element itself still receives an event if an event is a composed event.

This CL also reverts the most parts of https://codereview.chromium.org/312423002
because it is no longer valid in the latest SVG spec.

BUG=630870
==========

[hayato, 2016-07-29 06:15:12]

I have rewritten the CL. PTAL.

[hayato, 2016-07-29 06:23:23]

Description was changed from

==========
Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to shrink an event path so
it does not include a node in <use>'s UA shadow tree.

A <use> element itself still receives an event if an event is a composed event.

This CL also reverts the most parts of https://codereview.chromium.org/312423002
because it is no longer valid in the latest SVG spec.

BUG=630870
==========

to

==========
Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to shrink an event path so
it does not include a node in <use>'s UA shadow tree.

A <use> element itself still receives an event if an event is a composed event.

This CL also reverts the most parts of https://codereview.chromium.org/312423002
because it is no longer valid in the latest SVG spec, and stop copying event 
listeners from a referenced node into a cloned node because it is no longer
necessary.


BUG=630870
==========

[hayato, 2016-07-29 06:35:28]

BTW, I am also interested in plan B:

> B. Make <use> use a closed shadow tree, instead of UA shadow tree.
>   But I am not sure this would prevent the exploit. I have to test it. We can
still leak a node in a closed shadow tree, anyway.

because this is less-magical, and we can make the implementation and the spec
simple. Are SVG folks interested in implementing this?

[commit-bot: I haz the power, 2016-07-29 07:24:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-29 07:24:57]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hayato, 2016-07-29 08:51:55]

As an experiment, I have updated SVGUseElement to use an open shadow root,
instead of UA shadow root. See https://codereview.chromium.org/2197533002.
I can still exploit the chrome, though.

Given that, an UA shadow tree might not be considered harmful in terms of the
exploit. The root cause might be something other than it.

[fs, 2016-07-29 16:19:09]

On 2016/07/29 at 08:51:55, hayato wrote:
> As an experiment, I have updated SVGUseElement to use an open shadow root,
instead of UA shadow root. See https://codereview.chromium.org/2197533002.
> I can still exploit the chrome, though.
> 
> Given that, an UA shadow tree might not be considered harmful in terms of the
exploit. The root cause might be something other than it.

Yes, that sounds like might need some additional investigation. This CL
otherwise looks reasonable to me, although I've no idea how much it'll break in
the real world. (Hopefully not much.) Would appreciate additional looks and
opinions though.

[pdr., 2016-07-30 04:02:42]

I think this approach looks good overall.

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/custom/use-instanceRoot-event-listener-liveness.xhtml
(left):

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/use-instanceRoot-event-listener-liveness.xhtml:7:
<svg xmlns="http://www.w3.org/2000/svg"
xmlns:xlink="http://www.w3.org/1999/xlink" width="100px" height="100px">
Can you re-add a test like this (with a simulated mouse click) but ensure that
the tree is not exposed (sort of the opposite of the old test)?

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventPath.cpp (right):

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventPath.cpp:89: return
(node.isPseudoElement() && !node.parentElement()) || (isInSVGUseShadowTree(node)
&& event && !event->composed());
WDYT of breaking these two complex cases apart as if statements?

if (node.isPseudoElement() && !node.parentElement())
    return true;

// Exclude non-composed events from SVG use trees.
if (node.isSVGElement()) {
    if (toSVGElement(node).inUseShadowTree() && event && !event->composed())
        return true;
}

return false;

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventPath.cpp:102: void
EventPath::calculatePath()
I'm not very familiar with the event apis. Can you think of any ways other than
the event path that node information be exposed? For example, do we need to
worry about the event's currentTarget or srcElement?

I wonder if we could add a DCHECK in a central place to catch leaking of user
agent shadow tree nodes (svg or otherwise)?

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventPath.cpp:117: while
(isInSVGUseShadowTree(*current))
I think this can be rewritten using existing apis to avoid having to add
isInSVGUseShadowTree:
while (current->isSVGElement()) {
    SVGUseElement* correspondingUseElement =
toSVGElement(current)->correspondingUseElement();
    if (!correspondingUseElement)
        break;
    current = correspondingUseElement;
}

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGUseElement.cpp (left):

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGUseElement.cpp:555:
data->eventListenerMap.copyEventListenersNotCreatedFromMarkupToTarget(&element);
With cloneNonMarkupEventListeners gone we can remove a lot of other
dubious-looking code too. Can you delete
EventListenerMap::copyEventListenersNotCreatedFromMarkupToTarget and
v8LazyEventListener's wasCreatedFromMarkup and eventListener's
wasCreatedFromMarkup?

[hayato, 2016-08-01 03:31:52]

addressed

[hayato, 2016-08-01 03:31:54]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 03:31:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-08-01 03:32:22]

Description was changed from

==========
Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to shrink an event path so
it does not include a node in <use>'s UA shadow tree.

A <use> element itself still receives an event if an event is a composed event.

This CL also reverts the most parts of https://codereview.chromium.org/312423002
because it is no longer valid in the latest SVG spec, and stop copying event 
listeners from a referenced node into a cloned node because it is no longer
necessary.


BUG=630870
==========

to

==========
Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to shrink an event path so
it does not include a node in <use>'s UA shadow tree.

A <use> element itself still receives an event if an event is a composed event.

This CL also reverts the most parts of https://codereview.chromium.org/312423002
because it is no longer valid in the latest SVG spec, and stop copying event 
listeners from a referenced node into a cloned node because it is no longer
necessary.

BUG=630870
==========

[hayato, 2016-08-01 03:44:11]

Thank you for the review.

> I'm not very familiar with the event apis. Can you think of any ways other
than the event path that node information be exposed? For example, do we need to
worry about the event's currentTarget or srcElement?

I can not think of any case. Regarding currentTarget and srcElement, they are
never exposed to user-land javascript, as far as I know. If we find such a leak,
it should be considered a bug in the spec (or the Blink's implementation) which
we should fix.


> I wonder if we could add a DCHECK in a central place to catch leaking of user
agent shadow tree nodes (svg or otherwise)?

The problem is that users can add an event listener to a node in UA shadow
trees. That can be the cause of a leak. Dispatching an event on a node in UA
shadow trees should be okay. I think some built-in elements want to use an event
listener in their UA shadow trees in their implementation.

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/custom/use-instanceRoot-event-listener-liveness.xhtml
(left):

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/use-instanceRoot-event-listener-liveness.xhtml:7:
<svg xmlns="http://www.w3.org/2000/svg"
xmlns:xlink="http://www.w3.org/1999/xlink" width="100px" height="100px">
Done. I have re-added a test like this, but ensure that a node in a shadow tree
does not receive an event.

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventPath.cpp (right):

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventPath.cpp:89: return
(node.isPseudoElement() && !node.parentElement()) || (isInSVGUseShadowTree(node)
&& event && !event->composed());
Sounds nice. Done

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventPath.cpp:117: while
(isInSVGUseShadowTree(*current))
Thanks! Done.

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGUseElement.cpp (left):

https://codereview.chromium.org/2186823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGUseElement.cpp:555:
data->eventListenerMap.copyEventListenersNotCreatedFromMarkupToTarget(&element);
Good point! Done.

[commit-bot: I haz the power, 2016-08-01 05:23:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-01 05:23:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mike West, 2016-08-01 11:40:56]

The approach LGTM, but there's still a failing SVG layout test. Please wait to
land this patch until that's resolved, and pdr@/fs@ are happy with it from the
SVG side of things.

[hayato, 2016-08-01 11:45:24]

Thanks. I will not land this patch until everyone is happy with the approach.
Let me fix the failing layout test after that.

BTW, this CL does not fix the root cause. We have to investigate further, as
another effort.

[fs, 2016-08-01 16:37:24]

On 2016/08/01 at 11:45:24, hayato wrote:
> Thanks. I will not land this patch until everyone is happy with the approach.
Let me fix the failing layout test after that.

Approach is ok with me. I'll leave the 4-letter acronyming to pdr. As for the
failing test, that can hopefully be fixed up by moving the 'onclick' handler
("executeTest()") from the <rect> to the <use> or something like that.

[jww, 2016-08-01 18:12:49]

On 2016/08/01 16:37:24, fs (OoO until Aug 15) wrote:
> On 2016/08/01 at 11:45:24, hayato wrote:
> > Thanks. I will not land this patch until everyone is happy with the
approach.
> Let me fix the failing layout test after that.
> 
> Approach is ok with me. I'll leave the 4-letter acronyming to pdr. As for the
> failing test, that can hopefully be fixed up by moving the 'onclick' handler
> ("executeTest()") from the <rect> to the <use> or something like that.

If it's OK with Mike, it's OK by me, so no need to wait for my review.

[pdr., 2016-08-01 19:05:04]

I think this patch looks great now, LGTM

I have some light reservations about committing this before we fully understand
the root cause though. I will add some more information to the bug about why.

[hayato, 2016-08-03 03:15:39]

skip timeout test

[hayato, 2016-08-03 03:15:41]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 03:15:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-08-03 03:18:03]

Per the discussion in BUG=630870, we are going to land this CL.
I have marked one test skipped, temporarily, which we might want to take a look
further.

[hayato, 2016-08-03 03:18:16]

The CQ bit was unchecked by hayato@chromium.org

[hayato, 2016-08-03 03:18:20]

The CQ bit was checked by hayato@chromium.org

[hayato, 2016-08-03 03:18:20]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2186823002/#ps60001
(title: "skip timeout test")

[commit-bot: I haz the power, 2016-08-03 03:18:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 04:38:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-03 04:40:22]

Description was changed from

==========
Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to shrink an event path so
it does not include a node in <use>'s UA shadow tree.

A <use> element itself still receives an event if an event is a composed event.

This CL also reverts the most parts of https://codereview.chromium.org/312423002
because it is no longer valid in the latest SVG spec, and stop copying event 
listeners from a referenced node into a cloned node because it is no longer
necessary.

BUG=630870
==========

to

==========
Do not call an event listener on a cloned node in svg <use>'s UA shadow tree

SVG <use> element tries to clone a user-provided subtree into its UA shadow
tree.
This approach allows a user to access a cloned node in UA's shadow tree if a
node
has an event listener.

e.g.
<svg>
  <g id="a">
    <image href="" onerror="window.nodes.push(event.target);">
  </g>
  <use href="#a">
</svg>

As a result, we will leak a node in <use>'s UA shadow tree.

Given the current implementation of <use>, we have to shrink an event path so
it does not include a node in <use>'s UA shadow tree.

A <use> element itself still receives an event if an event is a composed event.

This CL also reverts the most parts of https://codereview.chromium.org/312423002
because it is no longer valid in the latest SVG spec, and stop copying event
listeners from a referenced node into a cloned node because it is no longer
necessary.

BUG=630870

Committed: https://crrev.com/c4f2b0897923d1fa54fd2b644f6771290e812f4b
Cr-Commit-Position: refs/heads/master@{#409455}
==========

[commit-bot: I haz the power, 2016-08-03 04:40:23]

Patchset 4 (id:??) landed as
https://crrev.com/c4f2b0897923d1fa54fd2b644f6771290e812f4b
Cr-Commit-Position: refs/heads/master@{#409455}