Closed Bug 1367948 Opened 7 years ago Closed 7 years ago

There is some garbage on EMF while converting PDF to EMF

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: fatseng, Assigned: fatseng)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

      No description provided.
Blocks: pdf-printing
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.
The garbage on upper left corner. I marked them by red rectangles.
Assignee: nobody → fatseng
Status: NEW → ASSIGNED
I found that Chromium doesn't use GDIPlusExt in PDFium while sandbox is enabled.
Summary: [Mortar] [pdfium] There is some garbage on EMF while converting PDF to EMF → There is some garbage on EMF while converting PDF to EMF
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.
Attachment #8886529 - Flags: feedback?(brsun)
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.
Attachment #8886530 - Flags: feedback?(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 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 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.
Attachment #8886529 - Flags: feedback?(brsun) → feedback+
Comment on attachment 8886530 [details]
Bug 1367948 - Part2. Prevent PDFium from using GDIPlusExt to avoid garbage rendering.

f+ if comments would be addressed.
Attachment #8886530 - Flags: feedback?(brsun) → feedback+
Attachment #8872858 - Attachment is obsolete: true
Priority: -- → P3
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.
Attachment #8886529 - Flags: review?(jwatt)
Attachment #8886530 - Flags: review?(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 <fatseng@mozilla.com>
> +# 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)."
Attachment #8886529 - Flags: review?(jwatt) → review+
Comment on attachment 8886530 [details]
Bug 1367948 - Part2. Prevent PDFium from using GDIPlusExt to avoid garbage rendering.

https://reviewboard.mozilla.org/r/157342/#review165670
Attachment #8886530 - Flags: review?(jwatt) → review+
Pushed by fatseng@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/ac08e5a6a6cf
https://hg.mozilla.org/mozilla-central/rev/6abd15012dcd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.