Closed Bug 1399787 Opened 8 years ago Closed 7 years ago

Create a new sandboxed process to run pdfium

Categories

(Core :: Printing: Output, enhancement, P1)

Product:
Core
Component:
Printing: Output
Platform:
Unspecified
Windows
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: fatseng, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(23 files, 17 obsolete files)

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
Attached patch [WIP] Part1 create a sandboxed process (obsolete) — — Splinter Review
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
Attached patch [WIP] part2 setup IPC (obsolete) — — Splinter Review
Setup an IPC between Chrome and a sandboxed process.
Attached 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
Attached 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
Attached patch merged patch (obsolete) — — Splinter Review
Assignee: fatseng → cku
Attached patch merged patch (obsolete) — — Splinter Review
Attachment #8915526 - Attachment is obsolete: true
Attached 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?
Attached patch Don't access files in the pdfium process (obsolete) — — Splinter Review
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+
Comment on attachment 8925564 [details] Bug 1399787 - Part 12. Delay dispatching FinalizePrint message until the last page was processed. https://reviewboard.mozilla.org/r/196692/#review209190 FWIW patches like this that are completely independant of the rest of the patch series and just make good sense could land independently in a separate bug. That helps remove some of the complexity from the review process in large bugs like this. ::: commit-message-34113:6 (Diff revision 7) > +Bug 1399787 - Part 12. Delay dispatching FinalizePrint message until the last page was processed. > + > +For the last page, here is the final three messages sent between the content > +process, RemotePrintJobChild, and the chrome process, RemotePrintJobParent, for > +printing: > +1. The content process sends *ProcessPage* to the chrome process by s/by/via/ ::: commit-message-34113:7 (Diff revision 7) > + > +For the last page, here is the final three messages sent between the content > +process, RemotePrintJobChild, and the chrome process, RemotePrintJobParent, for > +printing: > +1. The content process sends *ProcessPage* to the chrome process by > + SendProcessPrint to request the chrome process printing the last page. s/printing/print/ ::: commit-message-34113:8 (Diff revision 7) > +For the last page, here is the final three messages sent between the content > +process, RemotePrintJobChild, and the chrome process, RemotePrintJobParent, for > +printing: > +1. The content process sends *ProcessPage* to the chrome process by > + SendProcessPrint to request the chrome process printing the last page. > +2. The content process sends *FinalizePrint* to the chrome process by s/by/via/ ::: commit-message-34113:9 (Diff revision 7) > +process, RemotePrintJobChild, and the chrome process, RemotePrintJobParent, for > +printing: > +1. The content process sends *ProcessPage* to the chrome process by > + SendProcessPrint to request the chrome process printing the last page. > +2. The content process sends *FinalizePrint* to the chrome process by > + SendFinalizePrint to notify the chrome that there is no more print s/is no more print request/are no outstanding print requests/ ::: commit-message-34113:10 (Diff revision 7) > +printing: > +1. The content process sends *ProcessPage* to the chrome process by > + SendProcessPrint to request the chrome process printing the last page. > +2. The content process sends *FinalizePrint* to the chrome process by > + SendFinalizePrint to notify the chrome that there is no more print > + request, the chrome process can release interal resource now. s/the/and that the/ ::: commit-message-34113:13 (Diff revision 7) > +2. The content process sends *FinalizePrint* to the chrome process by > + SendFinalizePrint to notify the chrome that there is no more print > + request, the chrome process can release interal resource now. > +3. The content process receive PageProcessed message from the chrome process. > + > +This calling sequence is fine for sync style PrintTarget, even though s/though/though the/ ::: commit-message-34113:14 (Diff revision 7) > + SendFinalizePrint to notify the chrome that there is no more print > + request, the chrome process can release interal resource now. > +3. The content process receive PageProcessed message from the chrome process. > + > +This calling sequence is fine for sync style PrintTarget, even though > +FinalizePrint message was sent out a bit too ealy, but since a sync PrintTarget s/was sent/is sent/ s/a bit too ealy/a bit early/ And wrap the "even though...early" text in parenthess rather than using a comma and the start and end. s/but since/Since/ (start a new sentence) ::: commit-message-34113:15 (Diff revision 7) > + request, the chrome process can release interal resource now. > +3. The content process receive PageProcessed message from the chrome process. > + > +This calling sequence is fine for sync style PrintTarget, even though > +FinalizePrint message was sent out a bit too ealy, but since a sync PrintTarget > +complete print task right after receiving *ProcessPage* message in #1, send s/complete print/completes its print/ s/send/sending/ ::: commit-message-34113:16 (Diff revision 7) > +3. The content process receive PageProcessed message from the chrome process. > + > +This calling sequence is fine for sync style PrintTarget, even though > +FinalizePrint message was sent out a bit too ealy, but since a sync PrintTarget > +complete print task right after receiving *ProcessPage* message in #1, send > +FinalizePrint before getting PageProcessed response is no harm. s/is no harm/is harmless/ ::: layout/printing/nsPagePrintTimer.cpp:83 (Diff revision 7) > bool donePrinting; > > // donePrinting will be true if it completed successfully or > // if the printing was cancelled > donePrinting = !mPrintEngine || mPrintEngine->PrintPage(mPrintObj, inRange); > - if (donePrinting) { > + if (donePrinting && mPrintEngine) { Can you swap these operands? I find that more readable. ::: layout/printing/nsPagePrintTimer.cpp:181 (Diff revision 7) > if (!mWaitingForRemotePrint) { > return; > } > > + // now clean up print or print the next webshell > + if (mDone && mPrintEngine) { I think you can just MOZ_ASSERT(mDone) rather than check for it here (which looks a bit weird in PrintFinished).
Attachment #8925564 - Flags: review?(jwatt) → review+
Comment on attachment 8925565 [details] Bug 1399787 - Part 13. Handle AbortDocument. https://reviewboard.mozilla.org/r/196694/#review209196
Attachment #8925565 - Flags: review?(jwatt) → review+
Comment on attachment 8930380 [details] Bug 1399787 - Part 16. Add a preference in all.js to enable/disable print a document by pdf encoder and pdfium. https://reviewboard.mozilla.org/r/201520/#review209198 MOZ_ENABLE_SKIA_PDF is on by default in Nightly so this change would/will flip all Nightly users to use this new code. Besides the issue with the patch that needs to be fixed, I think we should do this in a separate bug. ::: modules/libpref/init/all.js:302 (Diff revision 1) > > pref("browser.triple_click_selects_paragraph", true); > > +#ifdef MOZ_ENABLE_SKIA_PDF > +// Print a document by using pdf encoder. > +pref("print.print_via_pdf_encoder", false); The print.print_via_pdf_encoder pref is a string pref. You should set it to "skia-pdf". I think the comment is redundant. Just remove it.
Attachment #8930380 - 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/#review208872 There is no reason, I put them into a wrong folder. Will correct it in the next update.
Comment on attachment 8925564 [details] Bug 1399787 - Part 12. Delay dispatching FinalizePrint message until the last page was processed. https://reviewboard.mozilla.org/r/196692/#review209190 > I think you can just MOZ_ASSERT(mDone) rather than check for it here (which looks a bit weird in PrintFinished). "PrintFinished" means one page was printed, while mDone means the "print job" is done. so this function can be called while mDone is false.
Attachment #8930380 - Attachment is obsolete: true
Attachment #8932876 - Flags: review?(jwatt)
Comment on attachment 8932876 [details] Bug 1399787 - Part 17. Clean up some comments and formatting in PDFViaEMFPrintHelper code. https://reviewboard.mozilla.org/r/203906/#review209392 Thanks for doing this in a separate patch. ::: commit-message-0a80d:1 (Diff revision 1) > +Bug 1399787 - Part 17. Clean up. Make that "Clean up some comments and formatting in PDFViaEMFPrintHelper code" ::: widget/windows/PDFViaEMFPrintHelper.h:54 (Diff revision 1) > void CloseDocument(); > > int GetPageCount() const { return mPDFiumEngine->GetPageCount(mPDFDoc); } > > - /** Convert specified PDF page to EMF and draw the EMF onto the given DC. */ > + /** Convert the specified PDF page to EMF and draw the EMF onto the > + * given DC. Fix the indentation here.
Attachment #8932876 - 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/#review209396 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: gfx/thebes/PrintTargetEMF.cpp:21 (Diff revision 7) > + : PrintTarget(/* not using cairo_surface_t */ nullptr, aSize) > + , mPrinterDC(aDC) > +{ > +} > + > +PrintTargetEMF::~PrintTargetEMF() Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]
Comment on attachment 8928107 [details] Bug 1399787 - Part 11.b. Use PrintTargetEMF to print chrome documents. https://reviewboard.mozilla.org/r/199354/#review209400 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: layout/printing/nsPrintEngine.cpp:677 (Diff revision 6) > printData->mPrintDC = new nsDeviceContext(); > rv = printData->mPrintDC->InitForPrinting(devspec); > NS_ENSURE_SUCCESS(rv, rv); > > + if (XRE_IsParentProcess() && !printData->mPrintDC->IsSyncPagePrinting()) { > + printData->mPrintDC->RegisterPageDoneCallback([this](nsresult aResult) { PageDone(aResult); }); Error: Refcounted variable 'this' of type 'nsprintengine' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]
Comment on attachment 8925560 [details] Bug 1399787 - Part 6. Implement the bulk of a new PrintTargetEMF. https://reviewboard.mozilla.org/r/196684/#review209402 ::: gfx/thebes/moz.build:231 (Diff revision 7) > if CONFIG['MOZ_ENABLE_SKIA_PDF']: > EXPORTS.mozilla.gfx += [ > 'PrintTargetSkPDF.h', > ] > SOURCES += [ > + 'PrintTargetEMF.cpp', Thanks for moving this file to gfx. This needs to be indented under a check for windows: if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows': ... since this file won't build on other platforms.
Attached patch patch for ipdl review — — Splinter Review
This should get review from an IPC peer too. dvander, can you review this? It's probably easiest to review this patch rather than individual files.
Attachment #8932968 - Flags: review?(dvander)
(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #287) > Created attachment 8932968 [details] [diff] [review] > patch for ipdl review > > This should get review from an IPC peer too. dvander, can you review this? > It's probably easiest to review this patch rather than individual files. you may start from this file * PPDFium.ipdl * PDFiumParent and PDFiumChild (actor) * PDFiumProcessParent and PDFiumProcessChild (process)
I'm guessing we probably want crash reporting to work for this new process - if so, please file a follow-up bug. It's not too much work. The boilerplate is in nsICrashService.idl, CrashManager.jsm, CrashReporterHost.cpp, and CrashService.js. You just need to add new entries to the enum/switch cases in those files (search for "GPU" in those files and you'll find all the places). Then in addition, the top-level protocol endpoints (PDFiumChild, PDFiumParent) would have CrashReporterHost and CrashReporterClient instances, as ContentChild/ContentParent/GMPChild/GMPParent etc do. Other things to consider: telemetry reporting, memory reporting. If you are interested in getting those working I can run through what the GPU process does.
Blocks: 1421862
(In reply to David Anderson [:dvander] from comment #289) > I'm guessing we probably want crash reporting to work for this new process - > if so, please file a follow-up bug. It's not too much work. The boilerplate > is in nsICrashService.idl, CrashManager.jsm, CrashReporterHost.cpp, and > CrashService.js. You just need to add new entries to the enum/switch cases > in those files (search for "GPU" in those files and you'll find all the > places). Bug 1421862 filed. > Then in addition, the top-level protocol endpoints (PDFiumChild, > PDFiumParent) would have CrashReporterHost and CrashReporterClient > instances, as ContentChild/ContentParent/GMPChild/GMPParent etc do. > > Other things to consider: telemetry reporting, memory reporting. If you are > interested in getting those working I can run through what the GPU process > does. Yes, it's helpful if you can give a brief introduction.
Attachment #8933210 - Flags: review?(jwatt)
(In reply to David Anderson [:dvander] from comment #289) > Other things to consider: telemetry reporting, memory reporting. Telemetry reporting may come in useful. As for memory reporting, for now at least the PDFium processes will be short lived, living only for the length of time that it takes to print a single document. While it would be useful to be informed if the PDFium process were to leak or start consuming "too much" memory, integration into about:memory probably wouldn't be all that useful. At any rate, we're now approaching 300 comments on this bug. I think these things should be follow-up bugs. C.J., can you file bugs and needinfo dvander there?
(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #313) > (In reply to David Anderson [:dvander] from comment #289) > > Other things to consider: telemetry reporting, memory reporting. > > Telemetry reporting may come in useful. As for memory reporting, for now at > least the PDFium processes will be short lived, living only for the length > of time that it takes to print a single document. While it would be useful > to be informed if the PDFium process were to leak or start consuming "too > much" memory, integration into about:memory probably wouldn't be all that > useful. > > At any rate, we're now approaching 300 comments on this bug. I think these > things should be follow-up bugs. C.J., can you file bugs and needinfo > dvander there? Done. Bug 1421862
Comment on attachment 8932968 [details] [diff] [review] patch for ipdl review Review of attachment 8932968 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/PDFiumParent.cpp @@ +16,5 @@ > +} > + > +PDFiumParent::~PDFiumParent() > +{ > + Close(); Since the object is refcounted, we generally like to let IPDL own a ref. Otherwise it's harder to guarantee the channel is closed. The recommended pattern is to not Close() here. Instead, call AddRef in Init(), and then override DeallocPPDFiumParent and call Release() in there. When the IPC channel closes for any reason, IPDL will release its ref. Then a forced call to Close() in PDFiumProcessParent will ensure the channel is shut down. @@ +25,5 @@ > +{ > + if (NS_WARN_IF(!Open(aChannel, aPid))) { > + return false; > + } > + This is where you want the AddRef() call. ::: widget/windows/PDFiumProcessParent.cpp @@ +39,5 @@ > + > + // Open the top level protocol for PDFium process. > + MOZ_ASSERT(!mPDFiumParentActor); > + mPDFiumParentActor = new PDFiumParent(aTarget); > + return mPDFiumParentActor->Init(GetChannel(), A note: If the thread here is not necessarily the main thread, then the (top-level) actor in question must be shut down before the thread is deleted. IPDL doesn't automatically ensure this, yet. @@ +52,5 @@ > + // EMF conversion response. > + mPDFiumParentActor->AbortConversion([this]() { Delete(false); }); > + return; > + } > + Please stick a call to mPDFiumParentActor->Close() here. (It's okay if it somehow gets called twice).
Attachment #8932968 - Flags: review?(dvander) → review+
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/#review209802 ::: commit-message-68590:3 (Diff revision 6) > +Bug 1399787 - Part 2.a. Add functions to load the content of a PDF from a FileDescriptor. > + > +All the functions that added in Part 2 are utilities for sharing EMF/PDF s/that// ::: widget/windows/PDFViaEMFPrintHelper.cpp:96 (Diff revision 6) > + auto rawFD = aFD.ClonePlatformHandle(); > + PRFileDesc* prfile = PR_ImportFile(PROsfd(rawFD.release())); > + NS_ENSURE_TRUE(prfile, NS_ERROR_FAILURE); > + > + mPDFDoc = mPDFiumEngine->LoadDocument(prfile, nullptr); > + NS_ENSURE_TRUE(mPDFDoc, NS_ERROR_FAILURE); We need to call PR_Close(prfile) here in the error condition.
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/#review209854 ::: widget/windows/PDFViaEMFPrintHelper.cpp:181 (Diff revision 6) > result = emf.Playback(aPrinterDC, printRect); > return result; > } > > bool > -PDFViaEMFPrintHelper::DrawPageToFile(const wchar_t* aFilePath, > +PDFViaEMFPrintHelper::SavePageToFile(const wchar_t* aFilePath, FWIW this rename should be in part c. ::: widget/windows/WindowsEMF.h:47 (Diff revision 6) > * GetDC() to obtain an HDC to modify the file. They can only use Playback(). > */ > bool InitFromFileContents(const wchar_t* aMetafilePath); > > /** > + * creates the EMF from the specified data s/creates/Creates/ ::: widget/windows/WindowsEMF.cpp:102 (Diff revision 6) > } > > +UINT > +WindowsEMF::GetEMFContentSize() > +{ > + if (!FinishDocument()) { Can't we MOZ_ASSERT this? It seems like it would be an error in the code somewhere if we're calling this too early. It would be good to either have the assert, or a comment explaining why this can happen, if it can. ::: widget/windows/WindowsEMF.cpp:112 (Diff revision 6) > +} > + > +bool > +WindowsEMF::GetEMFContentBits(LPBYTE aBytes) > +{ > + if (!FinishDocument()) { Same here.
Comment on attachment 8919759 [details] Bug 1399787 - Part 5. Implement the PDFium process. https://reviewboard.mozilla.org/r/190696/#review210408 ::: toolkit/xre/nsAppRunner.cpp:903 (Diff revision 13) > SYNC_ENUMS(DEFAULT, Default) > SYNC_ENUMS(PLUGIN, Plugin) > SYNC_ENUMS(CONTENT, Content) > SYNC_ENUMS(IPDLUNITTEST, IPDLUnitTest) > SYNC_ENUMS(GMPLUGIN, GMPlugin) > SYNC_ENUMS(GPU, GPU) Shouldn't there be a SYNC_ENUMS for pdfium? If not, add a comment noting why.
Comment on attachment 8925561 [details] Bug 1399787 - Part 7. Have PrintTargetEMF launch the PDFium process. https://reviewboard.mozilla.org/r/196686/#review210410 ::: gfx/thebes/PrintTargetEMF.h:14 (Diff revision 8) > #include "PrintTargetSkPDF.h" > > /* include windows.h for the HDC definitions that we need. */ > #include <windows.h> > > -namespace mozilla { > + namespace mozilla { This first line shouldn't be indented. In fact we usually don't indent the namespace declarations that just exist to wrap type declarations.
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/#review210412 ::: widget/windows/PDFiumParent.cpp:14 (Diff revision 9) > > namespace mozilla { > namespace widget { > > -PDFiumParent::PDFiumParent() > +PDFiumParent::PDFiumParent(PrintTargetEMF* aTarget) > + : mTarget(aTarget) Indent one more space.
Comment on attachment 8925563 [details] Bug 1399787 - Part 11.a. Use PrintTargetEMF to print content documents. https://reviewboard.mozilla.org/r/196690/#review210414 ::: gfx/thebes/PrintTarget.h:13 (Diff revision 9) > > #include "mozilla/RefPtr.h" > #include "mozilla/gfx/2D.h" > #include "nsISupportsImpl.h" > #include "nsStringFwd.h" > +#include <functional> As described in the style guide, this should come before the normal headers (with a blank line between it and the normal headers).
Comment on attachment 8925566 [details] Bug 1399787 - Part 15. Detect and handle breakage of the IPC channel. https://reviewboard.mozilla.org/r/196696/#review210420 ::: commit-message-95629:1 (Diff revision 9) > +Bug 1399787 - Part 15. Handle ipc channel broken. "Detect and handle breakage of the IPC channel." ::: gfx/thebes/PrintTargetEMF.h:59 (Diff revision 9) > already_AddRefed<DrawTarget> > GetReferenceDrawTarget(DrawEventRecorder* aRecorder) final; > > void ConvertToEMFDone(const nsresult& aResult, mozilla::ipc::Shmem&& aEMF); > bool IsSyncPagePrinting() const final { return false; } > + void ChannelBroken() { mChannelBroken = true; } You want ==, not = Also call this ChannelIsBroken.
Attachment #8925566 - 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/#review210416 ::: widget/windows/PDFiumEngineShim.cpp:40 (Diff revision 9) > static PDFiumEngineShim* sPDFiumEngineShim; > > +struct PDFFunctionPointerTable > +{ > + PDFFunctionPointerTable() > + : mFPDF_InitLibrary(nullptr) The initialization lines should be indented.
(In reply to Static Analysis Bot [:clangbot] from comment #284) > Comment on attachment 8928107 [details] > Bug 1399787 - Part 11.b. Use PrintTargetEMF to print chrome documents. > > https://reviewboard.mozilla.org/r/199354/#review209400 > > > C/C++ static analysis found 1 defect in this patch. > > You can run this analysis locally with: `./mach static-analysis check > path/to/file.cpp` > > > ::: layout/printing/nsPrintEngine.cpp:677 > (Diff revision 6) > > printData->mPrintDC = new nsDeviceContext(); > > rv = printData->mPrintDC->InitForPrinting(devspec); > > NS_ENSURE_SUCCESS(rv, rv); > > > > + if (XRE_IsParentProcess() && !printData->mPrintDC->IsSyncPagePrinting()) { > > + printData->mPrintDC->RegisterPageDoneCallback([this](nsresult aResult) { PageDone(aResult); }); > > Error: Refcounted variable 'this' of type 'nsprintengine' cannot be captured > by a lambda [clang-tidy: mozilla-refcounted-inside-lambda] It seems using std::bind hid this issue. The issue is real, and it needs to be addressed, but let's get these patches landed so you don't get bitrotted again, and file a follow-up bug for that. You're going to need to switch back to using std::bind temporarily to get this past the static analysis bot. As well as dvander's comments, also make sure you address the other static analysis comment in comment 283.
Comment on attachment 8933210 [details] Bug 1399787 - Part 11.c. Add assertions to make sure no page-done callback from the PrintTarget after the print job done. https://reviewboard.mozilla.org/r/204156/#review211394 Sorry, missed this one.
Attachment #8933210 - Flags: review?(jwatt) → review+
> > ::: layout/printing/nsPrintEngine.cpp:677 > > (Diff revision 6) > > > printData->mPrintDC = new nsDeviceContext(); > > > rv = printData->mPrintDC->InitForPrinting(devspec); > > > NS_ENSURE_SUCCESS(rv, rv); > > > > > > + if (XRE_IsParentProcess() && !printData->mPrintDC->IsSyncPagePrinting()) { > > > + printData->mPrintDC->RegisterPageDoneCallback([this](nsresult aResult) { PageDone(aResult); }); > > > > Error: Refcounted variable 'this' of type 'nsprintengine' cannot be captured > > by a lambda [clang-tidy: mozilla-refcounted-inside-lambda] > > It seems using std::bind hid this issue. The issue is real, and it needs to > be addressed, but let's get these patches landed so you don't get bitrotted > again, and file a follow-up bug for that. You're going to need to switch > back to using std::bind temporarily to get this past the static analysis bot. I fixed that warning by passing refptr(this), rather than this, into lambda context: RefPtr<nsPrintEngine> self(this); printData->mPrintDC->RegisterPageDoneCallback([self](nsresult aResult) { self->PageDone(aResult); }); > As well as dvander's comments, also make sure you address the other static > analysis comment in comment 283. Done
Comment on attachment 8925566 [details] Bug 1399787 - Part 15. Detect and handle breakage of the IPC channel. https://reviewboard.mozilla.org/r/196696/#review210420 > You want ==, not = > > Also call this ChannelIsBroken. ChannelBroken() is used to notify PrintTargetEMG that the IPC channel had been closed. So, I do need '=' here. Rename to ChannelIsBroken is ok.
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d56497aa5390 Part 1. Fix namespacing and include issues hidden by unified compilation. r=jwatt https://hg.mozilla.org/integration/autoland/rev/98758e79710d Part 2.a. Add functions to load the content of a PDF from a FileDescriptor. r=jwatt https://hg.mozilla.org/integration/autoland/rev/364b5b44932b Part 2.b. Add functions to read/write EMF content from/to a buffer. r=jwatt https://hg.mozilla.org/integration/autoland/rev/c385d0f60f8a Part 2.c. Rename DrawPageToFile to SavePageToFile. r=jwatt https://hg.mozilla.org/integration/autoland/rev/185368267354 Part 3. Create a top level protocol for the PDFium process. r=jwatt https://hg.mozilla.org/integration/autoland/rev/d165846cb5e1 Part 4. Take out the code we landed in bug 1370488. r=jwatt https://hg.mozilla.org/integration/autoland/rev/bc52b1781641 Part 5. Implement the PDFium process. r=jwatt https://hg.mozilla.org/integration/autoland/rev/cb3ae1dc20b2 Part 6. Implement the bulk of a new PrintTargetEMF. r=jwatt https://hg.mozilla.org/integration/autoland/rev/ff1436ae1ef7 Part 7. Have PrintTargetEMF launch the PDFium process. r=jwatt https://hg.mozilla.org/integration/autoland/rev/682c60e52749 Part 8. Open PDFium protocol channel. r=jwatt https://hg.mozilla.org/integration/autoland/rev/67e530c129c7 Part 9. Sandbox the PDFium process. r=bobowen,jwatt https://hg.mozilla.org/integration/autoland/rev/5c50bbde0950 Part 10. Make PrintTargetEMF use the PDFium process to convert to EMF. r=jwatt https://hg.mozilla.org/integration/autoland/rev/93f9a5e253d8 Part 11.a. Use PrintTargetEMF to print content documents. r=jwatt https://hg.mozilla.org/integration/autoland/rev/2382a348a6c1 Part 11.b. Use PrintTargetEMF to print chrome documents. r=jwatt https://hg.mozilla.org/integration/autoland/rev/0b75ef19e695 Part 11.c. Add assertions to make sure no page-done callback from the PrintTarget after the print job done. r=jwatt https://hg.mozilla.org/integration/autoland/rev/c995f4e18724 Part 11.d. Using PrintTargetEMF on windows if skia-pdf is enable. r=jwatt https://hg.mozilla.org/integration/autoland/rev/c478fb75f5cb Part 12. Delay dispatching FinalizePrint message until the last page was processed. r=jwatt https://hg.mozilla.org/integration/autoland/rev/8f600ac930ec Part 13. Handle AbortDocument. r=jwatt https://hg.mozilla.org/integration/autoland/rev/d875e45f591e Part 14. Prevent RemotePrintJobChild using ipc calls after the channel was destroyed. r=jwatt https://hg.mozilla.org/integration/autoland/rev/b1457eabd34e Part 15. Detect and handle breakage of the IPC channel. r=jwatt https://hg.mozilla.org/integration/autoland/rev/80c062fd58fb Part 16. Hide function table in PDFiumEngineShim.cpp. r=jwatt https://hg.mozilla.org/integration/autoland/rev/0afbd07d8219 Part 17. Clean up some comments and formatting in PDFViaEMFPrintHelper code. r=jwatt
Backout by btara@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f0afedbc68cf Backed out 22 changesets for failing on mozmake.EXE r=backout a=backout on a CLOSED TREE
Backed out 22 changesets (bug 1399787) 15:55:10 INFO - z:/build/build/src/widget/windows/PDFiumChild.cpp(42,54): error: non-const lvalue reference to type 'mozilla::widget::PPDFiumChild::Shmem' (aka 'mozilla::ipc::Shmem') cannot bind to a temporary of type 'ipc::Shmem' It seems clang-cl was used instead of msvc. https://treeherder.mozilla.org/logviewer.html#?job_id=150524664&repo=autoland&lineNumber=28245 https://hg.mozilla.org/integration/autoland/rev/f0afedbc68cf017c206d0d6706cfdff0b44c66ca https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0afbd07d821916af688227ad7144639b33d487c4&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-searchStr=static This has generated browser chrome failures and mingw32 bustage. Please do a Try push to test your changes.
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6d82ed0ba805 Part 1. Fix namespacing and include issues hidden by unified compilation. r=jwatt https://hg.mozilla.org/integration/autoland/rev/baa99fb50ba9 Part 2.a. Add functions to load the content of a PDF from a FileDescriptor. r=jwatt https://hg.mozilla.org/integration/autoland/rev/558526301a4c Part 2.b. Add functions to read/write EMF content from/to a buffer. r=jwatt https://hg.mozilla.org/integration/autoland/rev/a47bd86c35ee Part 2.c. Rename DrawPageToFile to SavePageToFile. r=jwatt https://hg.mozilla.org/integration/autoland/rev/3b89f189edff Part 3. Create a top level protocol for the PDFium process. r=jwatt https://hg.mozilla.org/integration/autoland/rev/323da3bddaaa Part 4. Take out the code we landed in bug 1370488. r=jwatt https://hg.mozilla.org/integration/autoland/rev/c186893ce0fc Part 5. Implement the PDFium process. r=jwatt https://hg.mozilla.org/integration/autoland/rev/ef21f295db3f Part 6. Implement the bulk of a new PrintTargetEMF. r=jwatt https://hg.mozilla.org/integration/autoland/rev/3fd2a734f887 Part 7. Have PrintTargetEMF launch the PDFium process. r=jwatt https://hg.mozilla.org/integration/autoland/rev/e7bfa51404c5 Part 8. Open PDFium protocol channel. r=jwatt https://hg.mozilla.org/integration/autoland/rev/c2ab1b454283 Part 9. Sandbox the PDFium process. r=bobowen,jwatt https://hg.mozilla.org/integration/autoland/rev/01284c55bf8a Part 10. Make PrintTargetEMF use the PDFium process to convert to EMF. r=jwatt https://hg.mozilla.org/integration/autoland/rev/f9fd3e750636 Part 11.a. Use PrintTargetEMF to print content documents. r=jwatt https://hg.mozilla.org/integration/autoland/rev/09b3c172a01e Part 11.b. Use PrintTargetEMF to print chrome documents. r=jwatt https://hg.mozilla.org/integration/autoland/rev/9dfa404abf9d Part 11.c. Add assertions to make sure no page-done callback from the PrintTarget after the print job done. r=jwatt https://hg.mozilla.org/integration/autoland/rev/2800f9d20d96 Part 11.d. Using PrintTargetEMF on windows if skia-pdf is enable. r=jwatt https://hg.mozilla.org/integration/autoland/rev/a7d70f7f3335 Part 12. Delay dispatching FinalizePrint message until the last page was processed. r=jwatt https://hg.mozilla.org/integration/autoland/rev/d7fef200e8b9 Part 13. Handle AbortDocument. r=jwatt https://hg.mozilla.org/integration/autoland/rev/e82ab72f71ee Part 14. Prevent RemotePrintJobChild using ipc calls after the channel was destroyed. r=jwatt https://hg.mozilla.org/integration/autoland/rev/2e91a90dfbc3 Part 15. Detect and handle breakage of the IPC channel. r=jwatt https://hg.mozilla.org/integration/autoland/rev/fc9776a2605d Part 16. Hide function table in PDFiumEngineShim.cpp. r=jwatt https://hg.mozilla.org/integration/autoland/rev/463d676df5da Part 17. Clean up some comments and formatting in PDFViaEMFPrintHelper code. r=jwatt
Flags: needinfo?(cku)
Checking.. I can repro timeout issue in comment 383 Can not repro the crash in comment 384. I do not expect PrintTargetEMF been created in any test case since we do not set "print.print_via_pdf_encoder" as "skia-pdf" yet.
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b6b78f4aa40f Part 1. Fix namespacing and include issues hidden by unified compilation. r=jwatt https://hg.mozilla.org/integration/autoland/rev/f0df14d5edab Part 2.a. Add functions to load the content of a PDF from a FileDescriptor. r=jwatt https://hg.mozilla.org/integration/autoland/rev/6354e568837e Part 2.b. Add functions to read/write EMF content from/to a buffer. r=jwatt https://hg.mozilla.org/integration/autoland/rev/83c6e818f150 Part 2.c. Rename DrawPageToFile to SavePageToFile. r=jwatt https://hg.mozilla.org/integration/autoland/rev/2d11412ee4a5 Part 3. Create a top level protocol for the PDFium process. r=jwatt https://hg.mozilla.org/integration/autoland/rev/dd16ce21aec6 Part 4. Take out the code we landed in bug 1370488. r=jwatt https://hg.mozilla.org/integration/autoland/rev/9211d19c1075 Part 5. Implement the PDFium process. r=jwatt https://hg.mozilla.org/integration/autoland/rev/b95127eb91d8 Part 6. Implement the bulk of a new PrintTargetEMF. r=jwatt https://hg.mozilla.org/integration/autoland/rev/5006b383bddb Part 7. Have PrintTargetEMF launch the PDFium process. r=jwatt https://hg.mozilla.org/integration/autoland/rev/1543f8cb7898 Part 8. Open PDFium protocol channel. r=jwatt https://hg.mozilla.org/integration/autoland/rev/24815dc40248 Part 9. Sandbox the PDFium process. r=bobowen,jwatt https://hg.mozilla.org/integration/autoland/rev/bb67ef211bf2 Part 10. Make PrintTargetEMF use the PDFium process to convert to EMF. r=jwatt https://hg.mozilla.org/integration/autoland/rev/716a2077fc0c Part 11.a. Use PrintTargetEMF to print content documents. r=jwatt https://hg.mozilla.org/integration/autoland/rev/72a4344a4197 Part 11.b. Use PrintTargetEMF to print chrome documents. r=jwatt https://hg.mozilla.org/integration/autoland/rev/e38c56824a75 Part 11.c. Add assertions to make sure no page-done callback from the PrintTarget after the print job done. r=jwatt https://hg.mozilla.org/integration/autoland/rev/a3362670656e Part 11.d. Using PrintTargetEMF on windows if skia-pdf is enable. r=jwatt https://hg.mozilla.org/integration/autoland/rev/fba1b5a37709 Part 12. Delay dispatching FinalizePrint message until the last page was processed. r=jwatt https://hg.mozilla.org/integration/autoland/rev/9c978a1b917e Part 13. Handle AbortDocument. r=jwatt https://hg.mozilla.org/integration/autoland/rev/8b5b57e746e6 Part 14. Prevent RemotePrintJobChild using ipc calls after the channel was destroyed. r=jwatt https://hg.mozilla.org/integration/autoland/rev/25f83fa43020 Part 15. Detect and handle breakage of the IPC channel. r=jwatt https://hg.mozilla.org/integration/autoland/rev/810b268d3044 Part 16. Hide function table in PDFiumEngineShim.cpp. r=jwatt https://hg.mozilla.org/integration/autoland/rev/972a3efac0d7 Part 17. Clean up some comments and formatting in PDFViaEMFPrintHelper code. r=jwatt
Flags: needinfo?(cku)
Depends on: 1424922
Depends on: 1426523
Depends on: 1425895
Depends on: 1426880
Depends on: 1427109
Depends on: 1427381
Depends on: 1427380
Depends on: 1427378
No longer depends on: 1427380
Depends on: 1436460
Depends on: 1436519
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: