701479 - Misuse of EnumPrintersW and GetDefaultPrinterW

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Zack Weinberg (:zwol)]

The "helper function" EnumerateNativePrinters (in nsDeviceContextSpecWin.cpp) uses the Win32 API function EnumPrintersW to get a list of all available printers.  It does so in a way that risks blocking the calling thread for an extended period (requesting "level 2" instead of "level 4" information) and inconsistently (using ENUM_PRINTERS_LOCAL instead of aWhichPrinters on the second call).

In nsDeviceContextSpecWin.cpp and also nsPrintDialogUtil.cpp, GetDefaultPrinterW is used to retrieve the name of the default printer.  This function also potentially blocks the calling thread for an extended period.

I am not sufficiently good at Windows GUI programming to know what _should_ be done in these cases, but it appears to me that the surviving use of EnumerateNativePrinters (after bug 629500) is probably using a much bigger hammer than is strictly necessary.

Comment 1

[Oskar Eisemuth]

Attachment: Removing EnumPrintersW for checking only if Print to FILE: and starting an UI prompt.

Technical the removed code asked if FILE: was used as printer port (and so need to find all info about all printers in the worst case), then it should show a Mozilla File selection dialog. 

However when using a printer with file: or setting print to file in the native print dialog on Windows 7 64bit it shows only the native OS file input box. So I have no idea who needs that special part where it even works.

Anyway I think the nsDeviceContextSpecWin shouldn't ask the user and manipulate the PrintSettings.

Comment 2

[Zack Weinberg (:zwol)]

Comment on attachment 8366047 [details] [diff] [review]
Removing EnumPrintersW for checking only if Print to FILE: and starting an UI prompt.

Please don't set feedback+ on your own patches, only feedback?  Also, setting any of the per-patch flags or needinfo to '?' is only effective if you select a specific person who should respond.

I think what you actually want here is feedback from Jim Mathies (who understands Windows much better than I do).  It appears to me that the patch cannot be landed as-is, so you're not looking for *review* at this time.  In particular, our style is to *never* #if things out; if they can't be removed yet, then don't remove them; if they *can* be removed, then just do it.  (I am under the impression that this stuff has other uses at least till we get through bug 629500, but I could be mistaken.  It's been a while since I seriously looked at this code.)

Comment 3

[Oskar Eisemuth]

There are two EnumerateNativePrinters, one in the GlobalPrinters Class as method and as a local function.
I removed the local function now that may hang when doing actual printing, as it used for the FILE: code. The other one is the GlobalPrinters one.

Comment 4

[Zack Weinberg (:zwol)]

Thanks for clarification.  Looking at the whole file, I think you should just go ahead and delete GetFileNameForPrintSettings.  If we wind up putting something like that back, it will be in a totally different place and probably won't resemble that code all that much either, so I don't see value in keeping it around.  We can always dig it back out of source control if we discover we want it.

Comment 5

[Jim Mathies [:jimm]]

This code stretches back all the way to 2002. I really have no idea what it's doing. Building up a patch with these changes, will see if I can spot anything odd.

Comment 6

[Jim Mathies [:jimm]]

These changes don't seem to impact the print to file drivers I have on my system. I also can't find any data on "Print To File" drivers that use FILE: in the name. *shrug*.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8366047 [details] [diff] [review]
Removing EnumPrintersW for checking only if Print to FILE: and starting an UI prompt.

minusing for now since 1) this patch didn't apply cleanly to mozilla-central, and 2) the ifdefing stuff needs to come out. The code should just be removed.

We can land this on mc and wait to see if bugs get filed on missing printers.

Comment 8

[Jim Mathies [:jimm]]

Oskar, mind working up an up to date patch with the suggested changes?

Comment 9

[Oskar Eisemuth]

I will remove GetFileNameForPrintSettings, and retry to create patch.

I did some more research, but I am bit lost in the code, it would be nice if someone could check this again.

I found: 
nsPrintDialogUtil.cpp
-> ShowNativePrintDialog

There is again code that does roughly the same, setting the filename for PrintSettings. 
When flag PD_PRINTTOFILE would be set, then FILE: could be the printer device name. (you still won't need the port part)

The SetupToPrintContent code in the platform independent part uses the filename in PrintSettings for later calling Win32 BeginPrinting. 
If it really is zero on that point Win32 seems to ask the user with the usually user unfriendly file input box. (A simple text input box!)

It looks like that the code was written for PrintDlgEx in mind. The API description there differ a bit from PrintDlg. PrintDlg seems to ask directly and return the filename. PrintDlgEx won't ask, let the user code show a file picker. 

In the end the PrintDlgEx Dialog should be used, the GetFileNameForPrintSettings should be moved to ShowNativePrintDialog. If the user selects print to file, replace the broken OS failback with a native file save as dialog. 

However after all it is still very odd to query all printers and not only the given printer.
ShowNativePrintDialog however should already have done all the work or I am wrong here?

Comment 10

[Oskar Eisemuth]

Attachment: Removing EnumPrintersW to see if port is FILE:, code path seems unused.

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8366637 [details] [diff] [review]
Removing EnumPrintersW to see if port is FILE:, code path seems unused.

Looks good, thanks!

Please update the patch to include the review (r=jimm) at the end of the title. Then you can mark the bug for checkin-needed in the keywords entry.

Comment 12

[Oskar Eisemuth]

Attachment: Removing EnumPrintersW to see if port is FILE:, code path seems unused. (review added)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a8f945b28da

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2a8f945b28da

Comment 15

[Kamil Jozwiak [:kjozwiak]]

Marking this as qe-verify- as there's no poc code or an exploitable bug that can be used to verify this code change. If there's something QE can do to test/verify this code change, please let me know and needinfo!