Protect MediaValuesCached creation when RenderView is missing

Protect MediaValuesCached creation when RenderView is missing

This CL protects MediaValuesCached creation with a frame which document doesn't have a render view.

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

[Yoav Weiss, 2014-04-25 23:24:13]

This is a speculative fix for
https://code.google.com/p/chromium/issues/detail?id=367210

[eseidel, 2014-04-25 23:53:24]

not lgtm.

https://codereview.chromium.org/252683007/diff/1/Source/core/css/MediaValues.cpp
File Source/core/css/MediaValues.cpp (right):

https://codereview.chromium.org/252683007/diff/1/Source/core/css/MediaValues....
Source/core/css/MediaValues.cpp:36: RenderStyle* style =
(RenderStyle*)(frame->document()->renderView());
Huh!?  That's not a style.

[eseidel, 2014-04-26 05:04:05]

It looks like the real error is here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

That it's not checking if the Document has a non-null frame.

https://codereview.chromium.org/252683007/diff/20001/Source/core/css/MediaVal...
File Source/core/css/MediaValues.cpp (right):

https://codereview.chromium.org/252683007/diff/20001/Source/core/css/MediaVal...
Source/core/css/MediaValues.cpp:36: RenderView* view =
(frame->document()->renderView());
The parens aren't needed.

[Yoav Weiss, 2014-04-26 06:42:19]

On 2014/04/26 05:04:05, eseidel wrote:
> It looks like the real error is here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> That it's not checking if the Document has a non-null frame.

The null frame check happens a little further down the line:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

> 
>
https://codereview.chromium.org/252683007/diff/20001/Source/core/css/MediaVal...
> File Source/core/css/MediaValues.cpp (right):
> 
>
https://codereview.chromium.org/252683007/diff/20001/Source/core/css/MediaVal...
> Source/core/css/MediaValues.cpp:36: RenderView* view =
> (frame->document()->renderView());
> The parens aren't needed.
I'll remove them.

[Yoav Weiss, 2014-04-28 20:12:20]

On 2014/04/26 06:42:19, Yoav Weiss wrote:
> On 2014/04/26 05:04:05, eseidel wrote:
> > It looks like the real error is here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > That it's not checking if the Document has a non-null frame.
> 
> The null frame check happens a little further down the line:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> > 
> >
>
https://codereview.chromium.org/252683007/diff/20001/Source/core/css/MediaVal...
> > File Source/core/css/MediaValues.cpp (right):
> > 
> >
>
https://codereview.chromium.org/252683007/diff/20001/Source/core/css/MediaVal...
> > Source/core/css/MediaValues.cpp:36: RenderView* view =
> > (frame->document()->renderView());
> > The parens aren't needed.
> I'll remove them.

ping

[eseidel, 2014-04-28 22:57:14]

I still don't understand how we have a document which does not have a RenderView
which we're processing media queries on.  If perhaps we should have stopped this
process earlier.

It's not clear that this is the right layer to null-check at.

[Yoav Weiss, 2014-04-29 06:14:57]

On 2014/04/28 22:57:14, eseidel wrote:
> I still don't understand how we have a document which does not have a
RenderView
> which we're processing media queries on.  If perhaps we should have stopped
this
> process earlier.
> 
> It's not clear that this is the right layer to null-check at.

I've removed the RenderView null check. I don't think that this is the issue we
see. I think that what we see is related to a missing RenderStyle.

[Yoav Weiss, 2014-04-29 06:15:43]

On 2014/04/29 06:14:57, Yoav Weiss wrote:
> On 2014/04/28 22:57:14, eseidel wrote:
> > I still don't understand how we have a document which does not have a
> RenderView
> > which we're processing media queries on.  If perhaps we should have stopped
> this
> > process earlier.
> > 
> > It's not clear that this is the right layer to null-check at.
> 
> I've removed the RenderView null check.
By removed, I meant "turned it into an assert"

> I don't think that this is the issue we
> see. I think that what we see is related to a missing RenderStyle.

[eseidel, 2014-04-29 17:34:27]

Renders can't exist w/o RenderStyles.

The crash stack showed this as being part of XHR.  XHR documents don't have a
frame.  So I'm still not sure how you're getting to this crash.  You can't have
any RenderObject w/o it also having a RenderStyle.

[Yoav Weiss, 2014-04-30 20:07:18]

Added a null check for frame->view() (along with an ASSERT) as discussed on irc.

[Yoav Weiss, 2014-04-30 20:17:09]

On 2014/04/30 20:07:18, Yoav Weiss wrote:
> Added a null check for frame->view() (along with an ASSERT) as discussed on
irc.

Removed the assert

[eseidel, 2014-04-30 21:01:32]

lgtm

[commit-bot: I haz the power, 2014-04-30 21:02:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/252683007/100001

[Yoav Weiss, 2014-04-30 21:08:21]

The CQ bit was unchecked by yoav@yoav.ws

[Yoav Weiss, 2014-04-30 21:11:26]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-04-30 21:11:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/252683007/120001

[commit-bot: I haz the power, 2014-04-30 22:15:18]

Change committed as 173037