New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 809763 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-02-13
OS: Mac
Pri: 1
Type: Compat

Blocked on:
issue skia:7630



Sign in to add a comment

Glyphs are not rendered correctly in print preview / PDF

Reported by s...@benchling.com, Feb 6 2018

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.140 Safari/537.36

Example URL:
https://benchling.com/saif/f/pKCJaDmR-chrome-errors-public/etr-6x7x1dkd-does-this-title-get-mangled/view?versionId=69605929&note=true&protocols=#print

Steps to reproduce the problem:
1. User visited above mentioned URL, which rendered correctly.
2. Clicked "print" on the modal that automatically opened.
3. Observed that the print preview had incorrect glyphs everywhere (see preview-actual.png)
4. Save as PDF resulted in the same issue (see Does This Title Get Mangled_ · Benchling-actual.pdf)

What is the expected behavior?
- Preview should have shown correctly (see preview-expected.png)
- PDF should have rendered correctly (see Does This Title Get Mangled_ · Benchling-expected.pdf)

What went wrong?
The glyphs on the HTML page are mangled in the print preview and the exported PDF.

Does it occur on multiple sites: No

Is it a problem with a plugin? N/A 

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 64.0.3282.140  Channel: stable
OS Version: OS X 10.12.6
Flash Version: 

I am a developer at Benchling, and I'm reporting this issue on behalf of a user. 

Also, another user with the same version of OSX and Chrome did not experience this issue.
 
preview-actual.png
410 KB View Download
Does This Title Get Mangled_ · Benchling-actual.pdf
252 KB Download
preview-expected.png
193 KB View Download
Does This Title Get Mangled_ · Benchling-expected.pdf
285 KB Download

Comment 1 by ajha@chromium.org, Feb 7 2018

Components: UI>Browser>PrintPreview
Labels: Needs-Triage-M64

Comment 2 by s...@benchling.com, Feb 7 2018

Update: the root cause is from enabling #enable-site-per-process from chrome://flags 

Comment 3 Deleted

Labels: Triaged-ET Needs-Feedback
Unable to reproduce the issue on mac 10.12.6 using chrome reported version #64.0.3282.140 and latest canary #66.0.3341.0.

Attached a screen shots for reference.

Following are the steps followed to reproduce the issue.
------------
1. Navigated to https://benchling.com/saif/f/pKCJaDmR-chrome-errors-public/etr-6x7x1dkd-does-this-title-get-mangled/view?versionId=69605929&note=true&protocols=#print
2. Clicked "print" on the modal that automatically opened.
3. Observed that preview is shown correctly as preview-expected.png and pdf also rendered correctly.

saif@ - Could you please check the issue on latest canary #66.0.3341.0 by creating a new profile without any apps and extensions and please let us know if the issue still persist or not.

Thanks...!!
809763.png
505 KB View Download
chrome@M64stable.png
295 KB View Download

Comment 5 by s...@benchling.com, Feb 7 2018

krajshree@ - see #3
chrome-flags.png
198 KB View Download
chrome@M66-site-per-process.png
276 KB View Download
repro.png
188 KB View Download
Project Member

Comment 6 by sheriffbot@chromium.org, Feb 7 2018

Cc: krajshree@chromium.org
Labels: -Needs-Feedback
Thank you for providing more feedback. Adding requester "krajshree@chromium.org" to the cc list and removing "Needs-Feedback" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Untriaged (was: Unconfirmed)
I see this on Mac Canary with Site Isolation enabled.
Cc: weili@chromium.org
This doesn't happen for me on Linux. Though in my test build, in PrintPreviewMessageHandler::OnDidPreviewPage(), I see it's using the print compositor because Site Isolation is turned on.

Comment 9 by weili@chromium.org, Feb 8 2018

Owner: weili@chromium.org
Status: Assigned (was: Untriaged)

Comment 10 by weili@chromium.org, Feb 10 2018

Cc: -weili@chromium.org
Status: Started (was: Assigned)
It is a regression due to moving compositor to mac v2 sandbox. The fix is pending https://chromium-review.googlesource.com/c/chromium/src/+/912316
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a3533592984bdb788d1335a223b37e125a07c99c

commit a3533592984bdb788d1335a223b37e125a07c99c
Author: Wei Li <weili@chromium.org>
Date: Mon Feb 12 19:16:28 2018

Set up mac v2 sandbox policy for pdf compositor

Pdf compositor service composites contents printed from different
renderer process into a complete pdf document. This is needed for
general printing when site isolation is enabled, because a web page with
out of process iframes may be rendered in multiple renderers.

Therefore, pdf compositor service needs to access font access.
This CL set up pdf compositor service's own seatbelt policy.

BUG= 809763 ,  455764 

Change-Id: I4a78ef793c1ea5e52b167a74fd02091f197bd61f
Reviewed-on: https://chromium-review.googlesource.com/912316
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Wei Li <weili@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536153}
[modify] https://crrev.com/a3533592984bdb788d1335a223b37e125a07c99c/content/browser/child_process_launcher_helper_mac.cc
[modify] https://crrev.com/a3533592984bdb788d1335a223b37e125a07c99c/services/service_manager/sandbox/mac/BUILD.gn
[add] https://crrev.com/a3533592984bdb788d1335a223b37e125a07c99c/services/service_manager/sandbox/mac/pdf_compositor.sb

Comment 12 by weili@chromium.org, Feb 12 2018

Labels: -Needs-Triage-M64 Merge-Request-65
Status: Fixed (was: Started)
This is regressed in M64, can this wait until M66? If it is critical for M65, will it be a safe merge to M65? 

Please note M65 is already promoted to Beta so merge bar is very high. Thank you.

Comment 14 by weili@chromium.org, Feb 13 2018

Cc: kerrnel@chromium.org
It should be safe to merge. But since the impacted feature is behind a flag, I think it could wait too.
The feature in question is the macOS V2 sandbox. It's going to 100% of the stable population in M-64 for the renderer process only. M-65 gets the rest of the process types, such as the PDF compositor, so if this CL doesn't get merged, we'll need to merge a different fix to disable the V2 sandbox for PDF compositor in M65.

But if we don't merge anything we're going to break PDF compositor when m-65 hits stable.
NextAction: 2018-02-13
Ok, pls update the bug with canary result tomorrow. If it looks good in canary, I will approve the merge to M65. Thank you.
Labels: Needs-Feedback
Tested the issue on mac 10.12.6 using latest chrome version #66.0.3346.0 as per comment #0 and #2.
Observed that glyphs on the HTML page are mangled in the print preview and the exported PDF also. Hence, the issue is still being observed.
Attached a screen cast and exported pdf for reference.

weili@ - Could you please check the attached screen cast and please help us in confirming the fix.

Thanks...!!

809763.mp4
1.9 MB View Download
Does This Title Get Mangled_ · Benchlingtest.pdf
274 KB Download
The NextAction date has arrived: 2018-02-13

Comment 19 by weili@chromium.org, Feb 13 2018

Cc: rsesek@chromium.org
Looks like the fix didn't solve all font problems. I am trying to see what else needs some fix.

Greg/Robert, did we change font loading or access in M64 which could be the corpus? 
I'm looking into this now, but if you run launch with the flag --disable-features=MacV2Sandbox, this problem should go away if its sandbox related.

Comment 21 by weili@chromium.org, Feb 13 2018

yes, I think the mangled text in M64 is not due to sandbox change. Maybe in M64 mac font loading changed, but pdf compositor was not aware of that. I will dig deeper. 
Ok, does that mean you ran with --disable-features=MacV2Sandbox and it has no effect? Please run with --no-sandbox as well. I'm building Chrome now so I can test all this.
I can't reproduce this so I think there's more going on here than just a sandbox profile change. What macOS version are you on?

I have: 

$ sw_vers 
ProductName:	Mac OS X
ProductVersion:	10.13.3
BuildVersion:	17D47
I also tried in the latest canary with the Macos V2 sandbox specifically enabled and can't repro.
Project Member

Comment 25 by sheriffbot@chromium.org, Feb 13 2018

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: M65 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Also sorry for all the comments but I also noticed the original report was for Chrome M-64. 64 only used the V2 sandbox for renderers so there must be something else wrong.

Now, it was still a mistake to apply the utility profile to the PDF compositor, so I'm glad we submitted a CL to allow font access and we should keep that, but it seems unlikely as the root cause of this.

From the M-64 code:

  if (base::FeatureList::IsEnabled(features::kMacV2Sandbox) &&
      GetProcessType() == switches::kRendererProcess && !no_sandbox) {
One potential difference between a renderer and pdf_compositor is that a renderer can load fonts remotely (https://cs.chromium.org/chromium/src/content/common/mac/font_loader.h). But that should only be used if the requested font is outside of the standard font locations.

Comment 28 by weili@chromium.org, Feb 13 2018

Status: Started (was: Fixed)
Right, I think I missed mac font loading case. Working on the fix now.

Comment 29 by weili@chromium.org, Feb 15 2018

We need more to fix the mangled text for OP's problem, I filed https://bugs.chromium.org/p/skia/issues/detail?id=7630

OK, what is the plan to merge things to M-65? Do we know if the sandbox profile is related to this at all? If so we should merge both fixes.

Comment 31 by weili@chromium.org, Feb 15 2018

If still possible, I would suggest merging the sandbox change since it is low risk and has broader impact: all users of pdf compsositor service on Mac. Right now pdf compositor service is still behind site isolation flag, so it is not a release blocker.
Cc: gov...@chromium.org
Ok I agree. Can we please merge the sandbox CL in this bug? 
Is this  https://chromium.googlesource.com/chromium/src.git/+/a3533592984bdb788d1335a223b37e125a07c99c (66.0.3346.0) you're requesting M65 merge?
Correct.
Thank you  kerrnel@. 
Since this is already regressed in M64, is it critical to merge this fix in to M65 (Just double checking as we're very close to M65 stable promotion so merge bar is high)?
Thank you  kerrnel@. 
Since this is already regressed in M64, is it critical to merge this fix in to M65 (Just double checking as we're very close to M65 stable promotion so merge bar is high)?
The sandbox change is not regressed in M-64. It wasn't present in M-64. We should merge this to M-65, yes.
Labels: -Merge-Review-65 Merge-Approved-65
Approving merge to M65 branch 3325 for https://chromium.googlesource.com/chromium/src.git/+/a3533592984bdb788d1335a223b37e125a07c99c  based on comments #31, #32 and #37. Please merge ASAP. Thank you.
Project Member

Comment 39 by bugdroid1@chromium.org, Feb 15 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d608af8f049339534ac68adaca1d4253068b486f

commit d608af8f049339534ac68adaca1d4253068b486f
Author: Wei Li <weili@chromium.org>
Date: Thu Feb 15 21:27:49 2018

Set up mac v2 sandbox policy for pdf compositor

Pdf compositor service composites contents printed from different
renderer process into a complete pdf document. This is needed for
general printing when site isolation is enabled, because a web page with
out of process iframes may be rendered in multiple renderers.

Therefore, pdf compositor service needs to access font access.
This CL set up pdf compositor service's own seatbelt policy.

BUG= 809763 ,  455764 

Change-Id: I4a78ef793c1ea5e52b167a74fd02091f197bd61f
Reviewed-on: https://chromium-review.googlesource.com/912316
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Wei Li <weili@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#536153}(cherry picked from commit a3533592984bdb788d1335a223b37e125a07c99c)
Reviewed-on: https://chromium-review.googlesource.com/922881
Reviewed-by: Wei Li <weili@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#480}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/d608af8f049339534ac68adaca1d4253068b486f/content/browser/child_process_launcher_helper_mac.cc
[modify] https://crrev.com/d608af8f049339534ac68adaca1d4253068b486f/services/service_manager/sandbox/mac/BUILD.gn
[add] https://crrev.com/d608af8f049339534ac68adaca1d4253068b486f/services/service_manager/sandbox/mac/pdf_compositor.sb

Tested the issue on mac 10.12.6 using chrome version #65.0.3325.88 as per comment #0, #2 and #39.
Steps followed to verify issue:
===============================
1. Opened chrome browser
2. Enabled #enable-site-per-process and #mac-v2-sandbox from chrome://flags.
3. Navigated to https://benchling.com/saif/f/pKCJaDmR-chrome-errors-public/etr-6x7x1dkd-does-this-title-get-mangled/view?versionId=69605929&note=true&protocols=#print and clicked on "print" on the modal that automatically opened.
4. Observed that glyphs on the HTML page are mangled in the print preview and the exported PDF also. Hence, the issue is still being observed.

Attached a screen cast and exported pdf for reference.

weili@ - Could you please check the attached screen cast and please confirm the expected behaviour and help us in confirming the fix.

Thanks...!!
809763@M65.mp4
1.2 MB View Download
Does This Title Get Mangled_ · Benchling@M65.pdf
290 KB Download

Comment 41 by weili@chromium.org, Feb 21 2018

Hi, krajshree@, thanks for testing. There are two issues discovered by this bug. If you test a M65 version before #65.0.3325.78, with #enable-site-per-process and #mac-v2-sandbox, print some regular web sites, not the benchling.com, you will see all text are missing. After #65.0.3325.78, you should see normal print out. This should be able to be confirmed.

The mangled text as seen by OP and #40 is still in progress. If I didn't mark this fixed, consider we are still working on it. thanks

Comment 42 by creis@chromium.org, Feb 21 2018

Components: Internals>Sandbox>SiteIsolation
Labels: -Pri-2 M-66 Pri-1
weili@: Can you post an update about the status of this and https://bugs.chromium.org/p/skia/issues/detail?id=7630?  I'm trying to determine if this is a blocker for Site Isolation in M66, and whether the fix is coming soon or can be merged to M66.

Is the bug definitely Mac specific, or could it apply to other platforms?  Do we have a sense for how common it will be?  IIUC, it applies to main frames as well as OOPIFs, but only when the new printing compositing logic is enabled (i.e., Site Isolation), correct?

Thanks!
Labels: Proj-SiteIsolation-LaunchBlocking
I have not got any update on https://bugs.chromium.org/p/skia/issues/detail?id=7630

To evaluate the impact of this bug, I have tried a few dozen web sites using web fonts across Mac/Linux/Windows, and none of them have problem. Even for this bug, it only manifests on Mac. So I would say it is definitely a tricky edge case we need more time to investigate. That said, I feel this might not be a launch blocker though if this only happens on a particular site.

btw, I have another CL https://chromium-review.googlesource.com/c/chromium/src/+/919968 under review, I plan to land and merge once approved. This might be needed for fixing this bug.

More update: after diving into some technical details, I found that skia does pretty decent job on webfont serialization. Even for OP's website, some fonts were done correctly. For that website, the errors happen during reconstruction of certain Mac fonts. But the cause of failing is still under investigation. 
For anti-copying reasons, a number of the webfonts on this page are split into two, an "A" and a "B" variety. These are always listed together in the css, "A" first then "B". The reason for this is that half of the font is in one of these files, and the rest of the font is in the other (each is a complimentary subset of the original font). While looking into this on Mac Canary 67.0.3364.0 it appears that the glyphs from the "B" font are all wrong. Instead of drawing from the "B" font the glyphs look like they're coming from a completely different font (maybe a fallback font, like Helvetica?). Since this is only happening in this specific situation, it may have something to do with all of the metadata internal to these fonts being the same.
Pretty sure the "B" font isn't getting used and Helvetica is being used as the fallback instead, since I'm seeing 's' replaces with '3' and these are both glyph id 22 in the "B" font and Helvetica. This means the "B" font didn't make it all the way through this process somehow.
Thank you, bungeman@, for looking into this and sharing the update! I noticed that "B" font is a type1 font. And it failed in reconstruction.
Following the directions at https://skia.org/user/tips#mskp-capture I managed to capture a recording of what was going to be used to draw the PDF. This shows the same issue. If I dig into the first DrawTextBlob which has issues, and download the data behind the typefaces I see that the "A" font got serialized fine, but the "B" font was serialized as the system font Helvetica (no data was serialized). It seems something happened while recording.
So after looking really hard it appears that this is a bug in round tripping the font data. If one takes the mskp which reproduces this the "B" font data can be found inside it. However, instead of starting with 'OTTO' it starts with 'typ1'. If one does a search and replace of all these 'type1' and replaces them with 'OTTO' then everything starts working fine.

The underlying complexity here is that CoreGraphics never gives back the original font data, it can only provide the tables. As a result, Skia has to reconstruct the headers. In this case, CoreGraphics guessed (incorrectly) that this was a 'typ1' font so that's what Skia wrote out. These bytes shouldn't matter that much, but apparently this is enough for CoreGraphics to reject this font data when we go to re-create it.
That being said, I'll fix this on the Skia side asap.
Project Member

Comment 53 by bugdroid1@chromium.org, Mar 9 2018

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/03cd6e6dec894b3447527f84e7591d62981df433

commit 03cd6e6dec894b3447527f84e7591d62981df433
Author: Ben Wagner <bungeman@google.com>
Date: Fri Mar 09 15:03:20 2018

Avoid serializing to 'typ1' on Mac.

CoreGraphics does not provide a means to get the original font data for a
CGFont, only the tables. As a result, Skia pieces the font data back
together when requested. The most awkward part of this is choosing the
first four bytes, and the CTFont suggestion seems to often be wrong.

This change doublechecks the selection of 'typ1', prefering to use 'OTTO'
if there are no 'TYP1' or 'CID ' tables. These sorts of fonts are
extremely old and unlikely to be in current use. It appears that CTFont
may report that it has this format if it is an 'OTTO' font with very few
glyphs. If Skia serializes such a font with 'typ1' as the first four
bytes, CoreGraphics will not create a CGFont from the resulting font data.

BUG= chromium:809763 , skia:7630 

Change-Id: I9979b9f0ebdd27c4ad0903e8ee6237241e755541
Reviewed-on: https://skia-review.googlesource.com/113306
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Ben Wagner <bungeman@google.com>

[modify] https://crrev.com/03cd6e6dec894b3447527f84e7591d62981df433/tests/TypefaceTest.cpp
[modify] https://crrev.com/03cd6e6dec894b3447527f84e7591d62981df433/src/ports/SkFontHost_mac.cpp
[add] https://crrev.com/03cd6e6dec894b3447527f84e7591d62981df433/resources/fonts/7630.otf

Project Member

Comment 54 by bugdroid1@chromium.org, Mar 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98a3bb300b766e5ff5eb5f06eec36e5d7b65c690

commit 98a3bb300b766e5ff5eb5f06eec36e5d7b65c690
Author: skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Mar 09 21:40:01 2018

Roll src/third_party/skia/ b537879c7..43f0ba073 (19 commits)

https://skia.googlesource.com/skia.git/+log/b537879c7214..43f0ba073068

$ git log b537879c7..43f0ba073 --date=short --no-merges --format='%ad %ae %s'
2018-03-09 reed update tool to sniff images during deserialization
2018-03-09 egdaniel Fix no gpu build
2018-03-09 herb Remove getCache from SkAutoGlyphCache*
2018-03-09 bsalomon Fix breakage from merge conflict related to deleteTestingOnlyBackendTexture sig. change
2018-03-09 robertphillips Enable opList-based DDLs
2018-03-09 bsalomon Make GrGpu::deleteTestingOnlyBackendTexture() take const GrBackendTexture&
2018-03-09 brianosman Fix a logical test in GrColorSpaceXform::Equals
2018-03-09 egdaniel Add promise images for deferred instantiation of wrapped gpu textures
2018-03-09 herb Remove one use of AttachCache, and a paint variant of FindOrCreate...
2018-03-09 robertphillips Make GrTextureStripAtlas DDL friendly
2018-03-09 bsalomon Vulkan backend render targets don't allow client to specify stencil bits
2018-03-09 angle-skia-autoroll Roll skia/third_party/externals/angle2/ e8a93c6ed..331404098 (1 commit)
2018-03-09 update-skps Update SKP version
2018-03-08 csmartdalton ccpr: Fix stale array access after dynamic resize
2018-03-09 bsalomon Add GM configs that test rendering to a GL backend texture and render target
2018-03-08 csmartdalton ccpr: Fix crash on zero-length fan tessellations
2018-03-09 herb Remove all uses of getCache with get.
2018-03-08 bungeman Avoid serializing to 'typ1' on Mac.
2018-03-09 caryclark some fuzzer fixes

Created with:
  roll-dep src/third_party/skia
BUG= 809763 


The AutoRoll server is located here: https://autoroll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
TBR=fmalita@chromium.org

Change-Id: If8fabf2721caab7a00df51dc9698da43867a7e7e
Reviewed-on: https://chromium-review.googlesource.com/956755
Commit-Queue: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#542233}
[modify] https://crrev.com/98a3bb300b766e5ff5eb5f06eec36e5d7b65c690/DEPS

Project Member

Comment 55 by bugdroid1@chromium.org, Mar 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/feb3b5d8fc0436f4ae83736f450544142e9a21f1

commit feb3b5d8fc0436f4ae83736f450544142e9a21f1
Author: Wei Li <weili@chromium.org>
Date: Sat Mar 10 06:28:54 2018

Allow mojo services to have blink web sandbox support

Most mojo services are simple utility processes that don't require
blink web sandbox support. However, services such as pdf compositor
would need such support to access and load fonts. This CL does the
following two major tasks:
-- Add SandboxSupportedUtilityBlinkPlatformImpl and implements it
to allow utility process to have the sandbox support when needed;
-- Move LoadFont() interprocess call on MacOS from renderer to
content/common, so it can be used by both renderer and utility
processes.

BUG= 809763 , 813034 

Change-Id: I4f8b50cc5cc31ac68dfbc2ba5b1195fb4e0748cc
Reviewed-on: https://chromium-review.googlesource.com/919968
Commit-Queue: Wei Li <weili@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542361}
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/chrome/browser/printing/print_browsertest.cc
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/components/printing/service/BUILD.gn
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/components/printing/service/DEPS
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/components/printing/service/pdf_compositor_manifest.json
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/components/printing/service/pdf_compositor_service.cc
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/components/printing/service/pdf_compositor_service_unittest.cc
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/components/printing/service/public/cpp/pdf_compositor_service_factory.cc
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/browser/renderer_host/render_message_filter.cc
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/browser/renderer_host/render_message_filter.h
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/browser/service_manager/common_browser_interfaces.cc
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/child/BUILD.gn
[add] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/child/child_process_sandbox_support_impl_mac.cc
[add] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/child/child_process_sandbox_support_impl_mac.h
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/child/child_thread_impl.cc
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/child/child_thread_impl.h
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/common/BUILD.gn
[add] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/common/font_loader_dispatcher_mac.cc
[add] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/common/font_loader_dispatcher_mac.h
[add] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/common/font_loader_mac.mojom
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/common/render_message_filter.mojom
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/ppapi_plugin/ppapi_blink_platform_impl.cc
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/public/app/mojo/content_browser_manifest.json
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/public/app/mojo/content_renderer_manifest.json
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/public/child/child_thread.h
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/public/test/mock_render_thread.cc
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/public/test/mock_render_thread.h
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/public/utility/utility_thread.h
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/renderer/renderer_blink_platform_impl.cc
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/utility/BUILD.gn
[add] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/utility/utility_blink_platform_with_sandbox_support_impl.cc
[add] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/utility/utility_blink_platform_with_sandbox_support_impl.h
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/utility/utility_thread_impl.cc
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/content/utility/utility_thread_impl.h
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/third_party/WebKit/public/platform/linux/WebSandboxSupport.h
[modify] https://crrev.com/feb3b5d8fc0436f4ae83736f450544142e9a21f1/third_party/WebKit/public/platform/mac/WebSandboxSupport.h

Comment 56 by weili@chromium.org, Mar 11 2018

Labels: -Needs-Feedback Merge-Request-66
Verified on today's Canary 67.0.3368.0 on Mac. OP's website now can be printed correctly. Requesting a merge to M66 now.
Project Member

Comment 57 by sheriffbot@chromium.org, Mar 11 2018

Labels: -Merge-Request-66 Merge-Review-66
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Blockedon: skia:7630
The Skia part of this merge request is to cherry-pick https://skia.googlesource.com/skia/+/03cd6e6dec894b3447527f84e7591d62981df433 onto the https://skia.googlesource.com/skia/+/chrome/m66 branch (as seen at https://skia-review.googlesource.com/c/skia/+/113706 ). This is a fairly small change which adds a test for the issue described in comment 51.

Comment 60 by creis@chromium.org, Mar 12 2018

Cc: abdulsyed@chromium.org cma...@chromium.org
+ more milestone owners for merge questions.

weili@: Do you need to merge both the Skia change (as noted in comment 59) and r542361 from comment 55, or just the Skia change?

Comment 61 by creis@chromium.org, Mar 12 2018

Note: It sounds like the M66 beta cut will likely be EOD Tuesday, so we should aim for that if the merge is safe and approved.  Thanks!

Comment 62 by weili@chromium.org, Mar 12 2018

I would like to merge both changes if possible. Both already have tests. Skia change is small and Mac only; the other change is a bit large, but only affect Mac and Linux, and we are not changing the existing behaviors but just add some capabilities for our own service process only. Thanks!
Labels: -Merge-Review-66 Merge-Approved-66
Approving this for M66. Branch:3359. 
Project Member

Comment 64 by bugdroid1@chromium.org, Mar 13 2018

Labels: merge-merged-m66
The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/b89d244a2f5cd5d06172a377643edd31526a752a

commit b89d244a2f5cd5d06172a377643edd31526a752a
Author: Ben Wagner <bungeman@google.com>
Date: Tue Mar 13 18:53:51 2018

Avoid serializing to 'typ1' on Mac.

CoreGraphics does not provide a means to get the original font data for a
CGFont, only the tables. As a result, Skia pieces the font data back
together when requested. The most awkward part of this is choosing the
first four bytes, and the CTFont suggestion seems to often be wrong.

This change doublechecks the selection of 'typ1', prefering to use 'OTTO'
if there are no 'TYP1' or 'CID ' tables. These sorts of fonts are
extremely old and unlikely to be in current use. It appears that CTFont
may report that it has this format if it is an 'OTTO' font with very few
glyphs. If Skia serializes such a font with 'typ1' as the first four
bytes, CoreGraphics will not create a CGFont from the resulting font data.

BUG= chromium:809763 , skia:7630 

Reviewed-on: https://skia-review.googlesource.com/113306
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Ben Wagner <bungeman@google.com>

Change-Id: Iaaa341d2416ba7541b771151883b02284f191021
Cherry-Pick: 03cd6e6dec894b3447527f84e7591d62981df433
Approval: https://bugs.chromium.org/p/chromium/issues/detail?id=809763#c63
Reviewed-on: https://skia-review.googlesource.com/113706
Reviewed-by: Ben Wagner <bungeman@google.com>

[modify] https://crrev.com/b89d244a2f5cd5d06172a377643edd31526a752a/tests/TypefaceTest.cpp
[modify] https://crrev.com/b89d244a2f5cd5d06172a377643edd31526a752a/src/ports/SkFontHost_mac.cpp
[add] https://crrev.com/b89d244a2f5cd5d06172a377643edd31526a752a/resources/fonts/7630.otf

Comment 65 by weili@chromium.org, Mar 13 2018

Status: Fixed (was: Started)
Thanks, Ben, for help fixing this!
Project Member

Comment 66 by sheriffbot@chromium.org, Mar 19 2018

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Approved-66
We've seen this issue re-emerge on OS X 10.11.6 and 10.12.6 running Chrome 66.0.3359.181. Disabling chrome://flags/#use-pdf-compositor-service-for-print appears to fix it. MacOS 10.13.4 doesn't experience this bug even with the flag flipped to enable.

The original test URL https://benchling.com/saif/f/pKCJaDmR-chrome-errors-public/etr-6x7x1dkd-does-this-title-get-mangled/view?versionId=69605929&note=true&protocols=#print still works.

Comment 69 by creis@chromium.org, May 23 2018

Comment 68: Thanks for the report.  weili@ and bungeman@, can you take a look to see if this is reproducible?  If so, we'll probably want to file a new bug and get it fixed quickly if possible-- the new print compositing service will roll out with Site Isolation, which is aiming for M67 on Tuesday.

Comment 70 by creis@chromium.org, May 23 2018

Comment 68: In the meantime, could you check to see if the bug occurs on Mac Beta (67.0.3396.56) or Mac Canary (68.0.3438.0+), when chrome://flags/#use-pdf-compositor-service-for-print is enabled?  If it's fixed in M67, then that will be rolling out soon.  Thanks!
I imagine you want the additional (and more extensive) fix in https://skia.googlesource.com/skia/+/93e4ea52f66320685d3bcd1d613d6cf36de5e080 . This change is in m67 but is not in m66.
On MacOS 10.12.6 with the Mac beta, we checked in chrome://settings/help we were on 67 and enabled the PDF compositor flag and the print previews looked fine.

Comment 73 by creis@chromium.org, May 24 2018

Great!  Thanks for confirming.  M67 is scheduled to roll out on Mac on this coming Tuesday, May 29, so we'll consider this resolved.

Sign in to add a comment