1749598 - Stop synchronizing the print_printer preference to content processes

Status: RESOLVED FIXED

(Toolkit :: Printing, defect, P1)

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

The print_printer preferences contain personal user information (printer name/model) and are synced to the content process. We need to investigate why these are sent to the content process, and if they are not required - stop them from being sent. If they are required to be there, we should document why and what would be needed to remove them so we can triage that effort.

Comment 1

[Jonathan Watt [:jwatt]]

Some resent patches mean that we no longer need this pref in the content process.

However, this raises another question: we also pass the printer name and any save-pdf-to-file [absolute] path to the content process in nsIPrintSettings's printerName and toFileName properties, respectively:

https://searchfox.org/mozilla-central/rev/83e67336083df9f9a3d1e0c33f2ba19703d57161/widget/nsIPrintSettings.idl#301,304

If we want to stop doing that we should do that work in a separate bug since making changes there will be more involved and take a bit more time.

Comment 2

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

It's been a little hard for me to trace - I got to PrintData being passed across in the ipdl's which contains those fields, and then to DeserializeToPrintSettings - but I couldn't quite follow it from there.

The main question about those will be: How long-lived are those? If they exist for only code execution, and aren't stored long-term and ideally not even in callbacks - then this isn't a concern. If they enter the content process based on user interaction and and then live only for a shortish time period - that's not a big deal either. But if they enter without user interaction, or they live permanently in the content process after they are sent, that would be something we would ideally improve.

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #2)

It's been a little hard for me to trace - I got to PrintData being passed across in the ipdl's which contains those fields, and then to DeserializeToPrintSettings - but I couldn't quite follow it from there.

They can end up stored in a few places, which I can go into if necessary, but...

The main question about those will be: How long-lived are those?

For the default values of the applicable prefs, they will exist in the content process from when the print is initiated - which could be due to window.print() - until the print preview window is closed (either until the user cancels the print/preview, or until after the print has been completely sent to the printer, if the user opts to print).

But if they enter without user interaction, or they live permanently in the content process after they are sent, that would be something we would ideally improve.

So it sounds like we should open a bug for this then.

Comment 4

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Yes please.

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Mark/:jwatt,
I'm suggesting the Priority as P1 to indicate the impact, importance and urgency for this bug, as it's a blocker of removing jit mitigations. Please feel free to let me know if you have any questions.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #1)

Some resent patches mean that we no longer need this pref in the content process.

Actually I missed one way in which we can read this pref in the content process:

In nsGlobalWindowOuter::Print we call GetDefaultPrintSettingsForPrinting (which can read the pref) if aPrintSettings is null, which happens when this function is invoked by a window.print() call. The question is, why do we even need this settings object, given that in the window.print() case we only do the static clone when invoked by window.print(), and we avoid doing any pagination/layout until later when we have a valid nsIPrintSettings object to do it with. Strangely, the thing we're doing with the settings object is passing it to the CreateStaticClone call. You would hope that a print settings object would be completely unnecessary for the clone, but it turns out that, since D119103, we've been calling UpdateRemotePrintSettings in BrowserChild::RecvCloneDocumentTreeIntoSelf. In other words, unfortunately there's not currently a strong separation between cloning and the subsequent layout for window.print() when it comes to any cross-origin iframes (as there is for those two operations in the originating nsGlobalWindowOuter::Print call, as described above).

That's as far as I've got with this so far. Maybe it's fairly straightforward to change the code to have cloning and layout done in two distinct steps for cross-origin iframes. (That would be ideal.) In that case CreateStaticClone wouldn't need an nsIPrintSettings object to be passed, and we wouldn't need the GetDefaultPrintSettingsForPrinting call mentioned above.

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #6)

Maybe it's fairly straightforward to change the code to have cloning and layout done in two distinct steps for cross-origin iframes. (That would be ideal.)

I opened bug 1776169 for that but will use a different approach to fix this bug.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: Bug 1749598 p1 - Stop reading the last used printer name in content processes. r=emilio

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: Bug 1749598 p2 - Stop synchronizing the print_printer preference to content processes. r=emilio

Depends on D150098

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4936dc5cc82d
p1 - Stop reading the last used printer name in content processes. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a11deab620cd
p2 - Stop synchronizing the print_printer preference to content processes. r=emilio

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4936dc5cc82d
https://hg.mozilla.org/mozilla-central/rev/a11deab620cd