323781 - Prints still go to printer even after it is deleted

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Ian Neal]

Tested using Firefox 1.6a1 BuildID20060116, SeaMonkey 1.1a BuildID 20060111 and Mozilla 1.8b2 BuildID 20050521
Steps to reproduce
1. Map two printers (Printer A and Printer B) to printer shares on another PC and make one of them the default (say Printer A)
2. Print a web page to the non-default printer (Printer B) using either print button or file and print.
3. Delete non-default printer (Printer B) via Control Panel, Printers.
4. Select file and print or click on print button to print the web page again, this time Printer A shows as printer to be printed to (do not click on the drop down at all).
5. Click on OK button to print.

Actual result
1. Both prints come off on the Printer B.

Expected result
1. First print should come off on Printer B and second should come off on Printer A.

Comment 1

[Ian Neal]

Just checked on Firefox 1.0RC1 (BuildID 20041027) and happens on there too.

Comment 2

[Ian Neal]

Using JS debugger on http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/printing.js#147 or http://lxr.mozilla.org/seamonkey/source/toolkit/components/printing/content/printUtils.js#155
onwards initPrintSettingsFromPrefs(aPrintSettings, true,  aPrintSettings.kInitSaveAll) works out the printerName to be Printer B even when it no longer exists and the popup dialog shows Printer A (the default)

Comment 3

[Kenneth Herron]

This sounds like the same logic error at work in bug 312583. |InitPrintSettingsFromPrefs()| receives an nsIPrintSettings object initialized with a printer name. It starts by loading global print settings into the object, then it loads printer-specific settings for the named printer.

The problem is that while loading the global settings, the printer name is overwritten with the name of the printer used for the last print job (from the print.print_printer preference).

Comment 4

[Ian Neal]

I suspect we should be testing if mPrinter in nsPrintSettings::GetPrinterName is still a valid printer somehow.

Comment 5

[Ian Neal]

Attachment: Provisional patch v0.1

This patch:
* Adds a check to GetAdjustedPrinterName to see if the last used printer, according to prefs, still exists and if not, picks the default printer.

I have compiled and tested the code on Windows/mingw/cygwin and it works but might still need some refinement / re-writing.

Comment 6

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

It would probably be a bit cleaner if some of the code was broken out into an IsPrinterAvailable static helper function. Also, wStr can be declared where it is assigned and the declaration of printerName moved closer to its first use.

I guess the other question is whether there's an easier way to achieve this.

Comment 7

[Kenneth Herron]

Comment on attachment 209154 [details] [diff] [review]
Provisional patch v0.1

I'm not fond of this approach. |GetAdjustedPrinterName()| is just a helper function to fix up a printer name so it can be used inside a preference name. It's bad design for it to substitute a completely different printer name. For example, both |InitPrintSettingsFromPrefs()| and |SavePrintSettingsToPrefs()| allow loading and saving prefs for a printer specified by the caller. But they call |GetAdjustedPrinterName()|, so the prefs they load or save could be for an entirely different printer. And see bug 312583, which is another case of |InitPrintSettingsFromPrefs()| changing the caller-specified printer unexpectedly.

I'd like to know where this invalid printer name is coming from in the first place. It's apparently not in the printer enumeration or this patch wouldn't be effective. Maybe we're loading the name of the printer used for the last print job, without checking whether it's still valid.

Comment 8

[Ian Neal]

(In reply to comment #7)
> And see bug 312583, which is another case of
> |InitPrintSettingsFromPrefs()| changing the caller-specified printer
> unexpectedly.
> 
> I'd like to know where this invalid printer name is coming from in the first
> place. It's apparently not in the printer enumeration or this patch wouldn't be
> effective. Maybe we're loading the name of the printer used for the last print
> job, without checking whether it's still valid.
> 
Yes, that is correct, we are loading the name of the printer used for the last print job without checking whether it's still valid. Both this bug and bug 312583 are to do with |InitPrintSettingsFromPrefs()| and for this bug I see |GetAdjustedPrinterName()| needing to return an adjusted and valid printer name. 

Is there any circumstance where |InitPrintSettingsFromPrefs()| should be using an invalid printer?

Comment 9

[Ian Neal]

Attachment: Revised patch v0.2

Changes since v0.1:
* Created IsPrinterAvailable static help function and other tidy ups as per comment 6.
* Changed GetAdjustedPrinterName to only be called when aUsePNP is true as Truncate() was not changing anything in the function.
* Changed the now redundant 2nd argument in GetAdjustedPrinterName to be used to indicate if printer availability should be checked.

Comment 10

[Kenneth Herron]

I've submitted a patch to bug 324072 (same issue as bug 312583) which I think applies to this bug as well. I attached it to that bug because both of those are linux bugs, and I can't test it on windows. Ian, I'd appreciate your comments on it.

Comment 11

[Ian Neal]

Comment on attachment 209247 [details] [diff] [review]
Revised patch v0.2

Cancelling requests as this should be fixed by patch on bug 324072

Comment 12

[Kenneth Herron]

This should be fixed by the checkin for bug 324072. See bug 324072 comment 15.