Do not reset controls upon removing media element from the DOM.

Do not reset controls upon removing media element from the DOM.

Upon removal of a media element, arrange for it to be removed from the
document before resetting its media controls. If the controls are
showing, they'll be hidden but not reset. A full reset of these controls
while a DOM removal operation is in progress may trigger nested event
delivery and mutate its attributes in unsupported ways.

R=fs@opera.com,philipj@opera.com
BUG=462046
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190986

[sof, 2015-02-27 10:24:34]

sigbjornf@opera.com changed reviewers:
+ fs@opera.com, philipj@opera.com

[sof, 2015-02-27 10:24:34]

Please take a look.

[fs, 2015-02-27 10:31:17]

lgtm

[philipj_slow, 2015-02-27 10:35:29]

A conservative fix, LGTM with confusion.

https://codereview.chromium.org/961243002/diff/1/LayoutTests/media/remove-fro...
File LayoutTests/media/remove-from-document-config-controls-no-crash.html
(right):

https://codereview.chromium.org/961243002/diff/1/LayoutTests/media/remove-fro...
LayoutTests/media/remove-from-document-config-controls-no-crash.html:35:
Array.prototype.forEach.call(node.attributes, function (n) { node[n &&
n.localName] = 2; });
This looks a bit cryptic to me. Is this to take code paths for reflected
attributes?

https://codereview.chromium.org/961243002/diff/1/LayoutTests/media/remove-fro...
LayoutTests/media/remove-from-document-config-controls-no-crash.html:38: if
(node.localName == 'input')
Could this not be made unconditional given the null check above?

[sof, 2015-02-27 10:39:54]

https://codereview.chromium.org/961243002/diff/1/LayoutTests/media/remove-fro...
File LayoutTests/media/remove-from-document-config-controls-no-crash.html
(right):

https://codereview.chromium.org/961243002/diff/1/LayoutTests/media/remove-fro...
LayoutTests/media/remove-from-document-config-controls-no-crash.html:35:
Array.prototype.forEach.call(node.attributes, function (n) { node[n &&
n.localName] = 2; });
On 2015/02/27 10:35:29, philipj_UTC7 wrote:
> This looks a bit cryptic to me. Is this to take code paths for reflected
> attributes?

It is a reduction from a fuzzer's doing; iterating over .attributes is required
to trigger the crashiness.

https://codereview.chromium.org/961243002/diff/1/LayoutTests/media/remove-fro...
LayoutTests/media/remove-from-document-config-controls-no-crash.html:38: if
(node.localName == 'input')
On 2015/02/27 10:35:29, philipj_UTC7 wrote:
> Could this not be made unconditional given the null check above?

It could, but I don't see the value of making the test more generic.

[sof, 2015-02-27 10:58:10]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-02-27 10:58:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/961243002/1

[philipj_slow, 2015-02-27 11:04:33]

https://codereview.chromium.org/961243002/diff/1/LayoutTests/media/remove-fro...
File LayoutTests/media/remove-from-document-config-controls-no-crash.html
(right):

https://codereview.chromium.org/961243002/diff/1/LayoutTests/media/remove-fro...
LayoutTests/media/remove-from-document-config-controls-no-crash.html:35:
Array.prototype.forEach.call(node.attributes, function (n) { node[n &&
n.localName] = 2; });
On 2015/02/27 10:39:54, sof wrote:
> On 2015/02/27 10:35:29, philipj_UTC7 wrote:
> > This looks a bit cryptic to me. Is this to take code paths for reflected
> > attributes?
> 
> It is a reduction from a fuzzer's doing; iterating over .attributes is
required
> to trigger the crashiness.

Does it matter if node.foo is set or if the callback does nothing at all?

https://codereview.chromium.org/961243002/diff/1/LayoutTests/media/remove-fro...
LayoutTests/media/remove-from-document-config-controls-no-crash.html:38: if
(node.localName == 'input')
On 2015/02/27 10:39:54, sof wrote:
> On 2015/02/27 10:35:29, philipj_UTC7 wrote:
> > Could this not be made unconditional given the null check above?
> 
> It could, but I don't see the value of making the test more generic.

Very well, it looked to me like it was already trying to traverse as much as
possible.

[sof, 2015-02-27 11:16:34]

https://codereview.chromium.org/961243002/diff/1/LayoutTests/media/remove-fro...
File LayoutTests/media/remove-from-document-config-controls-no-crash.html
(right):

https://codereview.chromium.org/961243002/diff/1/LayoutTests/media/remove-fro...
LayoutTests/media/remove-from-document-config-controls-no-crash.html:35:
Array.prototype.forEach.call(node.attributes, function (n) { node[n &&
n.localName] = 2; });
On 2015/02/27 11:04:33, philipj_UTC7 wrote:
> On 2015/02/27 10:39:54, sof wrote:
> > On 2015/02/27 10:35:29, philipj_UTC7 wrote:
> > > This looks a bit cryptic to me. Is this to take code paths for reflected
> > > attributes?
> > 
> > It is a reduction from a fuzzer's doing; iterating over .attributes is
> required
> > to trigger the crashiness.
> 
> Does it matter if node.foo is set or if the callback does nothing at all?

It matters, involving node.max in particular..but not entirely, I initially
reduced the TC too far only to do that which left it triggering nested event
dispatch asserts only.

I'm not going to spend more time than the day already spent reducing this one.

[commit-bot: I haz the power, 2015-02-27 11:41:33]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190986