1238964 - Printing via parent doesn't handle paper sizes correctly.

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

This was found when I landed bug 1156742, so printing via parent had to be disabled.

The paper size wasn't getting passed down to the content process as I thought.
By chance the profiles I'd used for testing worked.

I've broken this out into a separate bug so I can land the fix separately for testing first.
Then I'll re-land the default print via parent pref in bug 115742, which got reopened.

I have a patch, but I want to do some extra tidying in some related code as well.

Comment 1

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=047ffd2e8afc

Comment 2

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4be8264a1c9c

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 1: Hold new printable page sizes in print nsIPrintSettingsWin

This also holds the resolution in the print settings, so that we can start to
remove the access to the native Windows print devices in the child process.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 2: Move separate DEVMODE to nsIPrintSettings copying into nsPrintSettingsWin

This also corrects what I think are long standing issues with the mapping to and
from print settings on Windows.
Firstly it only uses the DEVMODE flags to decide what should get stored, in the
old code if paper size was not set, it would then use that possibly invalid
paper size to map to length and width. Paper setting prefs are mapped back if
they were stored or if they have been manually set to something sane.
Secondly it corrects the calculation that was used to convert from millimeters
to tenths of millimeters.
It also gets rid of the paperSizeType field, which was only used on Windows and
doesn't actually make sense according to the DEVMODE documentation as the
dmPaperLength and dmPaperWidth fields override the dmPaperSize, but can in
theory be specified at the same time.

Comment 5

[Bob Owen (:bobowen)]

Comment on attachment 8709035 [details] [diff] [review]
Part 1: Hold new printable page sizes in print nsIPrintSettingsWin

Sorry roc, hadn't noticed you were out ... jimm's graciously stepped in. :-)

Comment 6

[Bob Owen (:bobowen)]

Attachment: Part 2: Move separate DEVMODE to nsIPrintSettings copying into nsPrintSettingsWin

This also corrects what I think are long standing issues with the mapping to and
from print settings on Windows.
Firstly it only uses the DEVMODE flags to decide what should get stored, in the
old code if paper size was not set, it would then use that possibly invalid
paper size to map to length and width. Paper setting prefs are mapped back if
they were stored or if they have been manually set to something sane.
Secondly it corrects the calculation that was used to convert from millimeters
to tenths of millimeters.
It also gets rid of the paperSizeType field, which was only used on Windows and
doesn't actually make sense according to the DEVMODE documentation as the
dmPaperLength and dmPaperWidth fields override the dmPaperSize, but can in
theory be specified at the same time.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8709035 [details] [diff] [review]
Part 1: Hold new printable page sizes in print nsIPrintSettingsWin

Review of attachment 8709035 [details] [diff] [review]:
-----------------------------------------------------------------

::: embedding/components/printingui/win/nsPrintDialogUtil.cpp
@@ +985,5 @@
>      // Unlock DeviceNames
>      ::GlobalUnlock(prntdlg.hDevNames);
>  
>      // Transfer the settings from the native data to the PrintSettings
>      LPDEVMODEW devMode = (LPDEVMODEW)::GlobalLock(prntdlg.hDevMode);

would you mind filing a 'good first bug' on creating a mozilla version of ScopedHGlobal and using it in this print code?

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8709502 [details] [diff] [review]
Part 2: Move separate DEVMODE to nsIPrintSettings copying into nsPrintSettingsWin

Review of attachment 8709502 [details] [diff] [review]:
-----------------------------------------------------------------

nice.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d43e96b14c
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4737474a9c

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e7d43e96b14c
https://hg.mozilla.org/mozilla-central/rev/fb4737474a9c

Comment 11

[Toshihiro Yamada]

These changes causes build failure with my environment(Win7 64bit, VS2015 64bit).

widget/windows/nsDeviceContextSpecWin.cpp(477): error C2664: 'HDC CreateICW(LPCWSTR,LPCWSTR,LPCWSTR,const DEVMODEW *)': cannot convert argument 2 from 'const char16_t *' to 'LPCWSTR'

Comment 12

[Bob Owen (:bobowen)]

(In reply to Toshihiro Yamada from comment #11)
> These changes causes build failure with my environment(Win7 64bit, VS2015
> 64bit).
> 
> widget/windows/nsDeviceContextSpecWin.cpp(477): error C2664: 'HDC
> CreateICW(LPCWSTR,LPCWSTR,LPCWSTR,const DEVMODEW *)': cannot convert
> argument 2 from 'const char16_t *' to 'LPCWSTR'

Thanks, I've filed bug 1242952 and will fix as soon as I can.