Last Comment Bug 323781 - Prints still go to printer even after it is deleted
: Prints still go to printer even after it is deleted
Status: RESOLVED FIXED
: relnote
Product: Core
Classification: Components
Component: Printing: Output (show other bugs)
: Trunk
: x86 Windows XP
: -- major (vote)
: ---
Assigned To: Ian Neal
:
Mentors:
Depends on: 324072
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-17 09:43 PST by Ian Neal
Modified: 2006-02-04 11:51 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Provisional patch v0.1 (2.55 KB, patch)
2006-01-20 15:24 PST, Ian Neal
kherron+mozilla: review-
Details | Diff | Review
Revised patch v0.2 (6.09 KB, patch)
2006-01-21 17:48 PST, Ian Neal
no flags Details | Diff | Review

Description Ian Neal 2006-01-17 09:43:20 PST
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 2006-01-17 09:48:50 PST
Just checked on Firefox 1.0RC1 (BuildID 20041027) and happens on there too.
Comment 2 Ian Neal 2006-01-17 16:31:23 PST
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 2006-01-17 17:00:37 PST
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 2006-01-17 17:48:48 PST
I suspect we should be testing if mPrinter in nsPrintSettings::GetPrinterName is still a valid printer somehow.
Comment 5 Ian Neal 2006-01-20 15:24:59 PST
Created attachment 209154 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2006-01-20 20:23:35 PST
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 2006-01-21 11:41:25 PST
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 2006-01-21 12:53:59 PST
(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 2006-01-21 17:48:33 PST
Created attachment 209247 [details] [diff] [review]
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 2006-01-23 12:38:30 PST
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 2006-01-26 08:20:16 PST
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 2006-02-04 11:51:17 PST
This should be fixed by the checkin for bug 324072. See bug 324072 comment 15.

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