1414834 - Re-land print IPC changes so that the child doesn't open files

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P1)

Description

[Alex Gaynor [:Alex_Gaynor]]

Specifically, re-land part 3 of bug 1319423. It was reverted in bug 1412643.

Relanding this will include changes to print selection so that we don't cause the bug which was reported there.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1414834 - Part 1 - made print selection go through the print timer;

When printing a selection, we treat all pages in a selection as a single 'page'
internally. Before this patch we handled this in a single iteration, thereby
bypassing the control flow normally applied to the IPC. With this patch, we
don't attempt to print additional pages until IPC related to the former page has
completed.

Review commit: https://reviewboard.mozilla.org/r/196660/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196660/

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1414834 - re-land print IPC changes from bug 1319423;

With this patch print IPC no longer requires the child process to create files.
Instead files are opened by the parent and a file descriptor is sent to the
child. This will allow us to sandbox child processes more aggressively.

Review commit: https://reviewboard.mozilla.org/r/196662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196662/

Comment 3

[Bob Owen (:bobowen)]

I plan to get onto this tomorrow.

Comment 4

[Bob Owen (:bobowen)]

Comment on attachment 8925540 [details]
Bug 1414834 - re-land print IPC changes from bug 1319423;

https://reviewboard.mozilla.org/r/196662/#review203716

::: layout/printing/ipc/PRemotePrintJob.ipdl:52
(Diff revision 1)
>    // Inform the child that the print has been initialized in the parent or has
> -  // failed with result aRv.
> -  async PrintInitializationResult(nsresult aRv);
> -
> -  // Inform the child that the latest page has been processed remotely.
> -  async PageProcessed();
> +  // failed with result aRv. Includes a file descriptor which the first page
> +  // can be written to.
> +  async PrintInitializationResult(nsresult aRv, FileDescriptor aFd);
> +
> +  // Inform the child that the latest page has been processed remotely. Inclues

nit: s/Inclues/Includes/

::: layout/printing/ipc/RemotePrintJobChild.cpp:51
(Diff revision 1)
> +    SetNextPageFD(aFd);
> +  }
>    return IPC_OK();
>  }
>  
> +PRFileDesc* RemotePrintJobChild::GetNextPageFD() {

nit: return type on previous line, curly bracket on following line.

::: layout/printing/ipc/RemotePrintJobParent.cpp:94
(Diff revision 1)
>    }
>  
>    return NS_OK;
>  }
>  
> +nsresult RemotePrintJobParent::PrepareNextPageFD(FileDescriptor* aFd) {

nit: return type on previous line, curly bracket on following line.

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8925539 [details]
Bug 1414834 - Part 1 - made print selection go through the print timer;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196660/diff/1-2/

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8925540 [details]
Bug 1414834 - re-land print IPC changes from bug 1319423;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196662/diff/1-2/

Comment 7

[Bob Owen (:bobowen)]

Comment on attachment 8925539 [details]
Bug 1414834 - Part 1 - made print selection go through the print timer;

https://reviewboard.mozilla.org/r/196660/#review203758

I've filed bug 1419739 to handle this separately as I'm not sure how well mozreview will play with a different patch.

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8925540 [details]
Bug 1414834 - re-land print IPC changes from bug 1319423;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196662/diff/2-3/

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8925540 [details]
Bug 1414834 - re-land print IPC changes from bug 1319423;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196662/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87f5eb24ef4b
re-land print IPC changes from bug 1319423; r=bobowen

Comment 11

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/87f5eb24ef4b