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)
Tracking
()
People
(Reporter: dholbert, Unassigned)
References
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
image/png
|
Details |
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.
Reporter | ||
Comment 1•4 years ago
|
||
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.)
Reporter | ||
Comment 2•4 years ago
|
||
Reporter | ||
Comment 3•4 years ago
|
||
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.
Reporter | ||
Comment 4•4 years ago
|
||
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.
Reporter | ||
Comment 5•4 years ago
|
||
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
Comment 6•4 years ago
|
||
This probably affects regular printing as well, right?
Reporter | ||
Comment 7•4 years ago
|
||
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.)
Reporter | ||
Comment 8•4 years ago
|
||
jwatt mentioned that bobowen's work in bug 1669149 might help or be related here --> adding to see-also.
Reporter | ||
Comment 9•3 years ago
|
||
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 )
Reporter | ||
Comment 10•2 years ago
|
||
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.)
Reporter | ||
Comment 11•2 years ago
|
||
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).
Description
•