Status
()
People
(Reporter: fatseng, Assigned: u459114)
Tracking
(Blocks 1 bug)
Firefox Tracking Flags
(firefox59 fixed)
Details
Attachments
(23 attachments, 17 obsolete attachments)
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
bobowen
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
78.98 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
No description provided.
|
Reporter | |
Updated•2 years ago
|
OS: Unspecified → Windows
|
|
Reporter | |
Comment 1•2 years ago
|
||
Upload a WIP patch to create a sandboxed process.
|
||
Comment 2•2 years ago
|
||
LGTM. Please land after 9/21.
|
|
Reporter | |
Updated•2 years ago
|
Attachment #8908137 -
Attachment description: [WIP] create a sandboxed process → [WIP] Part1 create a sandboxed process
|
|
Reporter | |
Comment 3•2 years ago
|
||
Setup an IPC between Chrome and a sandboxed process.
|
|
Reporter | |
Comment 4•2 years ago
|
||
Convert PDF to EMF in the sandboxed process, after that send the EMF to the Chrome process for printing.
|
|
Reporter | |
Updated•2 years ago
|
Attachment #8912579 -
Attachment description: Part3 print PDF → [WIP]Part3 print PDF
|
|
Reporter | |
Updated•2 years ago
|
Attachment #8912579 -
Attachment description: [WIP]Part3 print PDF → [WIP] Part3 print PDF
|
|
Reporter | |
Comment 5•2 years ago
|
||
Attachment #8910586 -
Attachment is obsolete: true
|
|
Reporter | |
Comment 6•2 years ago
|
||
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
Attachment #8915526 -
Attachment is obsolete: true
Attachment #8918182 -
Attachment is obsolete: true
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
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)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
Attachment #8920985 -
Flags: review?(jwatt)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 44•2 years ago
|
||
| mozreview-review | ||
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+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
Attachment #8921859 -
Flags: review?(jwatt)
|
||
Comment 58•2 years ago
|
||
| mozreview-review | ||
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 59•2 years ago
|
||
| mozreview-review | ||
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)
|
|
||
Updated•2 years ago
|
Attachment #8919758 -
Flags: feedback+
|
|
||
Comment 60•2 years ago
|
||
| mozreview-review | ||
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)
|
|
||
Updated•2 years ago
|
Attachment #8919759 -
Flags: feedback+
|
|
||
Comment 61•2 years ago
|
||
| mozreview-review | ||
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 62•2 years ago
|
||
| mozreview-review | ||
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)
|
|
||
Comment 63•2 years ago
|
||
Needinfo'ing Bob so that at the very least he's aware of what's going on here.
Flags: needinfo?(bobowencode)
|
Assignee | |
Comment 64•2 years ago
|
||
| mozreview-review-reply | ||
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)
|
|
Assignee | |
Comment 65•2 years ago
|
||
| mozreview-review-reply | ||
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.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 77•2 years ago
|
||
(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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 87•2 years ago
|
||
| mozreview-review | ||
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)
|
|
||
Updated•2 years ago
|
Flags: needinfo?(bobowencode)
|
|
||
Comment 88•2 years ago
|
||
| mozreview-review | ||
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.
|
|
Assignee | |
Comment 89•2 years ago
|
||
| mozreview-review-reply | ||
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....
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 92•2 years ago
|
||
| mozreview-review-reply | ||
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 93•2 years ago
|
||
| mozreview-review | ||
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 94•2 years ago
|
||
| mozreview-review | ||
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?
|
|
Assignee | |
Comment 95•2 years ago
|
||
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.
|
|
||
Comment 96•2 years ago
|
||
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)
|
|
||
Comment 97•2 years ago
|
||
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.
|
|
Assignee | |
Comment 98•2 years ago
|
||
(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)
|
|
Assignee | |
Comment 99•2 years ago
|
||
(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.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
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 118•2 years ago
|
||
| mozreview-review | ||
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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 136•2 years ago
|
||
| mozreview-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/#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 137•2 years ago
|
||
| mozreview-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 138•2 years ago
|
||
| mozreview-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 139•2 years ago
|
||
| mozreview-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 140•2 years ago
|
||
| mozreview-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 141•2 years ago
|
||
| mozreview-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-
|
|
Assignee | |
Comment 142•2 years ago
|
||
| mozreview-review-reply | ||
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
|
|
Assignee | |
Comment 143•2 years ago
|
||
| mozreview-review-reply | ||
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()
|
|
Assignee | |
Comment 144•2 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
Assignee | |
Comment 145•2 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
||
Comment 146•2 years ago
|
||
(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.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
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 167•2 years ago
|
||
| mozreview-review | ||
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)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
Attachment #8924137 -
Attachment is obsolete: true
Attachment #8915856 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 178•2 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78dcb27b3ff83ead5beb4498cdee8524e430788f
|
|
||
Comment 179•2 years ago
|
||
| mozreview-review | ||
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.
|
|
Assignee | |
Comment 180•2 years ago
|
||
| mozreview-review-reply | ||
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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 195•2 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65f43e47558bb7bfde41c50db19a328f0580ee82
|
|
||
Comment 196•2 years ago
|
||
| mozreview-review | ||
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+
|
|
||
Updated•2 years ago
|
Attachment #8908137 -
Attachment is obsolete: true
|
|
||
Updated•2 years ago
|
Attachment #8912579 -
Attachment is obsolete: true
|
|
||
Updated•2 years ago
|
Attachment #8914207 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 197•2 years ago
|
||
| mozreview-review | ||
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 198•2 years ago
|
||
| mozreview-review | ||
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 199•2 years ago
|
||
| mozreview-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 200•2 years ago
|
||
| mozreview-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 201•2 years ago
|
||
| mozreview-review | ||
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 202•2 years ago
|
||
| mozreview-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 203•2 years ago
|
||
| mozreview-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+
|
|
Assignee | |
Comment 204•2 years ago
|
||
| mozreview-review-reply | ||
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.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
Attachment #8925560 -
Flags: review?(jwatt)
Attachment #8930380 -
Flags: review?(jwatt)
|
||
Updated•2 years ago
|
|
|
||
Comment 239•2 years ago
|
||
| mozreview-review | ||
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 us
Description
•