Closed Bug 1369386 Opened 2 years ago Closed 2 years ago

Better fix for invalid small page size printer preferences.

Categories

(Core :: Printing: Output, enhancement)

All
Windows
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
thunderbird_esr52 --- affected
firefox-esr52 55+ fixed
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1276717 +++

In bug 1276717 a problem with old invalid printer prefs causing small page sizes was uncovered, by the now "correct" processing of those prefs.
That bug had a fix that tried to recognize and sanitize the invalid prefs, which is complicated by the fact that the small page size could be valid in some circumstances. 

The fix there would not work in a small set of cases, because it relied on the values of an uninitialized variable.
The problem has also resurfaced for Thunderbird 52 and will probably be occurring more frequently for Fx52 ESR as well.
This is mainly because if users had upgraded (and printed) for Fx version 46 then the prefs would already have been fixed. ESR users jump straight from 45 to 52, so are more likely to see the issue.

This bug is to change that fix to one that uses the fact that an unused print_paper_size_type pref was also removed at the same time.
This pref will still be set in prefs that were created before version 46, so we can use it as part of the trigger for a slightly broader one off sanitization.
These old invalid prefs were caused by us not clearing out old inch page size prefs when the
user switched to a standard mm page size (for example A4).
This fix uses the fact that an old print_paper_size_type pref was removed at the same time
to trigger a one off sanitization.
Attachment #8873421 - Flags: review?(jmathies)
Comment on attachment 8873421 [details] [diff] [review]
Fix old invalid millimeter page size prefs using print_paper_size_type as trigger

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

::: widget/nsPrintOptionsImpl.cpp
@@ -511,5 @@
> -        // As an extra precaution only override, when the resolution is also
> -        // set to the legacy invalid, uninitialized value. We'll just broadly
> -        // assume that anything outside of a million DPI is invalid.
> -        if (GETINTPREF(kPrintResolution, &iVal) &&
> -            (iVal < 0 || iVal > 1000000)) {

Does that need rebasing? We're using <= now, right?
(In reply to Jorg K (GMT+2) from comment #2)
...
> > -            (iVal < 0 || iVal > 1000000)) {
> 
> Does that need rebasing? We're using <= now, right?

Ah yes it will, thanks.
I'll do that before I push it.
Attachment #8873421 - Flags: review?(jmathies) → review+
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ed8eb70165
Fix old invalid millimeter page size prefs using print_paper_size_type as trigger. r=jimm
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a37ca72024c6
Part 2: Follow-up #if to fix non-Windows build failures. r=me
https://hg.mozilla.org/mozilla-central/rev/a8ed8eb70165
https://hg.mozilla.org/mozilla-central/rev/a37ca72024c6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8873421 [details] [diff] [review]
Fix old invalid millimeter page size prefs using print_paper_size_type as trigger

* Note *: there was a follow-up patch to fix non-Windows build failures.

Also the patch that landed doesn't quite apply cleanly on ESR, but the original patch on the bug should.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Many users who use millimetre page sizes could be unable to print (or get many blank pages) due to invalid prefs.

User impact if declined: 
Same as above.

Fix Landed on Version:
55

Risk to taking this patch (and alternatives if risky): 
Low - the change is fairly simple and the sanitisation should only happen once if the old pref exists.
It would be good to get some quick Printing and Print Preview testing though.

String or UUID changes made by this patch: 
None
Attachment #8873421 - Flags: approval-mozilla-esr52?
I'd like this fix to stabilize some more on Nightly, let's uplift to ESR52.3 post-merge next week. This is a wontfix for ESR52.2
(In reply to Ritu Kothari (:ritu) from comment #9)
> I'd like this fix to stabilize some more on Nightly, let's uplift to ESR52.3
> post-merge next week. This is a wontfix for ESR52.2

FWIW, I think it's a shame if we can't get this into ESR52.2 as that is where we drop support for ESR45.
Looks like it doesn't apply to esr52
grafting 402898:a8ed8eb70165 "Bug 1369386: Fix old invalid millimeter page size prefs using print_paper_size_type as trigger. r=jimm"
merging widget/nsPrintOptionsImpl.cpp
 warning: conflicts while merging widget/nsPrintOptionsImpl.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(bobowencode)
(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> Looks like it doesn't apply to esr52
> grafting 402898:a8ed8eb70165 "Bug 1369386: Fix old invalid millimeter page
> size prefs using print_paper_size_type as trigger. r=jimm"
> merging widget/nsPrintOptionsImpl.cpp
>  warning: conflicts while merging widget/nsPrintOptionsImpl.cpp! (edit, then
> use 'hg resolve --mark')
> abort: unresolved conflicts, can't continue
> (use 'hg resolve' and 'hg graft --continue')

Yeah, I had to modify the patch slightly before landing because of another change, but the one attached to the bug should apply to ESR52 I believe.

The follow-up (a37ca72024c6) should apply on top of that OK I think.
Flags: needinfo?(bobowencode)
Comment on attachment 8873421 [details] [diff] [review]
Fix old invalid millimeter page size prefs using print_paper_size_type as trigger

Meets the triage bar, ESR52+
Attachment #8873421 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.