185588 - Changed printing preferences are saved corrupted in prefs.js

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Graham Hudspith]

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.3a) Gecko/20021215
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.3a) Gecko/20021215

Brought up Mozilla 1.3a for first time (have moved previous ~/.mozilla out of
way to start "afresh"). Brought up print dialog and changed Properties so that
"Paper Size" = "A4", "Paper Color" = "Greyscale" and "Print Command" = "prt"
(a local printing system). Print of a web page worked fine. Quit out of mozilla
and restarted it. Looked at prefs.js (actually, compared it against previous
version for 1.2.1) and found that the print preferences were corrupted.

Now have:

user_pref("print.  8.27", "  8.27");
user_pref("print. 11.65", " 11.65");
user_pref("print.ÿ¾­", "prt");

and the following are missing:

user_pref("print.printer_PostScript/default.print_command", "prt");
user_pref("print.printer_PostScript/default.print_paper_height", " 11.65");
user_pref("print.printer_PostScript/default.print_paper_name", "A4");
user_pref("print.printer_PostScript/default.print_paper_width", "  8.27");

Reproducible: Always

Steps to Reproduce:
Err, see "Details" section.
Actual Results:  
Bring up the print Properties dialog and the settings are back to their
defaults (my previous settings have been lost).

Expected Results:  
Saved my settings correctly and remembered them after a restart.

Comment 1

[Russel]

Yes. Happens with me, too ... Mozilla 1.3a
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021212

Most frustrating :) It is only a recent happening. Everything worked ok a few
days ago.

Comment 2

[Roland Mainz]

Caillon played recently on the prefs system...

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

the mentioned patch from caillon seems to be bug 182702

Comment 4

[Roland Mainz]

Confirming bug, I am seeing junk like
-- snip --
user_pref("print.", true);
user_pref("print.210.00", "210.00");
user_pref("print.297.00", "297.00");
user_pref("print.printer_ps003@castor:18.print_coÿ^Wz(", "");
user_pref("print.printer_ps003@castor:18.print_paÿ^Wz(", "iso-a4");
-- snip --
in my prefs.js

We've worked _HARD_ to get the printing prefs working... and now they are ruined
again... x@@@!!!... ;-(

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

rom the patch on that bug:
gfx/src/nsPrintOptionsImpl.cpp
@@ -316,21 +322,20 @@
+  nsCAutoString prefName;
[...]
+  return prefName.get();

I am not surprised that this fails. You are returning a pointer to a stack-based
string, which will be destroyed by the time the function has returned.

The old code had prefName as a member variable, so that was not an issue there.

Comment 6

[Roland Mainz]

Taking myself...

Comment 7

[Roland Mainz]

Attachment: Patch for 2002-12-16-08-trunk which restores the original, properly working code

Comment 8

[Simon Montagu :smontagu]

Comment on attachment 109540 [details] [diff] [review]
Patch for 2002-12-16-08-trunk which restores the original, properly working code

>-  if (!aPrefName || !*aPrefName) {
>-    NS_ERROR("Must have a valid pref name!");
>+   if (!aPrefName || !*aPrefName) {
>+    NS_ASSERTION(0, "Must have a valid pref name!");

Remove this indentation change.

I expect if I say that the string usage is nasty you will reply that we can
file a separate bug on that and you just want to get the prefs working ASAP, so
r=smontagu with the change noted above.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 109540 [details] [diff] [review]
Patch for 2002-12-16-08-trunk which restores the original, properly working code

hm, a few issues:
-  if (!aPrefName || !*aPrefName) {
-    NS_ERROR("Must have a valid pref name!");
+   if (!aPrefName || !*aPrefName) {
+    NS_ASSERTION(0, "Must have a valid pref name!");

1. why change the indentation of the if?
2. why change it back to NS_ASSERTION?

as for the rest:
a) can you make it an nsCAutoString rather than a nsCString? That's faster.
b) err. the string usage here is _ugly_. there is NS_LITERAL_CSTRING, you
know... also, if aPrinterName is non-ascii, you have a problem. not to mention
that function arguments should use nsAString& not nsString&.

hm, maybe that would better belong in a separate bug...
but please do file such a bug. and it would be nice if you could address a) as
that is a one-line change.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

to clarify: "it" in a) means the member variable mPrefName.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 109540 [details] [diff] [review]
Patch for 2002-12-16-08-trunk which restores the original, properly working code

This string stuff is way ugly, but it's not really your fault. sr=roc+moz

Comment 12

[Roland Mainz]

Attachment: New patch for 2002-12-16-08-trunk to match r=smontagu and biesi's comments

Comment 13

[Roland Mainz]

Christian Biesinger wrote:
> (From update of attachment 109540 [details] [diff] [review])
> hm, a few issues:
> -  if (!aPrefName || !*aPrefName) {
> -    NS_ERROR("Must have a valid pref name!");
> +   if (!aPrefName || !*aPrefName) {
> +    NS_ASSERTION(0, "Must have a valid pref name!");
>
> 1. why change the indentation of the if?

That was an accident. I pasted the patch into the code and removed the "+
"-lines and the "-" in front of the "- "-lines - and somehow I forgot one blank.

> 2. why change it back to NS_ASSERTION?

Another victim of the "lets roll-back this patch"-panic... ;-/

> as for the rest:
> a) can you make it an nsCAutoString rather than a nsCString? That's faster.#

Fixed.

> b) err. the string usage here is _ugly_. there is NS_LITERAL_CSTRING, you
> know... also, if aPrinterName is non-ascii, you have a problem. not to mention
> that function arguments should use nsAString& not nsString&.

In theory we could revamp the whole print prefs code to make it cleaner, faster
etc. - but I'd like to avoid it for now. Each xx@@!!!-time someone touched the
printing prefs stuff we have hordes of regressions. And this bug is only to fix
one of these regressions to get the code working again. Doing more on this code
may likely result in another regression... ;-/

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 109766 [details] [diff] [review]
New patch for 2002-12-16-08-trunk to match r=smontagu and biesi's comments

sr=roc+moz

Comment 15

[R.K.Aa.]

*** Bug 186254 has been marked as a duplicate of this bug. ***

Comment 16

[R.K.Aa.]

"rubbish in preferences file" was also commented about in bug 185491

Comment 17

[Simon Montagu :smontagu]

The fix has been checked in.

Comment 18

[R.K.Aa.]

*** Bug 189282 has been marked as a duplicate of this bug. ***