Open Bug 1680838 Opened 4 years ago Updated 2 years ago

Page-margin and page-size settings still aren't reliably respected in test_printpreview.xhtml, on Windows, because we end up stomping on them in a call to `nsPrintSettingsWin::CopyFromNative`

Categories

(Core :: Print Preview, defect, P2)

defect

Tracking

()

People

(Reporter: dholbert, Unassigned)

References

Details

Attachments

(3 files, 1 obsolete file)

I'm spinning off this bug as a way of resurrecting bug 1671036 (which is closed because a speculative patch landed, but wasn't really fully fixed per bug 1671036 comment 9). This is causing me some trouble in writing a test for bug 1670068; hence, I'm filing this bug.

The basic symptom here is that test_printpreview.xhtml doesn't seem to reliably respect its subtests' unwriteable margin settings, specifically on Windows.

I've partially tracked this down -- it looks like in our printPreview() call, we end up calling nsPrintSettingsWin::CopyFromNative on the passed-in printSettings argument, which overwrites its unwriteable margin data (among other things).

My windows dev environment is a bit cobwebby (I can't get MSVC to attach & debug the relevant process for some reason), so I don't fully understand what's going on, but I'll post a bit more of what I've learned at this point.

Attached patch patch to demonstrate bug (obsolete) (deleted) — Splinter Review

Here's a patch with a new test in printpreview_helper.xhtml, which I would expect to pass, but does not pass on Windows. (I've adjusted the logic to jump straight to that subtest, too.)

Attached file log of test run with patch applied (deleted) —

Relevant snippet of log:

 1:01.14 GECKO(18376) *****dholbert Entering SetUnwriteableMarginTop: 0.100000 -> 144 (this=000001D233E6A4A0)
[...]
 1:01.14 GECKO(18376) [18600, Main Thread] ###!!! ASSERTION: *****dholbert In nsPrintSettingsWin::CopyFromNative
[...]
 1:01.20 GECKO(18376) *****dholbert In nsPrintSettingsWin::InitUnwriteableMargin
 1:01.20 GECKO(18376) *****dholbert Entering GetUnwriteableMarginTop and returning: 0.000000

(I used an asserting NS_ERROR in CopyFromNative so that I'd get a backtrace; it's there in the log.)

The SetUnwriteableMarginTop: 0.100000 line is logging where the test is trying to set that custom value.

Then CopyFromNative gets called, which triggers InitUnwriteableMargin, which reinitializes the unwriteable margin to 0 in the printSettings object. And then that's the value we get from our printSettings queries when we're actually rendering, as shown in the GetUnwriteableMarginTop logging line.

Here's a Try run with the attached "patch to demonstrate bug":
https://treeherder.mozilla.org/jobs?repo=try&revision=e5f6458a3aa4f284eecb02d4ff4ec30a7a4ab3ba&selectedTaskRun=Z2-dad7cRmekJxWZbEeCKA.0

The included test is intended to demonstrate this Windows-specific issue. I haven't worked too hard on polishing it, so it does actually fail on all platforms, with fuzzy-failures on Linux and Mac -- this is due to fuzzy antialiasing differences at the edge of the filled-in area -- it's reported as different: 1022, maxDifference: 142. If you look at the actual screenshots, though, you can see that the testcase and reference do actually match visually, and the testcase is rendered in a way that does honor the nonzero unwriteable margins. (on those platforms).

In contrast: on Windows, the failure is more severe (different: 17610, maxDifference: 255), and the data URI screenshots show that the testcase was indeed rendered without any margins at all.

Also worth noting, the attached log is from a debug+opt build, so the backtrace of the nsPrintSettingsWin::CopyFromNative call is missing a couple of stack-levels (presumably due to code-inlining optimizations)

The log shows:

 ###!!! ASSERTION: *****dholbert In nsPrintSettingsWin::CopyFromNative
 'Error', file c:/Users/dholbert/builds/mozilla-central/mozilla/widget/windows/nsPrintSettingsWin.cpp:317
 #01: nsDeviceContextSpecWin::Init (c:\Users\dholbert\builds\mozilla-central\mozilla\widget\windows\nsDeviceContextSpecWin.cpp:230)
 #02: nsPrintJob::DoCommonPrint (c:\Users\dholbert\builds\mozilla-central\mozilla\layout\printing\nsPrintJob.cpp:770)
 #03: nsPrintJob::CommonPrint (c:\Users\dholbert\builds\mozilla-central\mozilla\layout\printing\nsPrintJob.cpp:483)
 #04: nsPrintJob::PrintPreview (c:\Users\dholbert\builds\mozilla-central\mozilla\layout\printing\nsPrintJob.cpp:864)

Inside of stack-level #01 there (nsDeviceContextSpecWin::Init), we actually descend into nsDeviceContextSpecWin::GetDataFromPrinter[1] (which is inlined and hence doesn't show up in the backtrace), and from there, into nsPrintSettingsWin::CopyFromNative, which is where we trip my logging patch's NS_ERROR line. This is the function that stomps on the custom unwriteable margin member-data in the print settings object, via its call to InitUnwriteableMargin[3] as noted above.

[1] https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/widget/windows/nsDeviceContextSpecWin.cpp#396
[2] https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/widget/windows/nsPrintSettingsWin.cpp#312
[3] https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/widget/windows/nsPrintSettingsWin.cpp#367

Blocks: 1672974

This probably affects regular printing as well, right?

Severity: -- → S3
Flags: needinfo?(dholbert)
Priority: -- → P2

It probably does affect regular printing, but fortunately it's probably not a bug that users would be in a position to trigger (or notice, if they do trigger it).

I think the issue here is that we're being overly respectful of the unwriteable margin values that we get back from the printer (and we're stomping on values in the nsIPrintSettings object that the test -- or more generally, the browser frontend code -- had manually provided & was trying to use instead). Given that the unwriteable-margin values aren't user-exposed/editable [1], it shouldn't be possible (or at least easy) for a user to trigger this.

[1] (I know these values are sort of exposed in about:config, but I seem to recall those are write-only? Or, if there are saved values in there that we're using, it's entirely likely they're junk and the printer-supplied values are better anyway.)

Flags: needinfo?(dholbert)

jwatt mentioned that bobowen's work in bug 1669149 might help or be related here --> adding to see-also.

FWIW, I just retested this (on Try) and confirmed this is still an issue.

Here's the rebased "patch to demonstrate bug" that can be used with current mozilla-central.
(And a Try run with windows-only failures in the added subtest, demonstrating this issue: https://treeherder.mozilla.org/jobs?repo=try&searchStr=c1&revision=5d2a9dd792b2fbde598e2f397f6103f089abe4ce )

Attachment #9191424 - Attachment is obsolete: true

It looks like page size settings aren't being respected here, either!

At least, it looks like that's happening in this autoland test-failure log, for some tests that use a 5in-by-5in square sheet of paper via the following in printpreview_helper.xhtml:

      settings: {
        paperWidth: 5,
        paperHeight: 5,
        paperSizeUnit: Ci.nsIPrintSettings.kPaperSizeInches,

(That log triggered a backout due to this test failure -- the tests in question just need higher fuzzy annotations to account for this bug -- but while reviewing the screenshots in the log, I noticed the paper-size seems to be not getting respected. So it's not just the unwriteable margins. See screenshot in next comment.)

Summary: Page-margin settings still aren't reliably respected in test_printpreview.xhtml, on Windows, because we end up stomping on them in a call to `nsPrintSettingsWin::CopyFromNative` → Page-margin and page-size settings still aren't reliably respected in test_printpreview.xhtml, on Windows, because we end up stomping on them in a call to `nsPrintSettingsWin::CopyFromNative`

Here's one of the screenshots from the log in comment 10.

This is supposed to show a 5x5 sheet of paper, but it's actually showing a rectangular sheet of paper (probably US-Letter or A4 or whatever the system default is on our test runners).

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: