There is some garbage on EMF while converting PDF to EMF

RESOLVED FIXED in Firefox 56

Status

()

Core
Printing: Output
P3
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: fatseng, Assigned: fatseng)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

a year ago
Blocks: 1347444
(Assignee)

Comment 1

a year ago
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.
(Assignee)

Comment 2

a year ago
Created attachment 8871552 [details]
Garbage on upper left corner

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

Updated

a year ago
Assignee: nobody → fatseng
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

a year ago
Created attachment 8872858 [details] [diff] [review]
[WIP] Avoid to use GDIPlusExt in PDFium

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

Updated

11 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

10 months ago
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)
(Assignee)

Comment 7

10 months ago
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 8

10 months ago
mozreview-review
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

10 months ago
mozreview-review
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

10 months ago
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 11

10 months ago
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8872858 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Priority: -- → P3
(Assignee)

Comment 14

10 months ago
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)
(Assignee)

Updated

10 months ago
Attachment #8886530 - Flags: review?(jwatt)

Comment 15

10 months ago
mozreview-review
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 16

10 months ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

10 months ago
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

Comment 23

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ac08e5a6a6cf
https://hg.mozilla.org/mozilla-central/rev/6abd15012dcd
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.