Closed Bug 499025 Opened 11 years ago Closed 2 years ago

[Error] "An error occurred while printing." when cancelling print to Microsoft XPS/PDF document Writer

Categories

(Core :: Printing: Setup, defect)

1.9.0 Branch
All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: v-jimclo, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.0; Trident/4.0; SLCC1; .NET CLR 2.0.50727; .NET CLR 3.0.04506; .NET CLR 1.1.4322; InfoPath.2; OfficeLiveConnector.1.3; OfficeLivePatch.0.0; MS-RTC LM 8)
Build Identifier: 

Tester/Developer Notes: 

Microsoft regularly tests applications currently in the market place against new products that we are working to release.  We have tested your Firefox Internet Browser application, on the Windows 7 Operating System.  The latest version of Firefox has an issue with print function to XPS.  After cancelling a print to XPS operation in Firefox, an error dialog pops up that says: "An unknown error occurred while printing". This is really confusing - Cancelling an operation should not cause errors, so this will be very confusing to users. Especially if you don't notice that you haven't installed a printer yet, and accidentally select the XPS Document Writer.

MXDW and other similar fake printer-drivers need to launch User Interface to let the user pick the save-as file-name. They do this during GDI32!StartDoc. When the user cancels the file-chooser dialogue, StartDoc returns ERROR_CANCELLED (0x4c7) back to the application. This is by-design and lets the application inform the user that they cancelled the operation. This behavior of MXDW's hasn't changed from Vista.

It turns out Firefox converts that error into a generic E_FAIL hresult. As a result, the user sees an error pop-up.

Firefox should handle ERROR_CANCELLED specially, for example like notepad does. FWIW, Microsoft Office Document Imaging, another virtual printer-driver behaves identically, and Firefox shows a similar error pop-up



Reproducible: Always

Steps to Reproduce:
Detailed Steps to Reproduce the Problem:  

1.	Install Windows 7
2.	Install Firefox Internet Browser from: http://www.mozilla.com/en-US/firefox/ie.html
3.	In Firefox, select File -> Print
4.	Select Microsoft XPS Document Writer (it's the default if you have no other printers installed)
5.	OK
6.	When the 'save' dialog is shown, select Cancel

Actual Results:  
Actual Result:  Error message is shown and print option is cancelled

Expected Results:  
Microsoft regularly tests applications currently in the market place against new products that we are working to release.  We have tested your Firefox Internet Browser application, on the Windows 7 Operating System.  The latest version of Firefox has an issue with print function to XPS.  After cancelling a print to XPS operation in Firefox, an error dialog pops up that says: "An unknown error occurred while printing". This is really confusing - Cancelling an operation should not cause errors, so this will be very confusing to users. Especially if you don't notice that you haven't installed a printer yet, and accidentally select the XPS Document Writer.


MXDW and other similar fake printer-drivers need to launch User Interface to let the user pick the save-as file-name. They do this during GDI32!StartDoc. When the user cancels the file-chooser dialogue, StartDoc returns ERROR_CANCELLED (0x4c7) back to the application. This is by-design and lets the application inform the user that they cancelled the operation. This behavior of MXDW's hasn't changed from Vista.

It turns out Firefox converts that error into a generic E_FAIL hresult. As a result, the user sees an error pop-up.

Firefox should handle ERROR_CANCELLED specially, for example like notepad does. FWIW, Microsoft Office Document Imaging, another virtual printer-driver behaves identically, and Firefox shows a similar error pop-up

This issue is deemed a poor user experience with the use of this product.
Have you tried with Firefox 3.5? Think this is core, more correctly.
Component: General → Printing: Setup
Product: Firefox → Core
QA Contact: general → printing.setup
Version: unspecified → 1.9.0 Branch
Confirming, I see this using Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1) Gecko/20090616 Firefox/3.5 (.NET CLR 3.5.30729).
Status: UNCONFIRMED → NEW
Ever confirmed: true
FWIW, the same thing happens using Windows Vista.
Still happening.

FWIW the only place we have added ERROR_CANCELLED handling is here:

https://hg.mozilla.org/mozilla-central/rev/cd1f281e562c
Summary: Error message is shown after trying to print in Firefox (to MXDW) and cancelling → [Error] "An error occurred while printing." when cancelling print to Microsoft XPS/PDF document Writer
Blocks: 1415347
(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #4)
> Still happening.

Although you need to print from print preview when e10s is enabled.
I have a fix that makes PrintTargetWindows::BeginPrinting return NS_ERROR_ABORT to indicate that the user canceled printing. We can't land that change without the 'part 1' patch in bug 1432409 though. Without it we'd get into the situation that that bug is fixing where initialization of the RemotePrintJob fails (in this case due to the user aborting), the parent sends a delete event to the child to delete the RemotePrintJobChild, but the child has async cleanup actions that try to send messages to the parent using the deleted RemotePrintJobChild.
Depends on: 1432409
To finish the notes in comment 6...we'd then MOZ_CRASH.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attachment #8952375 - Flags: review?(dholbert)
Comment on attachment 8952375 [details] [diff] [review]
patch

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

::: gfx/src/nsDeviceContext.cpp
@@ +532,5 @@
> +#ifdef DEBUG
> +    if (NS_FAILED(rv) && rv != NS_ERROR_ABORT) {
> +      NS_WARNING("nsDeviceContext::BeginDocument failed");
> +    }
> +#endif

Can you just make this NS_WARNING_ASSERTION to avoid the need for multiple statements & the #ifdef?

::: gfx/thebes/PrintTargetWindows.cpp
@@ +75,5 @@
>    docinfo.lpszOutput = docName.Length() > 0 ? docName.get() : nullptr;
>    docinfo.lpszDatatype = nullptr;
>    docinfo.fwType = 0;
>  
> +  // This can open a save-to-PDF/XPS pop-up dialog.  A negative return value

s/negative/negative (or zero)/

Otherwise, the implication is that zero would be treated as a non-error.  But the actual code treats zero as an error -- and that seems to be correct per the MS documentation:
https://msdn.microsoft.com/en-us/library/windows/desktop/dd145114(v=vs.85).aspx

@@ +76,5 @@
>    docinfo.lpszDatatype = nullptr;
>    docinfo.fwType = 0;
>  
> +  // This can open a save-to-PDF/XPS pop-up dialog.  A negative return value
> +  // means that the user pressed Cancel instead of entering a file name, which

Does it *definitely* mean the user pressed cancel?  I don't see that implied in the MS documentation for this API that I linked above.  And I found two pages that talk about StartDoc (the non-unicode version of this API) failing due to account-permission issues, IIUC:
https://stackoverflow.com/questions/20276193/my-startdoc-fails-in-windows-7-runs-well-in-windows-xp
http://support.efilecabinet.com/customer/en/portal/articles/2580207-general-%E2%80%93-%22startdoc-failed-printer-wont-initialize%22-error

The logic here might still be fine, but you probably want to soften the comment a bit (e.g. add "probably", as in "probably means that the user pressed Cancel"), and you might want to avoid having logic depend too heavily on this in nsDeviceContextSpecProxy.cpp -- see my comment over there.

::: layout/printing/ipc/RemotePrintJobParent.cpp
@@ +85,5 @@
>  
>    rv = mPrintDeviceContext->BeginDocument(aDocumentTitle, aPrintToFile,
>                                            aStartPage, aEndPage);
> +  if (NS_FAILED(rv)) {
> +    NS_WARN_IF(rv != NS_ERROR_ABORT); // don't warn if user canceled

You don't want NS_WARN_IF here. NS_WARN_IF is meant to be used *inside of* "if" checks, and it causes problems outside. See bug 1299384 comment 0 and its documentation:
https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/xpcom/base/nsDebug.h#28-29,35-40

You could pass it to Unused, but that's clumsy -- really you probably want NS_WARNING_ASSERTION(..., "explanatory message"), as noted later on in that ^^ nsDebug.h documentation.

::: layout/printing/nsPrintJob.cpp
@@ +1934,5 @@
>  
>    PR_PL(("****************** Begin Document ************************\n"));
>  
> +  if (NS_FAILED(rv)) {
> +    NS_WARN_IF(rv != NS_ERROR_ABORT);

As above, you don't really want NS_WARN_IF here -- this should probably be NS_WARNING_ASSERTION(rv == NS_ERROR_ABORT, "some brief warning");

@@ +2044,5 @@
>  
>    /* cleaup on failure + notify user */
>    if (aHandleError && NS_FAILED(rv)) {
> +    if (rv != NS_ERROR_ABORT) {
> +      NS_WARNING("nsPrintJob::AfterNetworkPrint failed");

This can collapse to
  NS_WARNING_ASSERTION(rv == NS_ERROR_ABORT,
                       "nsPrintJob::AfterNetworkPrint failed");

::: widget/nsDeviceContextSpecProxy.cpp
@@ +149,5 @@
>      // mRemotePrintJob.  We must not try to use it again.
>      mRemotePrintJob = nullptr;
> +
> +    if (rv == NS_ERROR_ABORT) {
> +      mPrintSettings->SetIsCancelled(true); // user canceled printing

1) the API uses the 2-L spelling ("cancelled") so any comments probably should use that too. ("user cancelled printing")


2) Is there a reason we *only* want to do this in the "user action" scenario?  Or should we just do this unconditionally? Because:
 - If we're nulling out mRemotePrintJob before we even complete BeginDocument, it seems accurate to report that "the print job has been cancelled" regardless of whether the *user* initiated the cancellation.
 - Based on the warnings you're adding elsewhere, it seems like other sorts of failures are unexpected here anyway.
 - NS_ERROR_ABORT isn't cleanly specific to "user initiated the cancellation". It looks like StartDocW can fail for other reasons beyond user clicking cancel as I noted in my PrintTargetWindows.cpp notes.  And presumably other failures could end up getting us NS_ERROR_ABORT here.  So rather than pretending that this is a check for that specific scenario, maybe it's better to do this unconditionally in the error-handling code here, if it's really something that we want to do?
Attachment #8952375 - Flags: review?(dholbert) → review-
Thanks for the pointer to NS_WARNING_ASSERTION (and the warning about NS_WARN_IF outside |if|). I looked for such a thing but failed to find it due to the wonky naming.

(In reply to Daniel Holbert [:dholbert] from comment #9)
> 2) Is there a reason we *only* want to do this in the "user action"
> scenario?

Actually I'm going to drop this change, since I'm going to remove the SetIsCancelled method from nsIPrintSettings in a separate bug (it's not appropriate to abuse the settings object to pass this non-settings state around).
(In reply to Daniel Holbert [:dholbert] from comment #9)
> ::: gfx/thebes/PrintTargetWindows.cpp
Z> @@ +76,5 @@
> >    docinfo.lpszDatatype = nullptr;
> >    docinfo.fwType = 0;
> >  
> > +  // This can open a save-to-PDF/XPS pop-up dialog.  A negative return value
> > +  // means that the user pressed Cancel instead of entering a file name, which
> 
> Does it *definitely* mean the user pressed cancel?  I don't see that implied
> in the MS documentation for this API that I linked above.  And I found two
> pages that talk about StartDoc (the non-unicode version of this API) failing
> due to account-permission issues, IIUC:
> https://stackoverflow.com/questions/20276193/my-startdoc-fails-in-windows-7-
> runs-well-in-windows-xp
> http://support.efilecabinet.com/customer/en/portal/articles/2580207-general-
> %E2%80%93-%22startdoc-failed-printer-wont-initialize%22-error

The cases of failures I found online seem to be related to NON-save-to-file uses of this API. For save-to-file I couldn't get it to fail using anything other than Cancel in the prompt that asks you where to save the file. For example, the prompt will refuse to allow you to close it if you try to save to a directory you don't have permissions to write to (it will pop up a second prompt telling you that you don't have permission to save to that directory). But yeah, there are probably some edge cases that can cause it to fail.

However, this seems to be moot since on further testing it seems that StartDoc *does* nowadays set the error code that can be returned by GetLastError(). At least GetLastError() seems to consistently returns ERROR_CANCELLED in Windows 10 and 7. Since we no longer support Windows versions prior to 7 we should be okay to use GetLastError() here. Admittedly this behavior doesn't seem to be officially documented (maybe the docs need updating?) but in the worst case scenario that a future version of Windows breaks this behavior we'll simply return to reporting a bogus error (the issue this bug is trying to fix).
Attached patch patch (obsolete) — Splinter Review
I'll punt this off to Bob now that he's back.
Attachment #8952375 - Attachment is obsolete: true
Attachment #8954249 - Flags: review?(bobowencode)
Attached patch patchSplinter Review
Attachment #8954249 - Attachment is obsolete: true
Attachment #8954249 - Flags: review?(bobowencode)
Attachment #8954250 - Flags: review?(bobowencode)
Comment on attachment 8954250 [details] [diff] [review]
patch

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

::: gfx/src/nsDeviceContext.cpp
@@ +528,5 @@
>          }
>          mIsCurrentlyPrintingDoc = true;
>      }
>  
> +    // Warn about any failure (except user cancelling):

tiny nit: did you mean to use a colon here instead of a full stop?

::: gfx/thebes/PrintTargetWindows.cpp
@@ +75,5 @@
>    docinfo.lpszOutput = docName.Length() > 0 ? docName.get() : nullptr;
>    docinfo.lpszDatatype = nullptr;
>    docinfo.fwType = 0;
>  
> +  // If the user selected save-to-PDF/XPS pop-up dialog, then the following

Should this say "... selected Microsoft Print to PDF or XPS Document Printer,"?

@@ +83,5 @@
> +  // because the user hit Cancel, since we want to treat that specially to
> +  // avoid notifying the user that the print "failed" in that case.
> +  int result = ::StartDocW(mDC, &docinfo);
> +  if (result <= 0) {
> +    if (::GetLastError() == ERROR_CANCELLED) {

Shame that this isn't documented, but it can only improve the situation here anyway.
Attachment #8954250 - Flags: review?(bobowencode) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d45bbc02ad0d
Don't open an error pop-up if the user cancels printing. r=bobowen
https://hg.mozilla.org/mozilla-central/rev/d45bbc02ad0d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.