Disallow non-flexbox child renderers for RenderMedia

Disallow non-flexbox child renderers for RenderMedia

Inline children of replaced content is not handled and triggered the
ASSERT(curr->isRenderInline()) UNKNOWN in RenderInline::splitInlines().

The render tree at the point of failure was:

RenderView 0xd0c23404010                #document 0x2be215c04b48
  RenderBlock 0xd0c2341c010             HTML 0x2be215c101a8
    RenderBody 0xd0c2341c110            BODY 0x2be215c102b8
      RenderBlock (anonymous) 0xd0c2341c310
        RenderVideo 0xd0c23420010       VIDEO 0x2be215c48010
*         RenderInline (relative positioned) 0xd0c23424010 DIV 0x2be215c68010
      RenderBlock (anonymous) 0xd0c2341c210
      RenderBlock (anonymous) 0xd0c2341c410

Making ::-webkit-media-controls* internal would have fixed this:
https://codereview.chromium.org/662243003/
https://groups.google.com/a/chromium.org/d/msg/blink-dev/YCIaYPa_DhI/RyDlcHmD1_wJ

Unfortunately, that is not without risk. This fix allows
::-webkit-media-controls { display: none; } to keep working until the
usage can be measured.

BUG=415407, 426759
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184652

[philipj_slow, 2014-10-29 10:04:31]

philipj@opera.com changed reviewers:
+ mstensho@opera.com, rune@opera.com

[philipj_slow, 2014-10-29 10:04:31]

philipj@opera.com changed required reviewers:
+ rune@opera.com

[philipj_slow, 2014-10-29 10:04:32]

PTAL

[mstensho (USE GERRIT), 2014-10-29 10:12:21]

https://codereview.chromium.org/689613002/diff/1/LayoutTests/media/controls-c...
File LayoutTests/media/controls-css-overload.html (right):

https://codereview.chromium.org/689613002/diff/1/LayoutTests/media/controls-c...
LayoutTests/media/controls-css-overload.html:2: <title>overloading controls CSS
doesn't crash</title>
Title not necessary.

Also, I really think it would be better to do a dumpAsText() instead of having a
reftest, and also have some text that says "PASS if no crash", or something.

[rune, 2014-10-29 10:14:18]

rune@opera.com changed required reviewers:
- rune@opera.com

[rune, 2014-10-29 10:14:19]

lgtm with nits. perhaps jchaffraix should have a say.

https://codereview.chromium.org/689613002/diff/1/LayoutTests/media/controls-c...
File LayoutTests/media/controls-css-overload-expected.html (right):

https://codereview.chromium.org/689613002/diff/1/LayoutTests/media/controls-c...
LayoutTests/media/controls-css-overload-expected.html:2: <title>overloading
controls CSS doesn't crash</title>
<p>

https://codereview.chromium.org/689613002/diff/1/LayoutTests/media/controls-c...
File LayoutTests/media/controls-css-overload.html (right):

https://codereview.chromium.org/689613002/diff/1/LayoutTests/media/controls-c...
LayoutTests/media/controls-css-overload.html:2: <title>overloading controls CSS
doesn't crash</title>
Drop <title> and make it a <p> to describe the pass condition.

https://codereview.chromium.org/689613002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderMedia.cpp (right):

https://codereview.chromium.org/689613002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMedia.cpp:84: // author style set display: inline we
would get an inline renderer as a
"author style sets"

[philipj_slow, 2014-10-29 10:31:21]

nits

[philipj_slow, 2014-10-29 10:32:03]

philipj@opera.com changed reviewers:
+ jchaffraix@chromium.org

[philipj_slow, 2014-10-29 10:32:03]

philipj@opera.com changed required reviewers:
+ jchaffraix@chromium.org

[philipj_slow, 2014-10-29 10:32:04]

Julien, can you PTAL?

[Julien - ping for review, 2014-10-29 18:42:14]

jchaffraix@chromium.org changed required reviewers:
- jchaffraix@chromium.org

[Julien - ping for review, 2014-10-29 18:42:15]

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderMedia.cpp (right):

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMedia.cpp:86: // check can be removed if
::-webkit-media-controls is made internal.
I really think this is peppering over the fact that we shouldn't allow people to
use this pseudo. I understand the need for a short-term solution in the wake of
its (potential) popularity but we should deprecate / use-count them as this is
engineering effort wasted on a feature that shouldn't be exposed anyway :(

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMedia.cpp:87: return child->isFlexibleBox();
How about display: inline-flex? I think Elliott's suggestion is a lot better
here:

::webkit-media-controls { display: flex !important; }

Authors can still style other properties differently but we ensure that we get
the proper child.

[philipj_slow, 2014-10-29 18:54:05]

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderMedia.cpp (right):

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMedia.cpp:86: // check can be removed if
::-webkit-media-controls is made internal.
On 2014/10/29 18:42:15, Julien Chaffraix - PST wrote:
> I really think this is peppering over the fact that we shouldn't allow people
to
> use this pseudo. I understand the need for a short-term solution in the wake
of
> its (potential) popularity but we should deprecate / use-count them as this is
> engineering effort wasted on a feature that shouldn't be exposed anyway :(

I agree, but failed to convince the API OWNERS. I can work on a CL to add
UseCounters for non-standard pseudo-classes and pseudo-elements if you'll review
them :)

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMedia.cpp:87: return child->isFlexibleBox();
On 2014/10/29 18:42:15, Julien Chaffraix - PST wrote:
> How about display: inline-flex? I think Elliott's suggestion is a lot better
> here:
> 
> ::webkit-media-controls { display: flex !important; }
> 
> Authors can still style other properties differently but we ensure that we get
> the proper child.

That would break pages that do ::webkit-media-controls { display: none; } to
hide the controls in fullscreen (they get added even if there's no controls
attribute) which is exactly what
http://css-tricks.com/custom-controls-in-html5-video-full-screen/ suggests. This
C++ fix only came about due to conservatism about stuff like that, so...

About inline-flex, that isn't what mediaControls.css uses, is it plausible that
pages could use it in some meaningful way? It seems far fetched to me, but if
you disagree I can expand the list of allowed child renders.

[philipj_slow, 2014-10-29 21:24:24]

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderMedia.cpp (right):

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMedia.cpp:86: // check can be removed if
::-webkit-media-controls is made internal.
On 2014/10/29 18:54:04, philipj wrote:
> On 2014/10/29 18:42:15, Julien Chaffraix - PST wrote:
> > I really think this is peppering over the fact that we shouldn't allow
people
> to
> > use this pseudo. I understand the need for a short-term solution in the wake
> of
> > its (potential) popularity but we should deprecate / use-count them as this
is
> > engineering effort wasted on a feature that shouldn't be exposed anyway :(
> 
> I agree, but failed to convince the API OWNERS. I can work on a CL to add
> UseCounters for non-standard pseudo-classes and pseudo-elements if you'll
review
> them :)

To be clear, that cannot block fixing the bug itself.

[Julien - ping for review, 2014-10-29 21:27:13]

The direction is good, let's just tighten (and test!) the couple of cases I
pointed out. lgtm.

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderMedia.cpp (right):

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMedia.cpp:86: // check can be removed if
::-webkit-media-controls is made internal.
On 2014/10/29 18:54:04, philipj wrote:
> On 2014/10/29 18:42:15, Julien Chaffraix - PST wrote:
> > I really think this is peppering over the fact that we shouldn't allow
people
> to
> > use this pseudo. I understand the need for a short-term solution in the wake
> of
> > its (potential) popularity but we should deprecate / use-count them as this
is
> > engineering effort wasted on a feature that shouldn't be exposed anyway :(
> 
> I agree, but failed to convince the API OWNERS. I can work on a CL to add
> UseCounters for non-standard pseudo-classes and pseudo-elements if you'll
review
> them :)

I am definitely supportive of more data on these! Sunsetting those proprietary
pseudos needs to happen and this is the best way forward for now.

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMedia.cpp:87: return child->isFlexibleBox();
On 2014/10/29 18:54:04, philipj wrote:
> On 2014/10/29 18:42:15, Julien Chaffraix - PST wrote:
> > How about display: inline-flex? I think Elliott's suggestion is a lot better
> > here:
> > 
> > ::webkit-media-controls { display: flex !important; }
> > 
> > Authors can still style other properties differently but we ensure that we
get
> > the proper child.
> 
> That would break pages that do ::webkit-media-controls { display: none; } to
> hide the controls in fullscreen (they get added even if there's no controls
> attribute) which is exactly what
> http://css-tricks.com/custom-controls-in-html5-video-full-screen/ suggests.
This
> C++ fix only came about due to conservatism about stuff like that, so...

OK, I thought we could get away with forcing display but you convinced me it
isn't good enough.

> About inline-flex, that isn't what mediaControls.css uses, is it plausible
that
> pages could use it in some meaningful way? It seems far fetched to me, but if
> you disagree I can expand the list of allowed child renders.

Your comment seems to imply that having an inline descendant (isInline()) is the
issue and inline-flex is definitely inline. If your issue is with having
something that's not a RenderFlexibleBox then you can discard my comment.

I don't think inline-flex would give us much too and this is security sensitive
so I wouldn't mind being on the conservative side.

Either way, we should probably document through testing what's allowed or not.

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMedia.cpp:88: }
I also forgot: RenderButton is a flexible box so it would pass this check. Do we
want to allow -webkit-appearance: button to work too?

[philipj_slow, 2014-10-29 21:40:37]

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderMedia.cpp (right):

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMedia.cpp:87: return child->isFlexibleBox();
On 2014/10/29 21:27:12, Julien Chaffraix - PST wrote:
> On 2014/10/29 18:54:04, philipj wrote:
> > On 2014/10/29 18:42:15, Julien Chaffraix - PST wrote:
> > > How about display: inline-flex? I think Elliott's suggestion is a lot
better
> > > here:
> > > 
> > > ::webkit-media-controls { display: flex !important; }
> > > 
> > > Authors can still style other properties differently but we ensure that we
> get
> > > the proper child.
> > 
> > That would break pages that do ::webkit-media-controls { display: none; } to
> > hide the controls in fullscreen (they get added even if there's no controls
> > attribute) which is exactly what
> > http://css-tricks.com/custom-controls-in-html5-video-full-screen/ suggests.
> This
> > C++ fix only came about due to conservatism about stuff like that, so...
> 
> OK, I thought we could get away with forcing display but you convinced me it
> isn't good enough.
> 
> > About inline-flex, that isn't what mediaControls.css uses, is it plausible
> that
> > pages could use it in some meaningful way? It seems far fetched to me, but
if
> > you disagree I can expand the list of allowed child renders.
> 
> Your comment seems to imply that having an inline descendant (isInline()) is
the
> issue and inline-flex is definitely inline. If your issue is with having
> something that's not a RenderFlexibleBox then you can discard my comment.

Truthfully, I'm not 100% confident that I've correctly diagnosed the root cause.
In discussion with Rune and Morten the fact that we have an inline as a child of
replaced content is what stood out as unusual, and they guessed that some
special handling was probably in place to make block content work that just
isn't there for inlines.

> I don't think inline-flex would give us much too and this is security
sensitive
> so I wouldn't mind being on the conservative side.
> 
> Either way, we should probably document through testing what's allowed or not.

Would a test case for every supported display value be sufficient, or are you
mostly worried about inline-flex? I just assumed that inline-flex would result
in something other than a RenderFlexibleBox, but have done no testing.

https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMedia.cpp:88: }
On 2014/10/29 21:27:12, Julien Chaffraix - PST wrote:
> I also forgot: RenderButton is a flexible box so it would pass this check. Do
we
> want to allow -webkit-appearance: button to work too?

Ugh, as long as it doesn't crash I don't think it matters. I guess you want a
test case to prove it doesn't crash :)

[philipj_slow, 2014-10-30 11:41:27]

test everything

[philipj_slow, 2014-10-30 11:41:58]

Rune, can you take a look at the new tests? I hope I covered everything this
time.

[rune, 2014-10-30 12:15:19]

https://codereview.chromium.org/689613002/diff/40001/LayoutTests/media/webkit...
File LayoutTests/media/webkit-media-controls-display-expected.html (right):

https://codereview.chromium.org/689613002/diff/40001/LayoutTests/media/webkit...
LayoutTests/media/webkit-media-controls-display-expected.html:14: var video =
document.createElement("video");
I'd probably just write out the video elements in markup instead of scripting
the reference, but this should be fine too.

https://codereview.chromium.org/689613002/diff/40001/LayoutTests/media/webkit...
File LayoutTests/media/webkit-media-controls-webkit-appearance-expected.html
(right):

https://codereview.chromium.org/689613002/diff/40001/LayoutTests/media/webkit...
LayoutTests/media/webkit-media-controls-webkit-appearance-expected.html:5:
position: absolute;
In this reference, this absolute positioned is painted behind the controls, but
that's only because the controls happen to establish a new stacking context. You
should add a z-index: -1 on the div to make sure this doesn't regress if we fix
the painting order for the controls.

[philipj_slow, 2014-10-30 12:28:14]

z-index

[philipj_slow, 2014-10-30 12:28:58]

https://codereview.chromium.org/689613002/diff/40001/LayoutTests/media/webkit...
File LayoutTests/media/webkit-media-controls-display-expected.html (right):

https://codereview.chromium.org/689613002/diff/40001/LayoutTests/media/webkit...
LayoutTests/media/webkit-media-controls-display-expected.html:14: var video =
document.createElement("video");
On 2014/10/30 12:15:19, rune wrote:
> I'd probably just write out the video elements in markup instead of scripting
> the reference, but this should be fine too.

I thought this way would make it more explicit which display values would result
in visible controls.

https://codereview.chromium.org/689613002/diff/40001/LayoutTests/media/webkit...
File LayoutTests/media/webkit-media-controls-webkit-appearance-expected.html
(right):

https://codereview.chromium.org/689613002/diff/40001/LayoutTests/media/webkit...
LayoutTests/media/webkit-media-controls-webkit-appearance-expected.html:5:
position: absolute;
On 2014/10/30 12:15:19, rune wrote:
> In this reference, this absolute positioned is painted behind the controls,
but
> that's only because the controls happen to establish a new stacking context.
You
> should add a z-index: -1 on the div to make sure this doesn't regress if we
fix
> the painting order for the controls.

Oh, my assumption was that the video would be painted above the div because it's
later in the DOM tree. I know nothing about painting order, will add z-index.

[philipj_slow, 2014-10-30 12:29:14]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-10-30 12:30:01]

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

[commit-bot: I haz the power, 2014-10-30 14:38:07]

Committed patchset #4 (id:60001) as 184652