1367948 - There is some garbage on EMF while converting PDF to EMF

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P3)

Comment 1

[Farmer Tseng[:fatseng](inactive)]

While printing the form 1040(https://www.irs.gov/pub/irs-pdf/f1040.pdf), PDFium generates some garbage in upper left corner. I marked them by red rectangles. Please see attachment.
When I launch Chrome by the no-sandbox flag, it also outputs the same result.

Comment 2

[Farmer Tseng[:fatseng](inactive)]

Attachment: Garbage on upper left corner

The garbage on upper left corner. I marked them by red rectangles.

Comment 3

[Farmer Tseng[:fatseng](inactive)]

Attachment: [WIP] Avoid to use GDIPlusExt in PDFium

I found that Chromium doesn't use GDIPlusExt in PDFium while sandbox is enabled.

Comment 4

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1367948 - Part1. Add a patch file to patch PDFium to prevent it from using GDIPlusExt to avoid garbage rendering.

Review commit: https://reviewboard.mozilla.org/r/157340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157340/

Comment 5

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1367948 - Part2. Prevent PDFium from using GDIPlusExt to avoid garbage rendering.

Review commit: https://reviewboard.mozilla.org/r/157342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157342/

Comment 6

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8886529 [details]
Bug 1367948 - Part1. Add a patch file to patch PDFium to prevent it from using GDIPlusExt to avoid garbage rendering.

Create a patch file to avoid using GDIPlusExt while rendering EMF.
Add the patch to update.py to modify the pdfium source automatically.

Comment 7

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8886530 [details]
Bug 1367948 - Part2. Prevent PDFium from using GDIPlusExt to avoid garbage rendering.

We force to |return false| here to avoid using GDIPlusExt.

Comment 8

[Bruce Sun [:brsun]]

Comment on attachment 8886529 [details]
Bug 1367948 - Part1. Add a patch file to patch PDFium to prevent it from using GDIPlusExt to avoid garbage rendering.

https://reviewboard.mozilla.org/r/157340/#review162494

::: modules/pdfium/patches/bug1367948_avoid_using_GDIPlusExt.patch:24
(Diff revision 1)
> +   return false;
> + }
> + 
> + bool IsGDIEnabled() {
> ++  // In Chromium, PDFium runs in sandboxed process which can't get the screen
> ++  // DC. Therefore, we force to |return false| here to avoid using GDIPlusExt

GDIPlusExt is not a common term. Suggest to rephrase this term with your intention, depending on whether you want to refer to the CGdiplusExt class within PDfium, or you want to refer to some specific GDI+ and GDI32 functions of Windows.

::: modules/pdfium/patches/bug1367948_avoid_using_GDIPlusExt.patch:25
(Diff revision 1)
> + }
> + 
> + bool IsGDIEnabled() {
> ++  // In Chromium, PDFium runs in sandboxed process which can't get the screen
> ++  // DC. Therefore, we force to |return false| here to avoid using GDIPlusExt
> ++  // in PDFium and make sure that we go the same way as Chromium.

Please also help to add comments about what would go wrong if we don't do this tricky stuff.

Comment 9

[Bruce Sun [:brsun]]

Comment on attachment 8886530 [details]
Bug 1367948 - Part2. Prevent PDFium from using GDIPlusExt to avoid garbage rendering.

https://reviewboard.mozilla.org/r/157342/#review162496

::: modules/pdfium/pdfium/core/fxge/win32/fx_win32_device.cpp:84
(Diff revision 1)
>    return false;
>  }
>  
>  bool IsGDIEnabled() {
> +  // In Chromium, PDFium runs in sandboxed process which can't get the screen
> +  // DC. Therefore, we force to |return false| here to avoid using GDIPlusExt

ditto

::: modules/pdfium/pdfium/core/fxge/win32/fx_win32_device.cpp:85
(Diff revision 1)
>  }
>  
>  bool IsGDIEnabled() {
> +  // In Chromium, PDFium runs in sandboxed process which can't get the screen
> +  // DC. Therefore, we force to |return false| here to avoid using GDIPlusExt
> +  // in PDFium and make sure that we go the same way as Chromium.

ditto

Comment 10

[Bruce Sun [:brsun]]

Comment on attachment 8886529 [details]
Bug 1367948 - Part1. Add a patch file to patch PDFium to prevent it from using GDIPlusExt to avoid garbage rendering.

f+ if comments would be addressed.

Comment 11

[Bruce Sun [:brsun]]

Comment on attachment 8886530 [details]
Bug 1367948 - Part2. Prevent PDFium from using GDIPlusExt to avoid garbage rendering.

f+ if comments would be addressed.

Comment 12

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8886529 [details]
Bug 1367948 - Part1. Add a patch file to patch PDFium to prevent it from using GDIPlusExt to avoid garbage rendering.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157340/diff/1-2/

Comment 13

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8886530 [details]
Bug 1367948 - Part2. Prevent PDFium from using GDIPlusExt to avoid garbage rendering.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157342/diff/1-2/

Comment 14

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8886529 [details]
Bug 1367948 - Part1. Add a patch file to patch PDFium to prevent it from using GDIPlusExt to avoid garbage rendering.

Using GDIPlusExt class in PDFium would render some garbage on EMF(attachment 8871552 [details]). Since Chromium runs in sandboxed process, it can't get screen DC and always returns |false| from IsGDIEnabled(). I tried to disable sandbox from Chromium. It also printed out some garbage.
I force to return false in IsGDIEnabled() to avoid using GDIPlusExt class within PDFium and make sure that we go the same way as Chromium.

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 8886529 [details]
Bug 1367948 - Part1. Add a patch file to patch PDFium to prevent it from using GDIPlusExt to avoid garbage rendering.

https://reviewboard.mozilla.org/r/157340/#review165668

::: commit-message-201cb:1
(Diff revision 2)
> +Bug 1367948 - Part1. Add a patch file for PDFium

Make this "Add a patch file to patch PDFium to prevent it from using GDIPlusExt to avoid garbage rendering."

::: modules/pdfium/patches/bug1367948_avoid_using_GDIPlusExt.patch:7
(Diff revision 2)
> +# User Farmer Tseng <[e-mail]>
> +# Date 1500268123 -28800
> +#      Mon Jul 17 13:08:43 2017 +0800
> +# Node ID 9f50f06ed643bcffff57dbfd9c176685f2554ccc
> +# Parent  4110bc73506d3e8f5d21479fe79ed91781f0300d
> +Bug 1367948 - Part2. Avoid using GDIPlusExt in PDFium

Make this comment "Prevent PDFium from using GDIPlusExt to avoid garbage rendering."

::: modules/pdfium/patches/bug1367948_avoid_using_GDIPlusExt.patch:24
(Diff revision 2)
> +   return false;
> + }
> + 
> + bool IsGDIEnabled() {
> ++  // In Chromium, PDFium runs in sandboxed process which can't get the screen
> ++  // DC. Therefore, we force to |return false| here to avoid using GDIPlusExt

Remove the words "force to", and add the word "the" before the word "GDIPlusExt".

::: modules/pdfium/patches/bug1367948_avoid_using_GDIPlusExt.patch:25
(Diff revision 2)
> + }
> + 
> + bool IsGDIEnabled() {
> ++  // In Chromium, PDFium runs in sandboxed process which can't get the screen
> ++  // DC. Therefore, we force to |return false| here to avoid using GDIPlusExt
> ++  // class within PDFium and make sure that we go the same way as Chromium.

Change "and make sure that we go the same way as Chromium" to "in order to take the same code paths as Chromium".

::: modules/pdfium/patches/bug1367948_avoid_using_GDIPlusExt.patch:26
(Diff revision 2)
> + 
> + bool IsGDIEnabled() {
> ++  // In Chromium, PDFium runs in sandboxed process which can't get the screen
> ++  // DC. Therefore, we force to |return false| here to avoid using GDIPlusExt
> ++  // class within PDFium and make sure that we go the same way as Chromium.
> ++  // If | return true |, PDFium would render some garbage on EMF, see Bug1367948

Replace this line with "This avoids an issue where PDFium can render some garbage to EMF (see bug 1367948)."

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 8886530 [details]
Bug 1367948 - Part2. Prevent PDFium from using GDIPlusExt to avoid garbage rendering.

https://reviewboard.mozilla.org/r/157342/#review165670

Comment 17

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8886529 [details]
Bug 1367948 - Part1. Add a patch file to patch PDFium to prevent it from using GDIPlusExt to avoid garbage rendering.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157340/diff/2-3/

Comment 18

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8886530 [details]
Bug 1367948 - Part2. Prevent PDFium from using GDIPlusExt to avoid garbage rendering.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157342/diff/2-3/

Comment 19

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ba26e0c813d36de8ba1d792e1cd5270f622b35a

Comment 20

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8886529 [details]
Bug 1367948 - Part1. Add a patch file to patch PDFium to prevent it from using GDIPlusExt to avoid garbage rendering.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157340/diff/3-4/

Comment 21

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8886530 [details]
Bug 1367948 - Part2. Prevent PDFium from using GDIPlusExt to avoid garbage rendering.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157342/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac08e5a6a6cf
Part1. Add a patch file to patch PDFium to prevent it from using GDIPlusExt to avoid garbage rendering. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/6abd15012dcd
Part2. Prevent PDFium from using GDIPlusExt to avoid garbage rendering. r=jwatt

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac08e5a6a6cf
https://hg.mozilla.org/mozilla-central/rev/6abd15012dcd