Create a new sandboxed process to run pdfium

RESOLVED FIXED in Firefox 59

Status

()

Product:
Core
Component:
Printing: Output
Type:
enhancement
Importance:
P1
normal
Status:
RESOLVED FIXED
Opened:
2 years ago
Updated:
2 years ago

People

(Reporter: fatseng, Assigned: u459114)

Tracking

(Blocks 1 bug)

Version:
unspecified
Target:
mozilla59
Platform:
Unspecified
Windows
Points:
---
Dependency tree / graph
Duplicates:
1424000

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(23 attachments, 17 obsolete attachments)

Bug 1399787 - Part 1. Fix namespacing and include issues hidden by unified compilation.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 3. Create a top level protocol for the PDFium process.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 5. Implement the PDFium process.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 8. Open PDFium protocol channel.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 9. Sandbox the PDFium process.
59 bytes, text/x-review-board-request
jwatt
: review+
bobowen
: review+
Details
Bug 1399787 - Part 14. Prevent RemotePrintJobChild using ipc calls after the channel was destroyed.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 2.a. Add functions to load the content of a PDF from a FileDescriptor.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 2.b. Add functions to read/write EMF content from/to a buffer.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 2.c. Rename DrawPageToFile to SavePageToFile.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 4. Take out the code we landed in bug 1370488.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 6. Implement the bulk of a new PrintTargetEMF.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 7. Have PrintTargetEMF launch the PDFium process.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 10. Make PrintTargetEMF use the PDFium process to convert to EMF.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 11.a. Use PrintTargetEMF to print content documents.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 12. Delay dispatching FinalizePrint message until the last page was processed.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 13. Handle AbortDocument.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 15. Detect and handle breakage of the IPC channel.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 16. Hide function table in PDFiumEngineShim.cpp.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 11.b. Use PrintTargetEMF to print chrome documents.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 11.d. Using PrintTargetEMF on windows if skia-pdf is enable.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1399787 - Part 17. Clean up some comments and formatting in PDFViaEMFPrintHelper code.
59 bytes, text/x-review-board-request
jwatt
: review+
Details
patch for ipdl review
78.98 KB, patch
dvander
: review+
Details | Diff | Splinter Review
Bug 1399787 - Part 11.c. Add assertions to make sure no page-done callback from the PrintTarget after the print job done.
59 bytes, text/x-review-board-request
jwatt
: review+
Details 
No description provided.
OS: Unspecified → Windows
Upload a WIP patch to create a sandboxed process.
LGTM. Please land after 9/21.
Assignee: nobody → fatseng
status-firefox57: --- → wontfix
Priority: -- → P3
Attachment #8908137 - Attachment description: [WIP] create a sandboxed process → [WIP] Part1 create a sandboxed process
Posted patch [WIP] part2 setup IPC (obsolete) — — Splinter Review 
Setup an IPC between Chrome and a sandboxed process.
Posted patch [WIP] Part3 print PDF (obsolete) — — Splinter Review 
Convert PDF to EMF in the sandboxed process, after that send the EMF to the Chrome process for printing.
Attachment #8912579 - Attachment description: Part3 print PDF → [WIP]Part3 print PDF
Attachment #8912579 - Attachment description: [WIP]Part3 print PDF → [WIP] Part3 print PDF
Posted patch [WIP] part2 setup IPC (obsolete) — — Splinter Review
Attachment #8910586 - Attachment is obsolete: true
I finished the WIP patches.
They could deal with EMF conversion in sandboxed process.
These patches are based on the following changeset.
I will leave Mozilla on 10/6.
Cj would take over this.

changeset:   377807:04b6be50a252
tag:         qparent
fxtree:      central
parent:      377695:d9b405d82cff
parent:      377806:9182a041d889
user:        Wes Kocher <wkocher@mozilla.com>
date:        Wed Aug 30 19:52:39 2017 -0700
summary:     Merge autoland to central, a=merge
Posted patch merged patch (obsolete) — — Splinter Review
Assignee: fatseng → cku
Posted patch merged patch (obsolete) — — Splinter Review
Attachment #8915526 - Attachment is obsolete: true
Posted file test (obsolete) —
Attachment #8918182 - Attachment is obsolete: true
Attachment #8919762 - Attachment is obsolete: true
Attachment #8919764 - Attachment is obsolete: true
Attachment #8919757 - Flags: review?(jwatt)
Attachment #8919758 - Flags: review?(jwatt)
Attachment #8919759 - Flags: review?(jwatt)
Attachment #8919760 - Flags: review?(jwatt)
Attachment #8919761 - Flags: review?(jwatt)
Attachment #8919763 - Flags: review?(jwatt)
Attachment #8920649 - Flags: review?(jwatt)
Attachment #8920650 - Flags: review?(jwatt)
Attachment #8919765 - Flags: review?(jwatt)
Attachment #8920985 - Flags: review?(jwatt)
Comment on attachment 8919757 [details]
Bug 1399787 - Part 1. Fix namespacing and include issues hidden by unified compilation.

https://reviewboard.mozilla.org/r/190692/#review197438

::: commit-message-a2905:1
(Diff revision 2)
> +Bug 1399787 - Part 1. Fix compile errors caused by new added files.

Call this "Fix namespacing and include issues hidden by unified compilation".
Attachment #8919757 - Flags: review?(jwatt) → review+
Attachment #8921859 - Flags: review?(jwatt)
Comment on attachment 8921859 [details]
Bug 1399787 - Part 10. Report and handle errors while generating EMF.

https://reviewboard.mozilla.org/r/192890/#review198134


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/printing/nsPrintEngine.cpp:677
(Diff revision 2)
>    }
> -
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  rv = devspec->Init(nullptr, printData->mPrintSettings, aIsPrintPreview,
> +                     [this](nsresult aResult) {
> +                       if (NS_FAILED(aResult)) {
> +                         CleanupOnFailure(aResult, true);

Error: Refcounted variable 'this' of type 'nsprintengine' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]

::: layout/printing/nsPrintEngine.cpp:677
(Diff revision 2)
>    }
> -
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  rv = devspec->Init(nullptr, printData->mPrintSettings, aIsPrintPreview,
> +                     [this](nsresult aResult) {
> +                       if (NS_FAILED(aResult)) {
> +                         CleanupOnFailure(aResult, true);

Error: Refcounted variable 'this' of type 'nsprintengine' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]
Comment on attachment 8919758 [details]
Bug 1399787 - Part 3. Create a top level protocol for the PDFium process.

https://reviewboard.mozilla.org/r/190694/#review198124

::: commit-message-a8ad8:3
(Diff revision 3)
> +Bug 1399787 - Part 2. Create a top level protocol for the PDFium process.
> +
> +Define ipdl and actor classes. Implementation of actors are left behind.

s/are left behind/is added in subsequent patches/

::: commit-message-a8ad8:5
(Diff revision 3)
> +Bug 1399787 - Part 2. Create a top level protocol for the PDFium process.
> +
> +Define ipdl and actor classes. Implementation of actors are left behind.
> +
> +Use case scenario

At first glance this looks like you are listing two different scenarios. Maybe s/Use case senario/Control flow/.

::: commit-message-a8ad8:8
(Diff revision 3)
> +Define ipdl and actor classes. Implementation of actors are left behind.
> +
> +Use case scenario
> +1. nsDeviceContextSpecWin, in chrome process, calls *SendStartPrint* to ask the
> +   PDFium process starting to convert a PDF into EMF files.
> +2. PDFiumChild, in the PDFium process, receives that request and starts to

s/receives/then receives/

::: widget/windows/PPDFium.ipdl:9
(Diff revision 3)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +namespace mozilla {
> +namespace widget {
> +
> +protocol PPDFium

This could do with some documentation. Even something  simple like "A protocol for communicating to the PDFium process. Useful for converting PDF files to EMF for printing, for example."

::: widget/windows/PPDFium.ipdl:13
(Diff revision 3)
> +
> +protocol PPDFium
> +{
> +parent:
> +  /**
> +   * A EMF file is ready for printing.

Expand on this comment. Something like:

Called by the PDFium process once the PDF that is to be converted to EMF is ready to print. The parent is responsible for removing the EMF file once it is done with it.

::: widget/windows/PPDFium.ipdl:15
(Diff revision 3)
> +{
> +parent:
> +  /**
> +   * A EMF file is ready for printing.
> +   */
> +  async PrintEMF(nsString aEMFFilePath);

Printing is incidental. What PDFium is being used for here is PDF-to-EMF conversion. Call this DoneConvertingToEMF?

::: widget/windows/PPDFium.ipdl:18
(Diff revision 3)
> +   * A EMF file is ready for printing.
> +   */
> +  async PrintEMF(nsString aEMFFilePath);
> +
> +  /**
> +   * All printing jobs are done.

PDFium doesn't do any printing, so this name is misleading. Maybe call it FinishedEMFConversions, or PDFToEMFQueueIsEmpty, or something like that (so it will make sense from the point of view of someone encountering the implementation of that function).

Change the comment to match, and mention why this is needed - i.e. what the parent will find it useful for. It's not immediately clear since PrintEMF passes over a finished file.

Also, change the closing comment to */ instead of **/ while you're here.

::: widget/windows/PPDFium.ipdl:26
(Diff revision 3)
> +
> +child:
> +  /**
> +   * Start to convert a PDF file into EMF files.
> +   */
> +  async StartPrint(nsString aPDFFilePath, int aPageWidth, int aPageHeight);

Call this ConvertToEMF?
Attachment #8919758 - Flags: review?(jwatt)
Attachment #8919758 - Flags: feedback+
Comment on attachment 8919759 [details]
Bug 1399787 - Part 5. Implement the PDFium process.

https://reviewboard.mozilla.org/r/190696/#review198136

::: xpcom/build/nsXULAppAPI.h:389
(Diff revision 4)
>    "plugin",
>    "tab",
>    "ipdlunittest",
>    "geckomediaplugin",
> -  "gpu"
> +  "gpu",
> +  "pdfiump"

Why is this "pdfiump" instead of just "pdfium"?
Attachment #8919759 - Flags: review?(jwatt)
Attachment #8919759 - Flags: feedback+
Comment on attachment 8921859 [details]
Bug 1399787 - Part 10. Report and handle errors while generating EMF.

https://reviewboard.mozilla.org/r/192890/#review198140


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/printing/nsPrintEngine.cpp:677
(Diff revision 1)
>    }
> -
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  rv = devspec->Init(nullptr, printData->mPrintSettings, aIsPrintPreview,
> +  	                 [this](nsresult aResult) {
> +  	                   if (NS_FAILED(aResult)) {
> +  	                     CleanupOnFailure(aResult, true);

Error: Refcounted variable 'this' of type 'nsprintengine' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]

::: layout/printing/nsPrintEngine.cpp:677
(Diff revision 1)
>    }
> -
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  rv = devspec->Init(nullptr, printData->mPrintSettings, aIsPrintPreview,
> +  	                 [this](nsresult aResult) {
> +  	                   if (NS_FAILED(aResult)) {
> +  	                     CleanupOnFailure(aResult, true);

Error: Refcounted variable 'this' of type 'nsprintengine' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]
Comment on attachment 8919760 [details]
Bug 1399787 - Part 4. Launch the PDFium process.

https://reviewboard.mozilla.org/r/190698/#review198144

::: widget/windows/nsDeviceContextSpecWin.cpp:466
(Diff revision 5)
>      }
>  
>      mPrintViaPDFInProgress = true;
> +
> +    MOZ_ASSERT(!mPDFiumProcess);
> +    mPDFiumProcess = new PDFiumProcessParent();

Do we want to create a new PDFium process for every document that is printed? (Well, every PDF currently, but potentially every document if we start printing everything by converting to PDF using SkPDF.) I had expected we'd create a single process and send conversion jobs to it to queue up. I'm not sure how much it matters in practice, but I'd be interested to hear your thoughts.

Also, do you happen to know what Chromium does?
Attachment #8919760 - Flags: review?(jwatt)
Needinfo'ing Bob so that at the very least he's aware of what's going on here.
Flags: needinfo?(bobowencode)
Comment on attachment 8919760 [details]
Bug 1399787 - Part 4. Launch the PDFium process.

https://reviewboard.mozilla.org/r/190698/#review198144

> Do we want to create a new PDFium process for every document that is printed? (Well, every PDF currently, but potentially every document if we start printing everything by converting to PDF using SkPDF.) I had expected we'd create a single process and send conversion jobs to it to queue up. I'm not sure how much it matters in practice, but I'd be interested to hear your thoughts.
> 
> Also, do you happen to know what Chromium does?

Although we allow to print several documents at the same time, but a user rarely does it. Print a document once a time is the most generic use case. If we keep a single process waiting for EMF-conversion command, that process will ocuppy system resource all the time, but most of time no one use it. One PDFium process for one document print job means we kill that process immidiately after printing and return the resoruce back to the system. Another benifit for this design is simplicity. As you can see in part 6 and 7, the code in these two patches are very easy to read, and the protocol for this process is very concise as well.

Not sure how Chromium does, but I will try to figure it out.
Attachment #8919765 - Flags: review?(bobowencode)
Comment on attachment 8919760 [details]
Bug 1399787 - Part 4. Launch the PDFium process.

https://reviewboard.mozilla.org/r/190698/#review198144

> Although we allow to print several documents at the same time, but a user rarely does it. Print a document once a time is the most generic use case. If we keep a single process waiting for EMF-conversion command, that process will ocuppy system resource all the time, but most of time no one use it. One PDFium process for one document print job means we kill that process immidiately after printing and return the resoruce back to the system. Another benifit for this design is simplicity. As you can see in part 6 and 7, the code in these two patches are very easy to read, and the protocol for this process is very concise as well.
> 
> Not sure how Chromium does, but I will try to figure it out.

Chromium creates one new process for each printing job.
(In reply to C.J. Ku[:cjku](UTC+8) from comment #64)
> If we keep a single process waiting for EMF-conversion command, that process
> will ocuppy system resource all the time, but most of time no one use it.

Well, we could create the process on demand and kill it off a timer after last conversion. But anyway...

> Another benifit for this design is simplicity.

Yes, this is valuable.

One process per print job also limits the ability of one malicious page being printed to interfere with another.

So sounds good, let's go with one process per print job.

(In reply to C.J. Ku[:cjku](UTC+8) from comment #65)
> Chromium creates one new process for each printing job.

Thanks for checking.
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review198942

Generally this looks good, thanks.
I think we can add a few more things to the policy.

::: ipc/glue/GeckoChildProcessHost.cpp:974
(Diff revision 9)
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +    case GeckoProcessType_PDFium:

I notice that when this is added into the enum it isn't #ifed, I'm guessing this is because of the static_assert in nsAppRunner.

::: ipc/glue/GeckoChildProcessHost.cpp:1087
(Diff revision 9)
>      // We need to be able to duplicate handles to some types of non-sandboxed
>      // child processes.
>      if (mProcessType == GeckoProcessType_Content ||
>          mProcessType == GeckoProcessType_GPU ||
> -        mProcessType == GeckoProcessType_GMPlugin) {
> +        mProcessType == GeckoProcessType_GMPlugin ||
> +        mProcessType == GeckoProcessType_PDFium) {

We only need this if we are going to duplicate handles from other processes to this one via the broker. For example shared memory.
It would only cause an issue if the sandbox were disabled anyway.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:834
(Diff revision 9)
>  
>    return true;
>  }
>  
>  bool
> +SandboxBroker::SetSecurityLevelForPDFiumProcess()

Missing #if

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:864
(Diff revision 9)
> +  sandbox::MitigationFlags mitigations =
> +    sandbox::MITIGATION_BOTTOM_UP_ASLR |
> +    sandbox::MITIGATION_HEAP_TERMINATE |
> +    sandbox::MITIGATION_SEHOP |
> +    sandbox::MITIGATION_DEP_NO_ATL_THUNK |
> +    sandbox::MITIGATION_DEP;

I would have thought you could use all the mitigations that we are using in the content process.
Including the delayed mitigations.

I think that Chromium disables win32k for the PPAPI process, so it would be great if we could do that as well.

I assume we can also use the alternate desktop and winstation same as for the GMP process.
Attachment #8919765 - Flags: review?(bobowencode)
Flags: needinfo?(bobowencode)
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review199068

::: commit-message-0f7b7:3
(Diff revision 9)
> +The PDFium process need privilege to access, includes read and write, files in
> +NS_OS_TEMP_DIR and to use windows GDI API only. Since we do not need special
> +permission for read/write/create a file under TEMP folder, the security level
> +that I give to this process is very restricted.

This comment doesn't make sense with the policy.
That's possibly because you're not calling the following anywhere:
mozilla::SandboxTarget::Instance()->StartSandbox()

You should do this as soon as you've done anything that requires the initial less strict permissions.

You should call that before we process anything that is untrusted, that possibly includes loading the pdfium binary, although you'll need to add a rule to be able to read it in that case.
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review198942

> I would have thought you could use all the mitigations that we are using in the content process.
> Including the delayed mitigations.
> 
> I think that Chromium disables win32k for the PPAPI process, so it would be great if we could do that as well.
> 
> I assume we can also use the alternate desktop and winstation same as for the GMP process.

mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_WIN32K_LOCKDOWN,
                           sandbox::TargetPolicy::FAKE_USER_GDI_INIT, nullptr);
 mitigations |= sandbox::MITIGATION_EXTENSION_POINT_DISABLE;
 mPolicy->SetProcessMitigations(mitigations);
 
If I add these codes into SetSecurityLevelForPDFiumProcess, then the pdfium process can not be created sucessfully. Do not know why....
Blocks: 1412933
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review198942

> mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_WIN32K_LOCKDOWN,
>                            sandbox::TargetPolicy::FAKE_USER_GDI_INIT, nullptr);
>  mitigations |= sandbox::MITIGATION_EXTENSION_POINT_DISABLE;
>  mPolicy->SetProcessMitigations(mitigations);
>  
> If I add these codes into SetSecurityLevelForPDFiumProcess, then the pdfium process can not be created sucessfully. Do not know why....

Create bug 1412933 to follow up this requirement: disables win32k for the pdfium process
Attachment #8919758 - Flags: review?(jwatt)
Attachment #8919759 - Flags: review?(jwatt)
Attachment #8919760 - Flags: review?(jwatt)
Attachment #8919765 - Flags: review?(bobowencode)
Attachment #8923507 - Flags: review?(bobowencode)
Attachment #8919758 - Flags: review?(jwatt)
Comment on attachment 8923507 [details]
Bug 1399787 - Part 12. Put temporay PDF and EMF files into NS_APP_CONTENT_PROCESS_TEMP_DIR.

https://reviewboard.mozilla.org/r/194646/#review199970

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:892
(Diff revision 1)
> +  // Add rule to allow read / write access to content temp dir. 
> +  AddCachedDirRule(mPolicy, sandbox::TargetPolicy::FILES_ALLOW_ANY,
> +                   sContentTempDir, NS_LITERAL_STRING("\\*"));

We're trying to phase out use of this, I'd much rather that we passed down file handles from the parent.

For example, the existing printing code is in the process of moving to using NS_OpenAnonymousTemporaryFile to open the file in the parent.
Attachment #8923507 - Flags: review?(bobowencode)
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review199966

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:846
(Diff revisions 9 - 10)
> -                             0 /* ui_exceptions */);
> +                            0 /* ui_exceptions */);
>    SANDBOX_ENSURE_SUCCESS(result,
>                           "SetJobLevel should never fail with these arguments, what happened?");
> -
>    result = mPolicy->SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS,
> -                                  sandbox::USER_LOCKDOWN);
> +                                  sandbox::USER_LIMITED);

Why the change here?
As this is a new process we ought to be able to run at USER_LOCKDOWN, I would have thought.
I believe Chromium does.

We might need to load any required system DLLs before the StartSandbox call and possible other initialisation.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:859
(Diff revisions 9 - 10)
>    result = mPolicy->SetIntegrityLevel(sandbox::INTEGRITY_LEVEL_LOW);
>    MOZ_ASSERT(sandbox::SBOX_ALL_OK == result,
>               "SetIntegrityLevel should never fail with these arguments, what happened?");
>  
>    result =
> -    mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_UNTRUSTED);
> +    mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_LOW);

Again I think we should be able to use UNTRUSTED as Chromium does.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:863
(Diff revisions 9 - 10)
>    result =
> -    mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_UNTRUSTED);
> +    mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_LOW);
>    SANDBOX_ENSURE_SUCCESS(result,
>                           "SetIntegrityLevel should never fail with these arguments, what happened?");
>  
>    sandbox::MitigationFlags mitigations =

I'm OK with the win32k lockdown being a follow-up, but I'm surprised we can't use MITIGATION_EXTENSION_POINT_DISABLE here.

Also, MITIGATION_IMAGE_LOAD_NO_LOW_LABEL and MITIGATION_IMAGE_LOAD_NO_REMOTE (when not running from a network drive).

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:894
(Diff revisions 9 - 10)
>    SANDBOX_ENSURE_SUCCESS(result,
>                           "With these static arguments AddRule should never fail, what happened?");
>  
> +  // Add this rule for loading pdfium.dll.
> +  AddCachedDirRule(mPolicy, sandbox::TargetPolicy::FILES_ALLOW_READONLY,
> +                   sBinDir, NS_LITERAL_STRING("\\*"));

Can we make this specific to just the pdfium DLL name?
This wip is to meet the bob's request in comment 93.
We need to
1. Send the FileDescriptor of the temporary PDF file from the chrome process to the PDFium process.
   a. ipdl change.
   b. Implement PDFiumEngineShim::LoadDocument(const FileDescriptor& aFD) to be able to read FD in the PDFium process.
2. Send EMF content from the PDFium process back to the chrome process
   a. Change sandbox setting to enable share HANDLE.
   b. HENHMETAFILE is not sharable, we need to read the content of EMF to a smem and send that smem back to the chrome process.
   c. WindowsEME need to be able read and write EMF content form a smem.
C.J., one thing that occurs to me is that if we split the PDF file up into multiple EMF files (one per page), the user is going to see one print job per page (there could be hundreds). If, like I some times do, a user wants to cancel printing of a document then it's going to be a real headache.
Flags: needinfo?(cku)
Other than that architectural issue I think I'm pretty much ready to review this now. I would like your comments on the above first though.
(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #96)
> C.J., one thing that occurs to me is that if we split the PDF file up into
> multiple EMF files (one per page), the user is going to see one print job
> per page (there could be hundreds). If, like I some times do, a user wants
> to cancel printing of a document then it's going to be a real headache.

Yes, I think we should take care of it and I will fix it. Please hold on this review, I will update patches to fix yours and bob's concern.
Flags: needinfo?(cku)
(In reply to C.J. Ku[:cjku](UTC+8) from comment #98)
> (In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #96)
> > C.J., one thing that occurs to me is that if we split the PDF file up into
> > multiple EMF files (one per page), the user is going to see one print job
> > per page (there could be hundreds). If, like I some times do, a user wants
> > to cancel printing of a document then it's going to be a real headache.
> 
> Yes, I think we should take care of it and I will fix it. Please hold on
> this review, I will update patches to fix yours and bob's concern.
Ok, I already have a WIP that we can cancel printing job, still need a few day to make that patch reviewable.
Attachment #8919760 - Attachment is obsolete: true
Attachment #8919763 - Attachment is obsolete: true
Attachment #8919763 - Flags: review?(jwatt)
Attachment #8920649 - Attachment is obsolete: true
Attachment #8920649 - Flags: review?(jwatt)
Attachment #8920650 - Attachment is obsolete: true
Attachment #8920650 - Flags: review?(jwatt)
Attachment #8921859 - Attachment is obsolete: true
Attachment #8921859 - Flags: review?(jwatt)
Attachment #8923507 - Attachment is obsolete: true
Attachment #8925556 - Flags: review?(jwatt)
Attachment #8925557 - Flags: review?(jwatt)
Attachment #8925558 - Flags: review?(jwatt)
Attachment #8919758 - Flags: review?(jwatt)
Attachment #8925559 - Flags: review?(jwatt)
Attachment #8919759 - Flags: review?(jwatt)
Attachment #8925560 - Flags: review?(jwatt)
Attachment #8925561 - Flags: review?(jwatt)
Attachment #8919765 - Flags: review?(bobowencode)
Attachment #8925566 - Flags: review?(jwatt)
Attachment #8925567 - Flags: review?(jwatt)
Attachment #8925562 - Flags: review?(jwatt)
Attachment #8925565 - Flags: review?(jwatt)
Attachment #8925563 - Flags: review?(jwatt)
Attachment #8925564 - Flags: review?(jwatt)
Comment on attachment 8925563 [details]
Bug 1399787 - Part 11.a. Use PrintTargetEMF to print content documents.

https://reviewboard.mozilla.org/r/196690/#review201938


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/src/nsDeviceContext.cpp:750
(Diff revision 1)
> +
> +void
> +nsDeviceContext::RegisterPagePrintedCallback(PagePrintedCallback aCallback)
> +{
> +  MOZ_ASSERT(mPrintTarget && aCallback && !IsSyncPrint());
> +  mPrintTarget->RegisterPagePrintedCallback(aCallback);

Warning: Parameter 'acallback' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

::: gfx/thebes/PrintTarget.cpp:236
(Diff revision 1)
>  
> +void
> +PrintTarget::RegisterPagePrintedCallback(PagePrintedCallback aCallback)
> +{
> +    MOZ_ASSERT(aCallback && !IsSyncPrint());
> +    mPagePrintedCallback = aCallback;

Warning: Parameter 'acallback' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]
Comment on attachment 8925556 [details]
Bug 1399787 - Part 2.a. Add functions to load the content of a PDF from a FileDescriptor.

https://reviewboard.mozilla.org/r/196676/#review202550

::: widget/windows/PDFViaEMFPrintHelper.cpp:83
(Diff revision 2)
>  
> +nsresult
> +PDFViaEMFPrintHelper::OpenDocument(const FileDescriptor& aFD)
> +{
> +  if (mPDFDoc) {
> +    MOZ_ASSERT_UNREACHABLE("We can only open one PDF at a time,"

You need a space character at the end of the first two strings.

::: widget/windows/PDFViaEMFPrintHelper.cpp:98
(Diff revision 2)
> +  NS_ENSURE_TRUE(prfile, NS_ERROR_FAILURE);
> +
> +  mPDFDoc = mPDFiumEngine->LoadDocument(prfile, nullptr);
> +  NS_ENSURE_TRUE(mPDFDoc, NS_ERROR_FAILURE);
> +
> +  // mPDFiumEngine keeps use this handle until we close mPDFDoc. Instead of

s/use/using/

::: widget/windows/PDFViaEMFPrintHelper.cpp:99
(Diff revision 2)
> +
> +  mPDFDoc = mPDFiumEngine->LoadDocument(prfile, nullptr);
> +  NS_ENSURE_TRUE(mPDFDoc, NS_ERROR_FAILURE);
> +
> +  // mPDFiumEngine keeps use this handle until we close mPDFDoc. Instead of
> +  // closing this HANDLE here, close it in PDFViaEMFPrintHelper::CloseDocument.

s/close/we close/
Attachment #8925556 - Flags: review?(jwatt) → review+
Comment on attachment 8925557 [details]
Bug 1399787 - Part 2.b. Add functions to read/write EMF content from/to a buffer.

https://reviewboard.mozilla.org/r/196678/#review202554

::: widget/windows/PDFViaEMFPrintHelper.h:61
(Diff revision 2)
>  
>    /** Convert specified PDF page to EMF and save it to file. */
>    bool DrawPageToFile(const wchar_t* aFilePath, unsigned int aPageIndex,
>                        int aPageWidth, int aPageHeight);
>  
> +  /** Create a share memory and serialize the EMF content into it. */

s/Create a/Create/
Attachment #8925557 - Flags: review?(jwatt) → review+
Comment on attachment 8925558 [details]
Bug 1399787 - Part 2.c. Rename DrawPageToFile to SavePageToFile.

https://reviewboard.mozilla.org/r/196680/#review202560
Attachment #8925558 - Flags: review?(jwatt) → review+
Comment on attachment 8925559 [details]
Bug 1399787 - Part 4. Take out the code we landed in bug 1370488.

https://reviewboard.mozilla.org/r/196682/#review202562

::: commit-message-59ad3:5
(Diff revision 2)
> +Bug 1399787 - Part 4. Take out the code we landed in bug 1370488.
> +
> +To move EMF conversion job to a dedicated process, I will implement a new
> +PrintTarget subclass, named PrintTargetEMF, to coordinate tasks among the
> +content process/ chrome process and PDFium process. All the code that we

Replace the "/" with ","
Attachment #8925559 - Flags: review?(jwatt) → review+
Comment on attachment 8925567 [details]
Bug 1399787 - Part 16. Hide function table in PDFiumEngineShim.cpp.

https://reviewboard.mozilla.org/r/196698/#review202564
Attachment #8925567 - Flags: review?(jwatt) → review+
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review202840

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:871
(Diff revision 12)
> +    sandbox::MITIGATION_SEHOP |
> +    sandbox::MITIGATION_DEP_NO_ATL_THUNK |
> +    sandbox::MITIGATION_DEP |
> +    sandbox::MITIGATION_EXTENSION_POINT_DISABLE |
> +    sandbox::MITIGATION_IMAGE_LOAD_NO_LOW_LABEL |
> +    sandbox::MITIGATION_IMAGE_LOAD_NO_REMOTE;

I think you'll need to test for |!sRunningFromNetworkDrive| before adding MITIGATION_IMAGE_LOAD_NO_REMOTE, same as for the content process.

::: widget/windows/PDFiumProcessChild.cpp:39
(Diff revision 12)
> +  // Preload pdfium.dll before we lower down the privilege of the
> +  // access token.
> +  mPDFium = PR_LoadLibrary("pdfium.dll");
> +  mozilla::SandboxTarget::Instance()->StartSandbox();

Why has this changed to load before we lower the sandbox?
I thought you had this working the other way around. 

Some explanation in the bug or even here, would help.

Also, the load should probably be outside the |#if defined(MOZ_SANDBOX)| and do we need to check the library loaded successfully?
Attachment #8919765 - Flags: review?(bobowencode) → review-
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review202840

> Why has this changed to load before we lower the sandbox?
> I thought you had this working the other way around. 
> 
> Some explanation in the bug or even here, would help.
> 
> Also, the load should probably be outside the |#if defined(MOZ_SANDBOX)| and do we need to check the library loaded successfully?

Hi Bob,
<bobowen> CJKu: we manage to do it for the GMP process, do you know what is getting blocked?

Here is how GMP process did it:
Lauch GMP plugin at [1]
Open channel for ipc at [2]
Preload libraries at [3]
StartSandbox at [4]

GMP reploads all librarys it needs before calling StartSandbox to lower down token level to USER_LOCKDOWN.

[1]
https://hg.mozilla.org/mozilla-central/file/4e4757f3ec89/dom/media/gmp/GMPParent.cpp#l160
[2]
https://hg.mozilla.org/mozilla-central/file/4e4757f3ec89/dom/media/gmp/GMPParent.cpp#l170
[3]
https://hg.mozilla.org/mozilla-central/file/4e4757f3ec89/dom/media/gmp/GMPParent.cpp#l182
[4]
https://hg.mozilla.org/mozilla-central/file/4e4757f3ec89/dom/media/gmp/GMPParent.cpp#l192
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review202840

> Hi Bob,
> <bobowen> CJKu: we manage to do it for the GMP process, do you know what is getting blocked?
> 
> Here is how GMP process did it:
> Lauch GMP plugin at [1]
> Open channel for ipc at [2]
> Preload libraries at [3]
> StartSandbox at [4]
> 
> GMP reploads all librarys it needs before calling StartSandbox to lower down token level to USER_LOCKDOWN.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/4e4757f3ec89/dom/media/gmp/GMPParent.cpp#l160
> [2]
> https://hg.mozilla.org/mozilla-central/file/4e4757f3ec89/dom/media/gmp/GMPParent.cpp#l170
> [3]
> https://hg.mozilla.org/mozilla-central/file/4e4757f3ec89/dom/media/gmp/GMPParent.cpp#l182
> [4]
> https://hg.mozilla.org/mozilla-central/file/4e4757f3ec89/dom/media/gmp/GMPParent.cpp#l192

And put the log on irc here, which explain why I did this change:
17:03 bobowen: this is because in comment 94
17:03 bobowen: you suggest set lock-down token level as USER_LOCKDOWN SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS, sandbox::USER_LOCKDOWN);
17:04 bobowen: I found I can not load any dll(including pdfium.dll) after I lower down job token
17:05 bobowen: that why I preload pdfium.dll before calling SandboxTarget::Instance()->StartSandbox()
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review202840

> And put the log on irc here, which explain why I did this change:
> 17:03 bobowen: this is because in comment 94
> 17:03 bobowen: you suggest set lock-down token level as USER_LOCKDOWN SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS, sandbox::USER_LOCKDOWN);
> 17:04 bobowen: I found I can not load any dll(including pdfium.dll) after I lower down job token
> 17:05 bobowen: that why I preload pdfium.dll before calling SandboxTarget::Instance()->StartSandbox()

I think we do not need to move PR_LoadLibrary("pdfium.dll") out of  |#if defined(MOZ_SANDBOX)| since we need to preload this lib only if MOZ_SANDBOX is enable.
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review202840

> I think we do not need to move PR_LoadLibrary("pdfium.dll") out of  |#if defined(MOZ_SANDBOX)| since we need to preload this lib only if MOZ_SANDBOX is enable.

With MOZ_SANDBOX_LOGGING=1, I did not get any information of why PR_LoadLibrary("pdfium.dll") failed to load pdfium.dll.
GetLastError returns 0 which means no error even if PR_LoadLibrary actually returns nullptr.
(In reply to C.J. Ku[:cjku](UTC+8) from comment #142)
...
> GMP reploads all librarys it needs before calling StartSandbox to lower down
> token level to USER_LOCKDOWN.

Just for the record, the dependencies are loaded before lowering the sandbox, but the GMP DLL is loaded after.

Anyway, I've talked further with CJKu on IRC and we've agreed to load before lowering for the moment and investigate why we can't load after in a follow-up bug.
Depends on: 1417000
Attachment #8919758 - Flags: review?(jwatt)
Attachment #8919759 - Flags: review?(jwatt)
Attachment #8919765 - Flags: review?(bobowencode)
Attachment #8928107 - Flags: review?(jwatt)
Attachment #8928108 - Flags: review?(jwatt)
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review204434

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:472
(Diff revision 13)
>    result = mPolicy->SetDelayedIntegrityLevel(delayedIntegrityLevel);
>    MOZ_RELEASE_ASSERT(sandbox::SBOX_ALL_OK == result,
>                       "SetDelayedIntegrityLevel should never fail, what happened?");
>  
> +  // XXX bug 1412933
> +  // We should also disables win32k for the PDFium process by adding 

nit: trailing whitespace.

::: widget/windows/PDFiumProcessChild.cpp:22
(Diff revision 13)
>  
>  namespace mozilla {
>  namespace widget {
>  
>  PDFiumProcessChild::PDFiumProcessChild(ProcessId aParentPid)
> -: ProcessChild(aParentPid)
> +: ProcessChild(aParentPid), mPDFium(nullptr)

With the new #ifs, I don't think this will compile if MOZ_SANDBOX is not defined now, as mPDFium won't exist.
Attachment #8919765 - Flags: review?(bobowencode)
Attachment #8924137 - Attachment is obsolete: true
Attachment #8915856 - Attachment is obsolete: true
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review204764

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:471
(Diff revision 14)
> +  // XXX bug 1412933
> +  // We should also disables win32k for the PDFium process by adding
> +  // MITIGATION_WIN32K_DISABLE flag here after fixing bug 1412933.

Sorry didn't spot this before, this comment is in the wrong function.
Should be in SetSecurityLevelForPDFiumProcess.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:885
(Diff revision 14)
> +    sandbox::MITIGATION_SEHOP |
> +    sandbox::MITIGATION_DEP_NO_ATL_THUNK |
> +    sandbox::MITIGATION_DEP |
> +    sandbox::MITIGATION_EXTENSION_POINT_DISABLE |
> +    sandbox::MITIGATION_IMAGE_LOAD_NO_LOW_LABEL |
> +    sandbox::MITIGATION_IMAGE_LOAD_NO_REMOTE;

Just spottted that this hasn't been fixed from previous review (comment 141).

If Firefox is running from a Network Drive, I think using MITIGATION_IMAGE_LOAD_NO_REMOTE will break things.
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review204764

> Just spottted that this hasn't been fixed from previous review (comment 141).
> 
> If Firefox is running from a Network Drive, I think using MITIGATION_IMAGE_LOAD_NO_REMOTE will break things.

Oops, sorry, something wrong when I merge code. Will update a new version to take change back
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review204808
Attachment #8919765 - Flags: review?(bobowencode) → review+
Attachment #8908137 - Attachment is obsolete: true
Attachment #8912579 - Attachment is obsolete: true
Attachment #8914207 - Attachment is obsolete: true
Comment on attachment 8925561 [details]
Bug 1399787 - Part 7. Have PrintTargetEMF launch the PDFium process.

https://reviewboard.mozilla.org/r/196686/#review204826

::: widget/windows/PrintTargetEMF.cpp:17
(Diff revision 4)
>  namespace mozilla {
>  namespace widget {
>  
>  PrintTargetEMF::PrintTargetEMF(HDC aDC, const IntSize& aSize)
>    : PrintTarget(/* not using cairo_surface_t */ nullptr, aSize)
>    , mDC(aDC)

, mPDFiumProcess(nullptr)
Comment on attachment 8919758 [details]
Bug 1399787 - Part 3. Create a top level protocol for the PDFium process.

https://reviewboard.mozilla.org/r/190694/#review198484

::: commit-message-a8ad8:7
(Diff revision 4)
> +
> +Define ipdl and actor classes. Implementation of actors is added in subsequent patches.
> +
> +Control flow
> +1. nsDeviceContextSpecWin, in chrome process, calls *ConvertToEMF* to ask the
> +   PDFium process starting to convert a PDF into page-base EMF files.

s/starting//
Attachment #8919758 - Flags: review+
Comment on attachment 8919759 [details]
Bug 1399787 - Part 5. Implement the PDFium process.

https://reviewboard.mozilla.org/r/190696/#review204890
Attachment #8919759 - Flags: review?(jwatt) → review+
Comment on attachment 8925560 [details]
Bug 1399787 - Part 6. Implement the bulk of a new PrintTargetEMF.

https://reviewboard.mozilla.org/r/196684/#review204894

::: widget/windows/PrintTargetEMF.h:25
(Diff revision 4)
> +  typedef mozilla::gfx::PrintTargetSkPDF PrintTargetSkPDF;
> +
> +  static already_AddRefed<PrintTargetEMF>
> +  CreateOrNull(HDC aDC, const IntSize& aSizeInPoints);
> +
> +  virtual nsresult BeginPrinting(const nsAString& aTitle,

Don't specify 'virtual' on functions where you're using 'override'. See the text "Method declarations must use at most one of the following keywords: virtual, override, or final." in the style guide:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

This comment applies to several methods here.

::: widget/windows/PrintTargetEMF.h:39
(Diff revision 4)
> +  virtual already_AddRefed<DrawTarget>
> +  MakeDrawTarget(const IntSize& aSize,
> +                 DrawEventRecorder* aRecorder = nullptr) final;
> +
> +  virtual already_AddRefed<DrawTarget>
> +  GetReferenceDrawTarget(DrawEventRecorder* aRecorder) override final;

Note here you only need 'final'.
Comment on attachment 8925561 [details]
Bug 1399787 - Part 7. Have PrintTargetEMF launch the PDFium process.

https://reviewboard.mozilla.org/r/196686/#review204896

::: commit-message-d32a9:1
(Diff revision 4)
> +Bug 1399787 - Part 7. Launch the PDFium process.

Can you make this "Have PrintTargetEMF launch the PDFium process". It's a bit more descriptive in the context off all commits that are landing.

::: widget/windows/PrintTargetEMF.h:10
(Diff revision 4)
>  
>  #ifndef PrintTargetEMF_h___
>  #define PrintTargetEMF_h___
>  
>  #include "mozilla/gfx/PrintTargetSkPDF.h"
> +#include "mozilla/UniquePtr.h"

Seems like you were going to use UniquePtr but didn't finish that or had issues? It would be great if you could do that.
Attachment #8925561 - Flags: review?(jwatt) → review+
Comment on attachment 8919761 [details]
Bug 1399787 - Part 8. Open PDFium protocol channel.

https://reviewboard.mozilla.org/r/190700/#review204898

::: widget/windows/PDFiumProcessParent.h:43
(Diff revision 11)
>  
>  private:
>  
>    DISALLOW_COPY_AND_ASSIGN(PDFiumProcessParent);
> +
> +  RefPtr<PDFiumParent> mPDFiumActor;

Why "Actor" rather than "Parent", or "ParentActor"?
Attachment #8919761 - Flags: review?(jwatt) → review+
Comment on attachment 8919765 [details]
Bug 1399787 - Part 9. Sandbox the PDFium process.

https://reviewboard.mozilla.org/r/190708/#review204902

Looks fine for now, but not my area of expertise so I'm mostly trusting bobowen's review here.
Attachment #8919765 - Flags: review?(jwatt) → review+
Comment on attachment 8925561 [details]
Bug 1399787 - Part 7. Have PrintTargetEMF launch the PDFium process.

https://reviewboard.mozilla.org/r/196686/#review204896

> Seems like you were going to use UniquePtr but didn't finish that or had issues? It would be great if you could do that.

mPDFiumProcess can not be destroy directly if it is still waiting a response from the pdfium process. that's way I did not declare the type of mPDFiumProcess as UniquePtr<PDFiumProcessParent>. I will remove this "#include" and hide the dtor of PDFiumProcessParent.
Attachment #8925560 - Flags: review?(jwatt)
Attachment #8930380 - Flags: review?(jwatt)
Blocks: pdf-printing
No longer blocks: 1412933
status-firefox57: wontfix → ---
Depends on: 1412933
Priority: P3 → P1
Comment on attachment 8925563 [details]
Bug 1399787 - Part 11.a. Use PrintTargetEMF to print content documents.

https://reviewboard.mozilla.org/r/196690/#review208682

::: layout/printing/ipc/RemotePrintJobParent.cpp:84
(Diff revision 7)
>      return rv;
>    }
>  
> +  if (!mPrintDeviceContext->IsSyncPagePrinting()) {
> +    mPrintDeviceContext->RegisterPageDoneCallback(
> +      std::bind(&RemotePrintJobParent::PageDone, this,

We shouldn't be using std::bind nowadays. Please use a lambda. They're more readable and can be more performant.

I'm also still working out the lifetimes of RemotePrintJobParent to figure out whether we have potential use-after-free issues here, since bind-objects/lambdas don't keep the objects they refer to alive. This also applies to the other std::bind comments I'm about to make.

::: widget/windows/PrintTargetEMF.h:46
(Diff revision 7)
>    already_AddRefed<DrawTarget>
>    GetReferenceDrawTarget(DrawEventRecorder* aRecorder) final;
>  
>    void ConvertToEMFDone(const nsresult& aResult, mozilla::ipc::Shmem&& aEMF);
>  
> +  virtual bool IsSyncPagePrinting() const { return false; }

Use override instead of virtual here.
Comment on attachment 8928107 [details]
Bug 1399787 - Part 11.b. Use PrintTargetEMF to print chrome documents.

https://reviewboard.mozilla.org/r/199354/#review208684

::: layout/printing/nsPrintEngine.cpp:678
(Diff revision 5)
>    rv = printData->mPrintDC->InitForPrinting(devspec);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  if (!XRE_IsContentProcess() && !printData->mPrintDC->IsSyncPagePrinting()) {
> +    printData->mPrintDC->RegisterPageDoneCallback(
> +      std::bind(&nsPrintEngine::PageDone, this, std::placeholders::_1));

We shouldn't be using std::bind nowadays. Please use a lambda. They're more readable and can be more performant.
Comment on attachment 8925565 [details]
Bug 1399787 - Part 13. Handle AbortDocument.

https://reviewboard.mozilla.org/r/196694/#review208686

::: widget/windows/PDFiumProcessParent.cpp:50
(Diff revision 7)
>  }
>  
>  void
> -PDFiumProcessParent::Delete()
> +PDFiumProcessParent::Delete(bool aWaitingForEMFConversion)
>  {
> +  // Can not kill the PDFium process yet since we are still waiting for a

This comment belongs inside the |if| block.

::: widget/windows/PDFiumProcessParent.cpp:53
(Diff revision 7)
> -PDFiumProcessParent::Delete()
> +PDFiumProcessParent::Delete(bool aWaitingForEMFConversion)
>  {
> +  // Can not kill the PDFium process yet since we are still waiting for a
> +  // EMF conversion response.
> +  if (aWaitingForEMFConversion) {
> +    mPDFiumParentActor->AbortConversion(std::bind(&PDFiumProcessParent::Delete,

We shouldn't be using std::bind nowadays. Please use a lambda. They're more readable and can be more performant.
Comment on attachment 8925563 [details]
Bug 1399787 - Part 11.a. Use PrintTargetEMF to print content documents.

https://reviewboard.mozilla.org/r/196690/#review208682

> We shouldn't be using std::bind nowadays. Please use a lambda. They're more readable and can be more performant.
> 
> I'm also still working out the lifetimes of RemotePrintJobParent to figure out whether we have potential use-after-free issues here, since bind-objects/lambdas don't keep the objects they refer to alive. This also applies to the other std::bind comments I'm about to make.

RemotePrintJobParent is destroyed in three cases
1. Print done: I think we are safe here.
2. Abort print: I think we are safe here
3. Content process die: Maybe I should unregist callback at RemotePrintJobParent::ActorDestroy
Comment on attachment 8925557 [details]
Bug 1399787 - Part 2.b. Add functions to read/write EMF content from/to a buffer.

https://reviewboard.mozilla.org/r/196678/#review208856

::: widget/windows/PDFViaEMFPrintHelper.h:57
(Diff revision 4)
>  
>    /** Convert specified PDF page to EMF and draw the EMF onto the given DC. */
>    bool DrawPage(HDC aPrinterDC, unsigned int aPageIndex,
>                  int aPageWidth, int aPageHeight);
>  
> -  /** Convert specified PDF page to EMF and save it to file. */
> +  /** Convert a specified PDF page to EMF and save it to file. */

That should be "the", not "a". But this sort of thing should really be in a separate "clean-up" bug since it's not part of the logical changes being made. It distracts from the main patch which makes both pre- and post-landing examination of a change annoying.

The same applies to the change to OpenDocument above.

::: widget/windows/PDFViaEMFPrintHelper.cpp:39
(Diff revision 4)
>  {
>    CloseDocument();
>  }
>  
>  nsresult
> -PDFViaEMFPrintHelper::OpenDocument(nsIFile *aFile)
> +PDFViaEMFPrintHelper::OpenDocument(nsIFile* aFile)

Same here.

::: widget/windows/WindowsEMF.h:46
(Diff revision 4)
>     * Initializes the object with an existing EMF file. Consumers cannot use
>     * GetDC() to obtain an HDC to modify the file. They can only use Playback().
>     */
>    bool InitFromFileContents(const wchar_t* aMetafilePath);
>  
> +  /*

Use /** please to make sure this is a doxygen comment too.

::: widget/windows/WindowsEMF.h:86
(Diff revision 4)
> +  /**
> +   * Return the size of the enhanced metafile, in bytes.
> +   */
> +  UINT GetEMFContentSize();
> +
> +  /*

And here.
Comment on attachment 8925557 [details]
Bug 1399787 - Part 2.b. Add functions to read/write EMF content from/to a buffer.

https://reviewboard.mozilla.org/r/196678/#review208864

::: widget/windows/WindowsEMF.h:52
(Diff revision 4)
> +   * creates the EMF from the specified data
> +   *
> +   * @param aByte Pointer to a buffer that contains EMF data.
> +   * @param aSize Specifies the size, in bytes, of aByte.
> +   */
> +  bool InitFromFileContents(PBYTE aByte, UINT aSize);

This should be called "aBytes" since it's a pointer to multiple bytes, not a single byte.

::: widget/windows/WindowsEMF.h:86
(Diff revision 4)
> +  /**
> +   * Return the size of the enhanced metafile, in bytes.
> +   */
> +  UINT GetEMFContentSize();
> +
> +  /*

( use /** )

::: widget/windows/WindowsEMF.h:91
(Diff revision 4)
> +  /*
> +   * Retrieves the contents of the EMF and copies them into a buffer.
> +   *
> +   * @param aByte the buffer to receive the data.
> +   */
> +  bool GetEMFContentBits(PBYTE aByte);

This should be called "aBytes" since it's a pointer to multiple bytes, not a single byte.

::: widget/windows/WindowsEMF.cpp:43
(Diff revision 4)
>    mEmf = ::GetEnhMetaFileW(aMetafilePath);
>    return !!mEmf;
>  }
>  
>  bool
> +WindowsEMF::InitFromFileContents(LPBYTE aByte, UINT aSize)

aBytes

::: widget/windows/WindowsEMF.cpp:110
(Diff revision 4)
> +
> +  return GetEnhMetaFileBits(mEmf, 0, NULL);
> +}
> +
> +bool
> +WindowsEMF::GetEMFContentBits(LPBYTE aByte)

aBytes
Comment on attachment 8925560 [details]
Bug 1399787 - Part 6. Implement the bulk of a new PrintTargetEMF.

https://reviewboard.mozilla.org/r/196684/#review208870

::: commit-message-3d7ec:1
(Diff revision 6)
> +Bug 1399787 - Part 6. Implement PrintTargetEMF.

This should be something like "Implement the bulk of a new PrintTargetEMF class" since the critical part to actually make this print via PDFium comes later.

::: commit-message-3d7ec:5
(Diff revision 6)
> +Bug 1399787 - Part 6. Implement PrintTargetEMF.
> +
> +A new subclass of PrintTarget.
> +1. It uses PrintTargetSkPDF to generate one PDF FileDescriptor for one page.
> +2. It then passes that FD to to PDFium process to convert PDF page to EMF

This should be "In a later patch..." since it doesn't do the PDFium part yet.

::: widget/windows/PrintTargetEMF.h:14
(Diff revision 6)
> +#include "mozilla/gfx/PrintTargetSkPDF.h"
> +
> +namespace mozilla {
> +namespace widget {
> +
> +class PrintTargetEMF final : public mozilla::gfx::PrintTarget

This should have a comment that contains pretty much the same information as you have in the commit message.

::: widget/windows/PrintTargetEMF.h:25
(Diff revision 6)
> +  typedef mozilla::gfx::PrintTargetSkPDF PrintTargetSkPDF;
> +
> +  static already_AddRefed<PrintTargetEMF>
> +  CreateOrNull(HDC aDC, const IntSize& aSizeInPoints);
> +
> +  nsresult BeginPrinting(const nsAString& aTitle,

Fix indentation.

::: widget/windows/PrintTargetEMF.cpp:89
(Diff revision 6)
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
> +
> +  nsAutoCString filePath;
> +  mPDFFileForOnePage->GetNativePath(filePath);
> +  auto  stream = MakeUnique<SkFILEWStream>(filePath.get());
> +  mTargetForCurrentPage = PrintTargetSkPDF::CreateOrNull(Move(stream), mSize);

Worth a comment here to explain why we create a new PrintTargetSkPDF for each page.

::: widget/windows/PrintTargetEMF.cpp:104
(Diff revision 6)
> +  mTargetForCurrentPage->EndPage();
> +  mTargetForCurrentPage->EndPrinting();
> +  mTargetForCurrentPage->Finish();
> +  mTargetForCurrentPage = nullptr;
> +
> +  // TBD: pass mPDFFileForOnePage to the PDFium process.

(For future reference, TBD means "to be decided", whereas what this seems to be is simply a TODO (which is done in part 10).)
Attachment #8925560 - Flags: review+
Comment on attachment 8925560 [details]
Bug 1399787 - Part 6. Implement the bulk of a new PrintTargetEMF.

https://reviewboard.mozilla.org/r/196684/#review208872

Oh, and another thing I meant to say about this patch - this file should really be in gfx/ along with the other PrintTarget classes. Why did you end up putting this in widget/?
Comment on attachment 8919758 [details]
Bug 1399787 - Part 3. Create a top level protocol for the PDFium process.

https://reviewboard.mozilla.org/r/190694/#review208876

::: widget/windows/PPDFium.ipdl:13
(Diff revision 8)
> +
> +
> +/**
> + * A protocol for communicating to the PDFium process. Useful for converting
> + * a PDF file to EMF contents.
> + **/

Just */ not **/

::: widget/windows/PPDFium.ipdl:19
(Diff revision 8)
> +protocol PPDFium
> +{
> +parent:
> +  /**
> +   * Called by the PDFium process once the PDF that is to be converted to EMF.
> +   **/

Just */ not **/
Comment on attachment 8919758 [details]
Bug 1399787 - Part 3. Create a top level protocol for the PDFium process.

https://reviewboard.mozilla.org/r/190694/#review208878

::: widget/windows/PPDFium.ipdl:18
(Diff revision 8)
> + **/
> +protocol PPDFium
> +{
> +parent:
> +  /**
> +   * Called by the PDFium process once the PDF that is to be converted to EMF.

s/that is to be/has been/
Comment on attachment 8919758 [details]
Bug 1399787 - Part 3. Create a top level protocol for the PDFium process.

https://reviewboard.mozilla.org/r/190694/#review208884

::: widget/windows/PPDFium.ipdl:11
(Diff revision 8)
> +namespace mozilla {
> +namespace widget {
> +
> +
> +/**
> + * A protocol for communicating to the PDFium process. Useful for converting

s/communicating to/communicating with/

And it should be "PDFium processes", not "the PDFium process", since there can be multiple processes as opposed to there being a singleton process.

To make it even clearer that there can be multiple processes please also tack on something like this at the end of the comment: "PDFium processes are created on-demand as necessary."
Comment on attachment 8920985 [details]
Bug 1399787 - Part 14. Prevent RemotePrintJobChild using ipc calls after the channel was destroyed.

https://reviewboard.mozilla.org/r/191956/#review208926

::: commit-message-90b85:1
(Diff revision 12)
> +Bug 1399787 - Part 14. Do not use ipc calls after the channel was destroyed.

s/Do not use/Prevert RemotePrintJobChild using/

::: commit-message-90b85:3
(Diff revision 12)
> +Bug 1399787 - Part 14. Do not use ipc calls after the channel was destroyed.
> +
> +If we force nsDeviceContextSpecWin::BeginDocument return NS_ERROR_FAILURE, then

s/If we force nsDeviceContextSpecWin::BeginDocument return/If in the future nsDeviceContextSpecWin::BeginDocument was to return/

But why would we do that? You change nsDeviceContextSpecWin::BeginDocument to be a no-op that simply returns NS_OK in part 4, so do we need this patch? It doesn't seem like a bad thing to have, but the motivation for the change that you're detailing here seems wrong.

::: commit-message-90b85:6
(Diff revision 12)
> +Bug 1399787 - Part 14. Do not use ipc calls after the channel was destroyed.
> +
> +If we force nsDeviceContextSpecWin::BeginDocument return NS_ERROR_FAILURE, then
> +the channel between RemotePrintJobParent and RemotePrintJobChild will be close
> +at [1]. RemotePrintJobChild keeps using ipc calls after the channel is broken
> +and then hits this assertion.

Which assertion? There's no mention of that in this patch's commit message or comments. You could just change this to just "hit assertions" if you don't think it's worth specifying.

::: layout/printing/ipc/RemotePrintJobChild.h:56
(Diff revision 12)
>  
>    bool mPrintInitialized = false;
>    nsresult mInitializationResult = NS_OK;
>    RefPtr<nsPagePrintTimer> mPagePrintTimer;
>    RefPtr<nsPrintEngine> mPrintEngine;
> +  bool mDestroyed;

Put this just after mPrintInitialized (better packing).

::: layout/printing/ipc/RemotePrintJobChild.cpp:20
(Diff revision 12)
>  
>  NS_IMPL_ISUPPORTS(RemotePrintJobChild,
>                    nsIWebProgressListener)
>  
>  RemotePrintJobChild::RemotePrintJobChild()
> + : mDestroyed(false)

Initialize this in the header (like mPrintInitialized), not here.

::: layout/printing/ipc/RemotePrintJobChild.cpp:52
(Diff revision 12)
>  void
>  RemotePrintJobChild::ProcessPage(const nsCString& aPageFileName)
>  {
>    MOZ_ASSERT(mPagePrintTimer);
>  
>    mPagePrintTimer->WaitForRemotePrint();

Looks like we should check mDestroyed here too.
Attachment #8920985 - Flags: review?(jwatt) → review+
Comment on attachment 8925562 [details]
Bug 1399787 - Part 10. Make PrintTargetEMF use the PDFium process to convert to EMF.

https://reviewboard.mozilla.org/r/196688/#review208930

::: commit-message-32d85:1
(Diff revision 7)
> +Bug 1399787 - Part 10. Convert PDF into EMF.

I think "Make PrintTargetEMF use the PDFium process to convert to EMF" would be more descriptive.
Attachment #8925562 - Flags: review?(jwatt) → review+
Comment on attachment 8925563 [details]
Bug 1399787 - Part 11.a. Use PrintTargetEMF to print content documents.

https://reviewboard.mozilla.org/r/196690/#review208932

We still need to resolve the lifetime issues but we can focus on that separately, and the rest of this looks good.

::: commit-message-a8667:1
(Diff revision 7)
> +Bug 1399787 - Part 11.a. Print content documents by PrintTargetEMF.

Make this: "Use PrintTargetEMF to print content documents".

::: gfx/thebes/PrintTarget.h:144
(Diff revision 7)
>    virtual already_AddRefed<DrawTarget> GetReferenceDrawTarget(DrawEventRecorder* aRecorder);
>  
> +  /**
> +   * If IsSyncPagePrinting returns true, then a user can assume the content of
> +   * a page was already printed after EndPage().
> +   * If IsSyncPagePrinting return false, then a user should register a callback

s/return/returns/

::: gfx/thebes/PrintTarget.h:145
(Diff revision 7)
>  
> +  /**
> +   * If IsSyncPagePrinting returns true, then a user can assume the content of
> +   * a page was already printed after EndPage().
> +   * If IsSyncPagePrinting return false, then a user should register a callback
> +   * function by RegisterPageDoneCallback to receive page print done

s/by/using/

::: gfx/thebes/PrintTarget.h:147
(Diff revision 7)
> +   * If IsSyncPagePrinting returns true, then a user can assume the content of
> +   * a page was already printed after EndPage().
> +   * If IsSyncPagePrinting return false, then a user should register a callback
> +   * function by RegisterPageDoneCallback to receive page print done
> +   * notifications.
> +   **/

Just */

::: layout/printing/ipc/RemotePrintJobParent.h:74
(Diff revision 7)
>                                   const nsString& aPrintToFile,
>                                   const int32_t& aStartPage,
>                                   const int32_t& aEndPage);
>  
>    nsresult PrintPage(const nsCString& aPageFileName);
> +  void PageDone(nsresult aResult);

Add some documentation for this. Something like:

  /**
   * Called to notify our corresponding RemotePrintJobChild once we've
   * finished printing a page.
   */

Also maybe call it OnPageDone.
Attachment #8925563 - Flags: review?(jwatt) → review+
Comment on attachment 8919759 [details]
Bug 1399787 - Part 5. Implement the PDFium process.

https://reviewboard.mozilla.org/r/190696/#review208960

::: widget/windows/PDFiumProcessChild.h:15
(Diff revision 11)
> +#include "PDFiumChild.h"
> +
> +namespace mozilla {
> +namespace widget {
> +
> +class PDFiumProcessChild final : public mozilla::ipc::ProcessChild {

Opening curly bracket on a new line.

And please add this comment:

  /**
   * Contains the PDFiumChild object that facilitates IPC communication to/from
   * the instance of the PDFium library that is run in this process.
   */

::: widget/windows/PDFiumProcessChild.cpp:19
(Diff revision 11)
> +
> +namespace mozilla {
> +namespace widget {
> +
> +PDFiumProcessChild::PDFiumProcessChild(ProcessId aParentPid)
> +: ProcessChild(aParentPid)

Indent.

::: widget/windows/PDFiumProcessParent.cpp:21
(Diff revision 11)
> +
> +namespace mozilla {
> +namespace widget {
> +
> +PDFiumProcessParent::PDFiumProcessParent()
> +: GeckoChildProcessHost(GeckoProcessType_PDFium)

Indent.
Comment on attachment 8928107 [details]
Bug 1399787 - Part 11.b. Use PrintTargetEMF to print chrome documents.

https://reviewboard.mozilla.org/r/199354/#review208934

We still need to check the lifetimes.

::: commit-message-3f943:1
(Diff revision 5)
> +Bug 1399787 - Part 11.b. Print chrome documents by PrintTargetEMF.

Make this: "Use PrintTargetEMF to print chrome documents".

::: layout/printing/nsPrintEngine.cpp:676
(Diff revision 5)
>  
>    printData->mPrintDC = new nsDeviceContext();
>    rv = printData->mPrintDC->InitForPrinting(devspec);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  if (!XRE_IsContentProcess() && !printData->mPrintDC->IsSyncPagePrinting()) {

This would be clearer if !XRE_IsContentProcess was replaced with XRE_IsParentProcess.

::: layout/printing/nsPrintEngine.cpp:2886
(Diff revision 5)
>      // true to notify the caller of current printing is done.
>      return true;
>    }
>  
> +  // We are printing a chrome document.
> +  if (!XRE_IsContentProcess() && !printData->mPrintDC->IsSyncPagePrinting()) {

Same here, and then you can remove the comment since it becomes redundant.
Attachment #8928107 - Flags: review?(jwatt) → review+
Comment on attachment 8928108 [details]
Bug 1399787 - Part 11.d. Using PrintTargetEMF on windows if skia-pdf is enable.

https://reviewboard.mozilla.org/r/199356/#review209178
Attachment #8928108 - Flags: review?(jwatt) → review+