Add skia extension to allow setting default fontmgr on linux. Use it to allow the linux blimp clien…

Add skia extension to allow setting default fontmgr on linux. Use it to allow the linux blimp client to use android fonts

BUG=617821
Committed: https://crrev.com/f6c1cd2bb6a2253e9547c091671c98ed28300685
Committed: https://crrev.com/deb6095223d0ee029136cb1708d81c3d91257e0b
Cr-Original-Commit-Position: refs/heads/master@{#432023}
Cr-Commit-Position: refs/heads/master@{#433292}

[steimel, 2016-11-11 23:28:06]

steimel@chromium.org changed reviewers:
+ bungeman@chromium.org, khushalsagar@chromium.org

[steimel, 2016-11-11 23:28:06]

Finally getting around to this after putting on the backburner for Helium
development. PTAL

[steimel, 2016-11-12 00:10:49]

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

[commit-bot: I haz the power, 2016-11-12 00:11:14]

Dry run: 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-11-12 00:16:27]

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

[commit-bot: I haz the power, 2016-11-12 00:16:27]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[steimel, 2016-11-12 01:31:58]

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

[commit-bot: I haz the power, 2016-11-12 01:32:18]

Dry run: 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-11-12 03:06:11]

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

[commit-bot: I haz the power, 2016-11-12 03:06:11]

Dry run: This issue passed the CQ dry run.

[bungeman-chromium, 2016-11-14 16:03:48]

https://codereview.chromium.org/2500643002/diff/20001/blimp/client/app/linux/...
File blimp/client/app/linux/blimp_main.cc (right):

https://codereview.chromium.org/2500643002/diff/20001/blimp/client/app/linux/...
blimp/client/app/linux/blimp_main.cc:63: std::unique_ptr<SkFontMgr>
CreateAndroidFontMgr(std::string android_fonts_dir) {
I think you want to use sk_sp<SkFontMgr> here. SkFontMgr is reference counted
and unique_ptr will just delete it.

https://codereview.chromium.org/2500643002/diff/20001/blimp/client/app/linux/...
blimp/client/app/linux/blimp_main.cc:98: std::unique_ptr<SkFontMgr>
android_font_manager = SetupAndroidFontManager();
I think the only reason you have this otherwise unused variable here is to keep
the unique_ptr alive so you don't inadvertently delete the global. If you change
to sk_sp<SkFontMgr> above, then I don't think you need this variable at all.
(Nor does SetupAndroidFontManager need to return anything).

https://codereview.chromium.org/2500643002/diff/20001/skia/ext/fontmgr_defaul...
File skia/ext/fontmgr_default_linux.cc (right):

https://codereview.chromium.org/2500643002/diff/20001/skia/ext/fontmgr_defaul...
skia/ext/fontmgr_default_linux.cc:12: SkFontMgr* g_default_fontmgr;
I'm not sure if we care about global destructors here. If we do care about
global destructors it's fine to leave this as very local owning bare pointer
(with a note that it is).

https://codereview.chromium.org/2500643002/diff/20001/skia/ext/fontmgr_defaul...
skia/ext/fontmgr_default_linux.cc:15: void SetDefaultSkiaFactory(SkFontMgr*
fontmgr) {
Since this is taking ownership, this should take an sk_sp<SkFontMgr>.

We're still updating the Skia API to std::unique_ptr and sk_sp (for shared
ownership) and haven't quite gotten to the SkFontMgr interface yet, but since
this is nice and self contained it would be good to state the ownership
explicitly since we can.

[steimel, 2016-11-14 18:04:43]

https://codereview.chromium.org/2500643002/diff/20001/blimp/client/app/linux/...
File blimp/client/app/linux/blimp_main.cc (right):

https://codereview.chromium.org/2500643002/diff/20001/blimp/client/app/linux/...
blimp/client/app/linux/blimp_main.cc:63: std::unique_ptr<SkFontMgr>
CreateAndroidFontMgr(std::string android_fonts_dir) {
On 2016/11/14 16:03:48, bungeman-chromium wrote:
> I think you want to use sk_sp<SkFontMgr> here. SkFontMgr is reference counted
> and unique_ptr will just delete it.

Done.

https://codereview.chromium.org/2500643002/diff/20001/blimp/client/app/linux/...
blimp/client/app/linux/blimp_main.cc:98: std::unique_ptr<SkFontMgr>
android_font_manager = SetupAndroidFontManager();
On 2016/11/14 16:03:48, bungeman-chromium wrote:
> I think the only reason you have this otherwise unused variable here is to
keep
> the unique_ptr alive so you don't inadvertently delete the global. If you
change
> to sk_sp<SkFontMgr> above, then I don't think you need this variable at all.
> (Nor does SetupAndroidFontManager need to return anything).

Yep. Done.

https://codereview.chromium.org/2500643002/diff/20001/skia/ext/fontmgr_defaul...
File skia/ext/fontmgr_default_linux.cc (right):

https://codereview.chromium.org/2500643002/diff/20001/skia/ext/fontmgr_defaul...
skia/ext/fontmgr_default_linux.cc:12: SkFontMgr* g_default_fontmgr;
On 2016/11/14 16:03:48, bungeman-chromium wrote:
> I'm not sure if we care about global destructors here. If we do care about
> global destructors it's fine to leave this as very local owning bare pointer
> (with a note that it is).

I'm not sure I understand what you mean, but I've changed this to hold the sk_sp
since I'm no longer having the caller retain ownership with the unique_ptr.

https://codereview.chromium.org/2500643002/diff/20001/skia/ext/fontmgr_defaul...
skia/ext/fontmgr_default_linux.cc:15: void SetDefaultSkiaFactory(SkFontMgr*
fontmgr) {
On 2016/11/14 16:03:48, bungeman-chromium wrote:
> Since this is taking ownership, this should take an sk_sp<SkFontMgr>.
> 
> We're still updating the Skia API to std::unique_ptr and sk_sp (for shared
> ownership) and haven't quite gotten to the SkFontMgr interface yet, but since
> this is nice and self contained it would be good to state the ownership
> explicitly since we can.

Done.

[bungeman-chromium, 2016-11-14 18:10:03]

https://codereview.chromium.org/2500643002/diff/40001/skia/ext/fontmgr_defaul...
File skia/ext/fontmgr_default_linux.cc (right):

https://codereview.chromium.org/2500643002/diff/40001/skia/ext/fontmgr_defaul...
skia/ext/fontmgr_default_linux.cc:21: return g_default_fontmgr.get();
This needs to return a reference (sorry again about not already making this
method return sk_sp yet). As a result, this needs to be

return SkRef(g_default_fontmgr.get());

otherwise the global isn't holding a reference anymore.

[steimel, 2016-11-14 18:15:11]

Thanks for your help!

https://codereview.chromium.org/2500643002/diff/40001/skia/ext/fontmgr_defaul...
File skia/ext/fontmgr_default_linux.cc (right):

https://codereview.chromium.org/2500643002/diff/40001/skia/ext/fontmgr_defaul...
skia/ext/fontmgr_default_linux.cc:21: return g_default_fontmgr.get();
On 2016/11/14 18:10:03, bungeman-chromium wrote:
> This needs to return a reference (sorry again about not already making this
> method return sk_sp yet). As a result, this needs to be
> 
> return SkRef(g_default_fontmgr.get());
> 
> otherwise the global isn't holding a reference anymore.

Done.

[bungeman-chromium, 2016-11-14 18:22:05]

The Skia parts at least lgtm. Don't know much about how the back-end works
though.

[bungeman-skia, 2016-11-14 18:22:42]

bungeman@google.com changed reviewers:
+ bungeman@google.com

[bungeman-skia, 2016-11-14 18:22:43]

OWNER lgtm for skia/.

[Khushal, 2016-11-14 20:30:08]

On 2016/11/14 18:22:43, bungeman-skia wrote:
> OWNER lgtm for skia/.

blimp LGTM.

[steimel, 2016-11-14 21:39:10]

The CQ bit was checked by steimel@chromium.org

[commit-bot: I haz the power, 2016-11-14 21:42:45]

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-11-15 00:41:31]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-15 00:50:33]

Description was changed from

==========
Add skia extension to allow setting default fontmgr on linux. Use it to allow
the linux blimp client to use android fonts

BUG=617821
==========

to

==========
Add skia extension to allow setting default fontmgr on linux. Use it to allow
the linux blimp client to use android fonts

BUG=617821

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

[commit-bot: I haz the power, 2016-11-15 00:50:35]

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

[horo, 2016-11-15 03:46:01]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2503763002/ by horo@chromium.org.

The reason for reverting is: Caused failures at "sizes" check on Linux x64 bot.

https://uberchromegw.corp.google.com/i/chromium/builders/Linux%20x64

See crbug.com/665274

BUG=665274.

[steimel, 2016-11-18 19:09:49]

Description was changed from

==========
Add skia extension to allow setting default fontmgr on linux. Use it to allow
the linux blimp client to use android fonts

BUG=617821

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

to

==========
Add skia extension to allow setting default fontmgr on linux. Use it to allow
the linux blimp client to use android fonts

BUG=617821

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

[steimel, 2016-11-18 19:10:11]

The CQ bit was checked by steimel@chromium.org

[steimel, 2016-11-18 19:10:12]

The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com,
bungeman@chromium.org, khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2500643002/#ps80001
(title: "Change fontmgr to bare pointer to avoid global object/dtor")

[commit-bot: I haz the power, 2016-11-18 19:11:01]

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

[steimel, 2016-11-18 19:42:21]

The CQ bit was unchecked by steimel@chromium.org

[steimel, 2016-11-18 19:43:29]

The CQ bit was checked by steimel@chromium.org

[commit-bot: I haz the power, 2016-11-18 19:44:56]

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-11-18 20:57:13]

Description was changed from

==========
Add skia extension to allow setting default fontmgr on linux. Use it to allow
the linux blimp client to use android fonts

BUG=617821

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

to

==========
Add skia extension to allow setting default fontmgr on linux. Use it to allow
the linux blimp client to use android fonts

BUG=617821

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

[commit-bot: I haz the power, 2016-11-18 20:57:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-18 20:59:22]

Description was changed from

==========
Add skia extension to allow setting default fontmgr on linux. Use it to allow
the linux blimp client to use android fonts

BUG=617821

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

to

==========
Add skia extension to allow setting default fontmgr on linux. Use it to allow
the linux blimp client to use android fonts

BUG=617821

Committed: https://crrev.com/f6c1cd2bb6a2253e9547c091671c98ed28300685
Committed: https://crrev.com/deb6095223d0ee029136cb1708d81c3d91257e0b
Cr-Original-Commit-Position: refs/heads/master@{#432023}
Cr-Commit-Position: refs/heads/master@{#433292}
==========

[commit-bot: I haz the power, 2016-11-18 20:59:23]

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

[krasin1, 2016-11-19 01:17:20]

For the record, this has broken CFI Linux ToT and CFI Linux Full bots:
https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/4...

At the first glance, there's a bad cast or something similar in
SkLinearBitmapPipeline::chooseBlenderForShading, but I am yet to reproduce it
locally.

[krasin1, 2016-11-19 01:20:49]

On 2016/11/19 01:17:20, krasin1 wrote:
> For the record, this has broken CFI Linux ToT and CFI Linux Full bots:
>
https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/4...
> 
> At the first glance, there's a bad cast or something similar in
> SkLinearBitmapPipeline::chooseBlenderForShading, but I am yet to reproduce it
> locally.

And the bug seems to be introduced in
https://skia-review.googlesource.com/c/4987/5/src/core/SkLinearBitmapPipeline...
My current blind guess is that the allocator is already destroyed by this point
(or not yet initialized):

Blender* SkLinearBitmapPipeline::chooseBlenderForShading(
    SkAlphaType alphaType,
    float postAlpha,
    SkFallbackAlloc* allocator)
{
    if (alphaType == kUnpremul_SkAlphaType) {
        return allocator->make<SrcFPPixel<kUnpremul_SkAlphaType>>(postAlpha);
    } else {
        // kOpaque_SkAlphaType is treated the same as kPremul_SkAlphaType
        return allocator->make<SrcFPPixel<kPremul_SkAlphaType>>(postAlpha);
    }
}

[krasin1, 2016-11-19 02:06:50]

Reproduced locally:
../../third_party/skia/src/core/SkFixedAlloc.h:39:20: runtime error: control
flow integrity check for type '(anonymous
namespace)::SrcFPPixel<SkAlphaType::kUnpremul_SkAlphaType>' failed
durin│···················
g cast to unrelated type (vtable address 0x000000000003)                        
                                                                                
                             │···················
0x000000000003: note: invalid vtable                                            
                                                                                
                             │···················
<memory cannot be printed>             

There it does an invalid case of uninitialized memory:

// Make space for T.
auto ptr = (T*)(fBuffer+fUsed);
fUsed += sizeof(T);

This is dangerous and correctly triggers a Control Flow Integrity check. I will
try to make a change that preserves the logic but avoids such invalid casts.

[krasin1, 2016-11-19 02:30:14]

The tentative fix is https://skia-review.googlesource.com/c/5074/

[krasin1, 2016-11-19 03:34:18]

On 2016/11/19 02:30:14, krasin1 wrote:
> The tentative fix is https://skia-review.googlesource.com/c/5074/

Local testing confirmed that the fix is correct.