Fixes for two different HUD issues related to win32k lockdown

Fixes for two different HUD issues related to win32k lockdown

1. Using a GDI bitmap -- switch to SkSurface instead

2. Lazily loading a font. This breaks for two different reasons.
The first is a DCHECK for doing IO on the Compositor thread. This
could have been just ignored in general since it's only for debug
usage, the drawbacks of minor jank are limited.

However, once lockdown is on, the font load hard fails from the
renderer. So, load the typeface before lockdown and pass it into
CC and the hud via LayerTreeSettings.

R=danakj@chromium.org
BUG=453731, 455104
TEST=run chrome with --enable-win32k-renderer-lockdown --show-fps-counter --ui-show-fps-counter on Win8

Committed: https://crrev.com/616a8e656b2d923ed393bde86adc150e555ba799
Cr-Commit-Position: refs/heads/master@{#314929}

[scottmg, 2015-02-05 00:36:29]

Patchset #2 (id:20001) has been deleted

[scottmg, 2015-02-05 00:40:52]

Patchset #2 (id:40001) has been deleted

[scottmg, 2015-02-05 00:40:57]

Patchset #2 (id:60001) has been deleted

[scottmg, 2015-02-05 01:24:57]

scottmg@chromium.org changed reviewers:
+ danakj@chromium.org

[scottmg, 2015-02-05 01:25:16]

Is there a better way to get that SkTypeface* through?

[enne (OOO), 2015-02-05 01:30:27]

enne@chromium.org changed reviewers:
+ enne@chromium.org
- danakj@chromium.org

[enne (OOO), 2015-02-05 01:30:28]

Thanks for this! Sorry for all the plumbing.

https://codereview.chromium.org/900063002/diff/80001/cc/layers/heads_up_displ...
File cc/layers/heads_up_display_layer_impl.h (right):

https://codereview.chromium.org/900063002/diff/80001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.h:134: SkCanvas* hud_canvas_;
Can you make this a RefPtr or just access it off the surface when you need it,
rather than having a raw pointer member variable?

https://codereview.chromium.org/900063002/diff/80001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.h:136: SkTypeface* typeface_;
Can this be a skia::RefPtr all the way through the plumbing? For this browser
compositor case, I think this won't break, but cc has other embedders, so this
seems like a use-after-free waiting to happen.

https://codereview.chromium.org/900063002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_client.h (right):

https://codereview.chromium.org/900063002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_client.h:67: virtual SkTypeface* GetHudTypeface() = 0;
This is here (vs LayerTreeSettings) so that the font can be initialized lazily
for non-Windows platforms?

[scottmg, 2015-02-05 03:12:25]

Patchset #3 (id:100001) has been deleted

[scottmg, 2015-02-05 03:24:10]

Patchset #3 (id:120001) has been deleted

[scottmg, 2015-02-05 03:26:12]

Patchset #3 (id:130001) has been deleted

[scottmg, 2015-02-05 03:28:03]

Patchset #3 (id:150001) has been deleted

[scottmg, 2015-02-05 03:28:32]

Thanks!

https://codereview.chromium.org/900063002/diff/80001/cc/layers/heads_up_displ...
File cc/layers/heads_up_display_layer_impl.h (right):

https://codereview.chromium.org/900063002/diff/80001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.h:134: SkCanvas* hud_canvas_;
On 2015/02/05 01:30:28, enne wrote:
> Can you make this a RefPtr or just access it off the surface when you need it,
> rather than having a raw pointer member variable?

Done.

https://codereview.chromium.org/900063002/diff/80001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.h:136: SkTypeface* typeface_;
On 2015/02/05 01:30:28, enne wrote:
> Can this be a skia::RefPtr all the way through the plumbing? For this browser
> compositor case, I think this won't break, but cc has other embedders, so this
> seems like a use-after-free waiting to happen.

I did the Windows one first where it's leaked early so weak seemed OK. Done.

https://codereview.chromium.org/900063002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_client.h (right):

https://codereview.chromium.org/900063002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_client.h:67: virtual SkTypeface* GetHudTypeface() = 0;
On 2015/02/05 01:30:28, enne wrote:
> This is here (vs LayerTreeSettings) so that the font can be initialized lazily
> for non-Windows platforms?

LayerTreeSettings seems fine if that's OK w/ you. I almost put it there but it
seemed like ... just bools and numbers so wasn't sure. Much less plumbing to
have it there. :)

[enne (OOO), 2015-02-05 17:54:44]

lgtm

https://codereview.chromium.org/900063002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_client.h (right):

https://codereview.chromium.org/900063002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_client.h:67: virtual SkTypeface* GetHudTypeface() = 0;
On 2015/02/05 01:30:28, enne wrote:
> This is here (vs LayerTreeSettings) so that the font can be initialized lazily
> for non-Windows platforms?

Or, said differently, if lazy initialization isn't a goal, I'd prefer it to be a
LayerTreeSetting or even just a setter on LayerTreeHost that got called
unconditionally.

If it is a goal, I can't really think of a better place to put a function like
that, so this is probably ok.

[enne (OOO), 2015-02-05 18:07:12]

^ oh code review tool publishing stale comments when I hit the lgtm button

[scottmg, 2015-02-05 18:12:30]

On 2015/02/05 17:54:44, enne wrote:
> lgtm
> 
>
https://codereview.chromium.org/900063002/diff/80001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_client.h (right):
> 
>
https://codereview.chromium.org/900063002/diff/80001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_client.h:67: virtual SkTypeface* GetHudTypeface() =
0;
> On 2015/02/05 01:30:28, enne wrote:
> > This is here (vs LayerTreeSettings) so that the font can be initialized
lazily
> > for non-Windows platforms?
> 
> Or, said differently, if lazy initialization isn't a goal, I'd prefer it to be
a
> LayerTreeSetting or even just a setter on LayerTreeHost that got called
> unconditionally.

We've been creating it non-lazily on each hud layer constructor 'til now, so
doesn't seem to urgent to make it lazy.

> 
> If it is a goal, I can't really think of a better place to put a function like
> that, so this is probably ok.

[scottmg, 2015-02-05 18:16:15]

On 2015/02/05 18:07:12, enne wrote:
> ^ oh code review tool publishing stale comments when I hit the lgtm button

Ah. :)

Apparently android webview is a thing ... that doesn't have any C++ ui/. So
render_widget_compositor.cc can't set up the LayerTreeSettings that way.

[scottmg, 2015-02-05 19:46:15]

New patchsets have been uploaded after l-g-t-m from enne@chromium.org

[scottmg, 2015-02-05 19:52:08]

Patchset #5 (id:210001) has been deleted

[scottmg, 2015-02-05 20:30:18]

Patchset #6 (id:250001) has been deleted

[scottmg, 2015-02-05 20:35:46]

Patchset #6 (id:270001) has been deleted

[scottmg, 2015-02-05 20:52:43]

Moved the font to ui/gfx/hud_font.cc/h instead for android webview.

+danakj for ui/ OWNERS
+piman content/ OWNERS

[scottmg, 2015-02-05 20:53:06]

scottmg@chromium.org changed reviewers:
+ danakj@chromium.org, piman@chromium.org

[scottmg, 2015-02-05 20:53:07]

Moved the font to ui/gfx/hud_font.cc/h instead for android webview.

+danakj for ui/ OWNERS
+piman content/ OWNERS

[piman, 2015-02-05 21:34:38]

lgtm

[danakj, 2015-02-05 21:40:43]

https://codereview.chromium.org/900063002/diff/290001/ui/gfx/hud_font.h
File ui/gfx/hud_font.h (right):

https://codereview.chromium.org/900063002/diff/290001/ui/gfx/hud_font.h#newco...
ui/gfx/hud_font.h:16: GFX_EXPORT void SetHudTypeface(SkTypeface* typeface);
Can you use skia::RefPtr instead of a raw pointer and comment about ownership?

[scottmg, 2015-02-05 22:49:11]

New patchsets have been uploaded after l-g-t-m from piman@chromium.org

[scottmg, 2015-02-05 22:50:42]

https://codereview.chromium.org/900063002/diff/290001/ui/gfx/hud_font.h
File ui/gfx/hud_font.h (right):

https://codereview.chromium.org/900063002/diff/290001/ui/gfx/hud_font.h#newco...
ui/gfx/hud_font.h:16: GFX_EXPORT void SetHudTypeface(SkTypeface* typeface);
On 2015/02/05 21:40:42, danakj wrote:
> Can you use skia::RefPtr instead of a raw pointer and comment about ownership?

Done.

[danakj, 2015-02-05 22:56:14]

LGTM

https://codereview.chromium.org/900063002/diff/310001/ui/gfx/hud_font.cc
File ui/gfx/hud_font.cc (right):

https://codereview.chromium.org/900063002/diff/310001/ui/gfx/hud_font.cc#newc...
ui/gfx/hud_font.cc:21: #if !defined(OS_WIN)
can we do like

#if defined(OS_WIN)
DCHECK(g_hud_typeface.Get());
#else
 .. make a typeface ..
#endif

so if it didn't happen we'll DCHECK closer to the problem?

[danakj, 2015-02-05 22:56:47]

https://codereview.chromium.org/900063002/diff/310001/content/renderer/render...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/900063002/diff/310001/content/renderer/render...
content/renderer/renderer_main_platform_delegate_win.cc:60: SkTypeface* typeface
=
bonus points for doing the AdoptRef here, though.

[scottmg, 2015-02-05 23:21:57]

https://codereview.chromium.org/900063002/diff/310001/content/renderer/render...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/900063002/diff/310001/content/renderer/render...
content/renderer/renderer_main_platform_delegate_win.cc:60: SkTypeface* typeface
=
On 2015/02/05 22:56:47, danakj wrote:
> bonus points for doing the AdoptRef here, though.

This one has to leak for the sandbox, so I'd have to store the refptr in a
global or something. So I'm going to leave this one as is.

https://codereview.chromium.org/900063002/diff/310001/ui/gfx/hud_font.cc
File ui/gfx/hud_font.cc (right):

https://codereview.chromium.org/900063002/diff/310001/ui/gfx/hud_font.cc#newc...
ui/gfx/hud_font.cc:21: #if !defined(OS_WIN)
On 2015/02/05 22:56:13, danakj wrote:
> can we do like
> 
> #if defined(OS_WIN)
> DCHECK(g_hud_typeface.Get());
> #else
>  .. make a typeface ..
> #endif
> 
> so if it didn't happen we'll DCHECK closer to the problem?

No, unfortunately not. This is used in the browser too and NULL is OK there.

[scottmg, 2015-02-05 23:23:24]

The CQ bit was checked by scottmg@chromium.org

[danakj, 2015-02-05 23:24:11]

https://codereview.chromium.org/900063002/diff/310001/content/renderer/render...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/900063002/diff/310001/content/renderer/render...
content/renderer/renderer_main_platform_delegate_win.cc:60: SkTypeface* typeface
=
On 2015/02/05 23:21:57, scottmg wrote:
> On 2015/02/05 22:56:47, danakj wrote:
> > bonus points for doing the AdoptRef here, though.
> 
> This one has to leak for the sandbox, so I'd have to store the refptr in a
> global or something. So I'm going to leave this one as is.

Then I'm concerned about below. You adopt-ref it there. So this pointer will be
deleted when the HudTypeface goes away (tho it's a global so never).

I'm not sure how moving the AdoptRef from there to here would change lifetimes,
though, so is something wrong going on now?

[commit-bot: I haz the power, 2015-02-05 23:24:39]

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

[danakj, 2015-02-05 23:25:07]

On 2015/02/05 23:21:57, scottmg wrote:
>
https://codereview.chromium.org/900063002/diff/310001/content/renderer/render...
> File content/renderer/renderer_main_platform_delegate_win.cc (right):
> 
>
https://codereview.chromium.org/900063002/diff/310001/content/renderer/render...
> content/renderer/renderer_main_platform_delegate_win.cc:60: SkTypeface*
typeface
> =
> On 2015/02/05 22:56:47, danakj wrote:
> > bonus points for doing the AdoptRef here, though.
> 
> This one has to leak for the sandbox, so I'd have to store the refptr in a
> global or something. So I'm going to leave this one as is.
> 
> https://codereview.chromium.org/900063002/diff/310001/ui/gfx/hud_font.cc
> File ui/gfx/hud_font.cc (right):
> 
>
https://codereview.chromium.org/900063002/diff/310001/ui/gfx/hud_font.cc#newc...
> ui/gfx/hud_font.cc:21: #if !defined(OS_WIN)
> On 2015/02/05 22:56:13, danakj wrote:
> > can we do like
> > 
> > #if defined(OS_WIN)
> > DCHECK(g_hud_typeface.Get());
> > #else
> >  .. make a typeface ..
> > #endif
> > 
> > so if it didn't happen we'll DCHECK closer to the problem?
> 
> No, unfortunately not. This is used in the browser too and NULL is OK there.

Then should we move the if (typeface) out of the ifdef? Will the browser
compositor with --show-ui-fps-counter crash because it has no typeface?

[scottmg, 2015-02-05 23:29:53]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2015-02-05 23:33:26]

On 2015/02/05 23:25:07, danakj wrote:
> On 2015/02/05 23:21:57, scottmg wrote:
> >
>
https://codereview.chromium.org/900063002/diff/310001/content/renderer/render...
> > File content/renderer/renderer_main_platform_delegate_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/900063002/diff/310001/content/renderer/render...
> > content/renderer/renderer_main_platform_delegate_win.cc:60: SkTypeface*
> typeface
> > =
> > On 2015/02/05 22:56:47, danakj wrote:
> > > bonus points for doing the AdoptRef here, though.
> > 
> > This one has to leak for the sandbox, so I'd have to store the refptr in a
> > global or something. So I'm going to leave this one as is.
> > 
> > https://codereview.chromium.org/900063002/diff/310001/ui/gfx/hud_font.cc
> > File ui/gfx/hud_font.cc (right):
> > 
> >
>
https://codereview.chromium.org/900063002/diff/310001/ui/gfx/hud_font.cc#newc...
> > ui/gfx/hud_font.cc:21: #if !defined(OS_WIN)
> > On 2015/02/05 22:56:13, danakj wrote:
> > > can we do like
> > > 
> > > #if defined(OS_WIN)
> > > DCHECK(g_hud_typeface.Get());
> > > #else
> > >  .. make a typeface ..
> > > #endif
> > > 
> > > so if it didn't happen we'll DCHECK closer to the problem?
> > 
> > No, unfortunately not. This is used in the browser too and NULL is OK there.
> 
> Then should we move the if (typeface) out of the ifdef? Will the browser
> compositor with --show-ui-fps-counter crash because it has no typeface?

Ah, thanks! So many flags I've never heard of. :) It doesn't crash with
--ui-show-fps-counter, because Skia falls back to some default font (which
succeeds because it's in the browser), but it does look a bit ugly because it
doesn't choose a nice font.

[scottmg, 2015-02-05 23:34:15]

New patchsets have been uploaded after l-g-t-m from danakj@chromium.org

[danakj, 2015-02-05 23:43:41]

https://codereview.chromium.org/900063002/diff/330001/content/renderer/render...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/900063002/diff/330001/content/renderer/render...
content/renderer/renderer_main_platform_delegate_win.cc:68:
ui::SetHudTypeface(skia::AdoptRef(typeface));
Thanks, that looks good. Still curious how doing the adopt at the site of the
create changes the lifetime here?

[scottmg, 2015-02-05 23:47:55]

https://codereview.chromium.org/900063002/diff/330001/content/renderer/render...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/900063002/diff/330001/content/renderer/render...
content/renderer/renderer_main_platform_delegate_win.cc:68:
ui::SetHudTypeface(skia::AdoptRef(typeface));
On 2015/02/05 23:43:40, danakj wrote:
> Thanks, that looks good. Still curious how doing the adopt at the site of the
> create changes the lifetime here?

Oh, sorry, you mean at line 65? My brain is slow today it seems. I thought you
meant for the "Times New Roman" one.

Done.

[danakj, 2015-02-05 23:49:47]

Thanks! LGTM!

https://codereview.chromium.org/900063002/diff/350001/content/renderer/render...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/900063002/diff/350001/content/renderer/render...
content/renderer/renderer_main_platform_delegate_win.cc:65:
skia::RefPtr<SkTypeface> hud_typeface =
OH! I failed to read that there were 2 different typefaces here. Woops.

[scottmg, 2015-02-05 23:53:12]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2015-02-05 23:54:36]

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

[commit-bot: I haz the power, 2015-02-06 00:52:43]

Committed patchset #9 (id:350001)

[commit-bot: I haz the power, 2015-02-06 00:53:43]

Patchset 9 (id:??) landed as
https://crrev.com/616a8e656b2d923ed393bde86adc150e555ba799
Cr-Commit-Position: refs/heads/master@{#314929}

[scottmg, 2015-02-23 16:41:14]

A revert of this CL (patchset #9 id:350001) has been created in
https://codereview.chromium.org/922293006/ by scottmg@chromium.org.

The reason for reverting is: Causes regression in cold start on Android and
maybe Mac.

Details in http://crbug.com/457248..

[danakj, 2015-02-23 17:42:20]

> 1. Using a GDI bitmap -- switch to SkSurface instead

We should def split this off and reland it. Should have asked for 2 patches at
the time :(

[scottmg, 2015-02-23 17:55:08]

On 2015/02/23 17:42:20, danakj wrote:
> > 1. Using a GDI bitmap -- switch to SkSurface instead
> 
> We should def split this off and reland it. Should have asked for 2 patches at
> the time :(

They both need to be relanded before 42. I'll do the two pieces separately.

[enne (OOO), 2015-02-23 18:11:03]

I'm assuming the cold start regression is because the font is created every time
instead of just when the HUD gets turned on?  Is the answer to do something more
like your original patch where for non-Win platforms it gets created lazily?

Alternatively, is there another font that's already loaded that we could use
instead?

[scottmg, 2015-02-23 18:14:59]

On 2015/02/23 18:11:03, enne wrote:
> I'm assuming the cold start regression is because the font is created every
time
> instead of just when the HUD gets turned on?  Is the answer to do something
more
> like your original patch where for non-Win platforms it gets created lazily?

Yeah, I guess that must be it. I wasn't able to repro the regression locally.

I was thinking of replacing the LayerTreeSettings `skia::RefPtr<SkTypeface>
hud_typeface` with `skia::RefPtr<SkTypeface> (*get_hud_typeface)()`, where
that's pre-created only on Windows. Does that seem OK?

> 
> Alternatively, is there another font that's already loaded that we could use
> instead?

I don't know of one.