97907 - Print system ignores device error codes at initalisation and does not display an error dialog in those cases

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Roland Mainz]

The print system ignores error codes from device context at initalisation.

The print dialog cannot be opened again after the device context failed to
initalize in the previous print attempt. It looks that the code still assumes
that a "print session" is on progress... ;-(

Comment 1

[Roland Mainz]

Reassigning to myself - assuming that dcone gives me a quick r= for the
following patch ... :-)

Comment 2

[Roland Mainz]

Accepting...

Comment 3

[Roland Mainz]

Attachment: Patch for 2001-08-27-08-trunk

Comment 4

[Roland Mainz]

Filed patch, requesting r=/sr= ...

Comment 5

[Roland Mainz]

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

Comment 6

[Roland Mainz]

Comment on attachment 47949 [details] [diff] [review]
Patch for 2001-08-27-08-trunk

I am going to file a new patch incl a fix for bug  87727 ...

Comment 7

[Roland Mainz]

Upcoming patch requires patch from bug 90380 to be applied first ...

Comment 8

[Roland Mainz]

Attachment: New patch

Comment 9

[Roland Mainz]

Filed new patch, incl. support for a "dumb" error dialog if something goes wrong
during printing.

I really wish that Mozilla code would allow C++ exceptions - it would make
things much easier (like passing arguments to error messages via exceptions) ...

I know that the error dialog code should be smarter (i18n support, arguments for
messages (e.g. "cannot open file '%s'.") etc.) - but I think this version is far
better than the current behaviour (e.g. picking the error codes from a core
dump...) ;-/
Additionally we do not honor any errors from
BeginDocument()/BeginPage()/EndPage()/EndDocument() - but that's worth another
bug (currently we only catch errors at device context initalisation) ...

Requesting r=/sr= for this version; we can improve it step-by-step in later bugs
... :-)

Comment 10

[(Don't use, old account) - Kai Engert]

You use showOSAlert. This is an error function only available in the mozilla
app, not inside any component. Although it might work on some platforms, it will
not work on all, if you try to access this function from a component.

The showOSAlert is thought for emergency purposes only, where the startup of the
application is not working.

When you print, the application is fully functional. There is no need to use
showOSAlert. Please use the nsIPromptService to display error messages.

Strings that are displayed to the user must go into the bundle files where they
can be localized.

Comment 11

[Roland Mainz]

Attachment: Better patch per kai's comments

Comment 12

[Roland Mainz]

kai:
OK... I replaced the ShowOSAlert() with that service stuff you've suggested...
thanks for the hint... :-)

> Strings that are displayed to the user must go into the bundle files where 
> they can be localized.

Can you point me to an example in CVS how to do this ? Currently I do not have
any clue how to implement this (and... is it possible to forward this task to
another bug, please ?) ... ;-(

Comment 13

[(Don't use, old account) - Kai Engert]

Roland: while I can give you some tips, please note that my review will not be
enough, I think you will need review from the module owners of the content / gfx
directories.

For string resources, please have a look at
http://www.mozilla.org/projects/intl/string-resources.html . My understanding
is, hard coded UI in source files are generally not accept, and the
superreviewers will most likely not accept it. For examples search lxr for
GetStringFromName or FormatStringFromName.

By the way, I noticed some typos:
- printter => printer
- availuable => available

Comment 14

[Roland Mainz]

kaie:
thanks for the tips... I am working on the new patch ...

Comment 15

[Roland Mainz]

Attachment: New patch from 2001-09-05-08-trunk (may require patch from bug 90380 to apply)

Comment 16

[Roland Mainz]

Filed new patch, incl. localisation support.

Requesting r=/sr= ...

Comment 17

[(Don't use, old account) - Kai Engert]

1)
I don't know how others think about it, but I personally don't like the
statement:

  if (NS_SUCCEEDED(rv = mPresContext->GetDeviceContext(getter_AddRefs(dx))))

I'd prefer to split this up into separate lines, it's clearer to read.

  rv = mPresContext->GetDeviceContext(getter_AddRefs(dx));
  if (NS_SUCCEEDED(rv))

You do the same multiple times, please consider splitting all of them.

2)
    switch(saved_printerror)
    {
      ...

Please include a default case and make sure that the following GetStringFromName
will not crash in case you have not found a valid string name.

You try to obtain the parent of the window and dereference the pointer. To
prevent a null crash, please check that you indeed were able to obtain the
parent pointer.

3)
I can't judge whether 

  #define NS_ERROR_GFX_PRINTER_BASE (1)

is a good value. Please make sure you checked that.


If you do the above, r= kaie.

Please note that you still need r= from a module owner of the print module, e.g.
dcone.

Comment 18

[Roland Mainz]

Attachment: New patch for 2001-09-08-08-trunk per kaie's comments

Comment 19

[Roland Mainz]

kaie wrote:

> I don't know how others think about it, but I personally don't like the
> statement:
> 
>   if (NS_SUCCEEDED(rv = mPresContext->GetDeviceContext(getter_AddRefs(dx))))
> 
> I'd prefer to split this up into separate lines, it's clearer to read.
> You do the same multiple times, please consider splitting all of them.

Fixed.

>     switch(saved_printerror)
>     {
>       ...
> 
> Please include a default case and make sure that the following 
> GetStringFromName
> will not crash in case you have not found a valid string name.
> 
> You try to obtain the parent of the window and dereference the pointer. To
> prevent a null crash, please check that you indeed were able to obtain the
> parent pointer.

Fixed.

> I can't judge whether 
> 
>   #define NS_ERROR_GFX_PRINTER_BASE (1)
> 
> is a good value. Please make sure you checked that.

AFAIK it is not a "perfect" value - but currently I am the only one who's using
the GFX module error base. If there is a problem in the future with this thing a
simple one-line patch can adjust this magic knob to a more usefull value...

> If you do the above, r= kaie.

thanks!

> Please note that you still need r= from a module owner of the print module, 
> e.g. dcone.

OK... CC:'ing jst for sr= ... AFAIK the moa=dcone can be "done" in parallel ...
:-)

Comment 20

[Johnny Stenback (:jst)]

The code:

  /* create factory (incl. create print dialog) */
  nsCOMPtr<nsIDeviceContextSpecFactory> factory =
          do_CreateInstance(kDeviceContextSpecFactoryCID, &rv);
          
  if (NS_SUCCEEDED(rv) && factory) {
    ...
  }

does a redundant error check, factory will never be non-null when
NS_SUCCEEDED(rv) is true, so checking either one is enough, I'd suggest checking
the pointer for a non-null value only. There are a few occurances of code like
this in the patch.

The code:

+  const PRUnichar *stringName = nsnull;
+
+  switch(printerror)
+  {
+#define NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(nserr) case nserr: stringName =
NS_LITERAL_STRING(#nserr).get(); break;
+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_CMD_NOT_FOUND)
...
+  }
+
+  myStringBundle->GetStringFromName(stringName, &msg);

is a crash waiting to happen, you make stringName point to the internal buffer
of a string object (NS_LITERAL_STRING().get()) which goes out of scope before
you use the pointer, not good, this needs to change. Just use a local
nsAutoString and copy the string into the local string and use that later on...

This code uses nsCOMPtr's, but it still does a manual QI:

+    nsCOMPtr<nsIDOMWindowInternal> parent;
+    nsCOMPtr<nsIDOMWindow> active;
+    wwatch->GetActiveWindow(getter_AddRefs(active));
+    if (active) {
+      active->QueryInterface(NS_GET_IID(nsIDOMWindowInternal),
getter_AddRefs(parent));
+    
+      if (parent) {

replace that with:

+    nsCOMPtr<nsIDOMWindow> active;
+    wwatch->GetActiveWindow(getter_AddRefs(active));
+
+    nsCOMPtr<nsIDOMWindowInternal> parent(do_QueryInterface(active));
+    
+    if (parent) {

Note that do_QueryInterface() is null-safe, so no need to do an if (ptr) check
before passing it to do_QueryInterface().

- Since you're already changing the code in nsDeviceContextSpecFactoryG.cpp you
might as well go ahead and make the code use nsCOMPtr's and do_QueryInterface()
in stead of what it does now. Up to you.

- In nsDeviceContextPS::GetDeviceSurfaceDimensions() rename 'res' to 'rv' since
you're already cleaning it up a bit? Up to you.

- Please split up return value assignment and return value check onto two lines,
it's much more readable that way:

+  if (NS_FAILED(rv = SetupWindow(rect.x, rect.y, rect.width, rect.height)))
+    return rv;

Comment 21

[Roland Mainz]

Attachment: New patch for 2001-09-08-08-trunk per jst's comments

Comment 22

[Roland Mainz]

jst wrote:
>   /* create factory (incl. create print dialog) */
>   nsCOMPtr<nsIDeviceContextSpecFactory> factory =
>           do_CreateInstance(kDeviceContextSpecFactoryCID, &rv);
>           
>   if (NS_SUCCEEDED(rv) && factory) {
>     ...
>   }
> 
> does a redundant error check, factory will never be non-null when
> NS_SUCCEEDED(rv) is true, so checking either one is enough, I'd suggest 
> checking
> the pointer for a non-null value only. There are a few occurances of code like
> this in the patch.

Fixed.

> +  const PRUnichar *stringName = nsnull;
> +
> +  switch(printerror)
> +  {
> +#define NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(nserr) case nserr: stringName =
> NS_LITERAL_STRING(#nserr).get(); break;
> +   NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_CMD_NOT_FOUND)
> ...
> +  }
> +
> +  myStringBundle->GetStringFromName(stringName, &msg);
> 
> is a crash waiting to happen, you make stringName point to the internal buffer
> of a string object (NS_LITERAL_STRING().get()) which goes out of scope before
> you use the pointer, not good, this needs to change. Just use a local
> nsAutoString and copy the string into the local string and use that later 
> on...

Fixed.

> This code uses nsCOMPtr's, but it still does a manual QI:
> 
> +    nsCOMPtr<nsIDOMWindowInternal> parent;
> +    nsCOMPtr<nsIDOMWindow> active;
> +    wwatch->GetActiveWindow(getter_AddRefs(active));
> +    if (active) {
> +      active->QueryInterface(NS_GET_IID(nsIDOMWindowInternal),
> getter_AddRefs(parent));
> +    
> +      if (parent) {
> 
> replace that with:
> 
> +    nsCOMPtr<nsIDOMWindow> active;
> +    wwatch->GetActiveWindow(getter_AddRefs(active));
> +
> +    nsCOMPtr<nsIDOMWindowInternal> parent(do_QueryInterface(active));
> +    
> +    if (parent) {
> 
> Note that do_QueryInterface() is null-safe, so no need to do an if (ptr) check
> before passing it to do_QueryInterface().

Fixed.

> - Since you're already changing the code in nsDeviceContextSpecFactoryG.cpp 
> you might as well go ahead and make the code use nsCOMPtr's and 
> do_QueryInterface() in stead of what it does now. Up to you.

Fixed.
However, there is far more rubbish in PS&&GTK modules which should be
cleaned-up...

> - In nsDeviceContextPS::GetDeviceSurfaceDimensions() rename 'res' to 'rv' 
> since you're already cleaning it up a bit? Up to you.

Fixed.

> - Please split up return value assignment and return value check onto two 
> lines, it's much more readable that way:
> 
> +  if (NS_FAILED(rv = SetupWindow(rect.x, rect.y, rect.width, rect.height)))
> +    return rv;

Fixed.

Comment 23

[Roland Mainz]

dcone/suyay:
Can I get a r=/moa= from you, please ?

Comment 24

[Johnny Stenback (:jst)]

sr=jst

Comment 25

[Roland Mainz]

dcone:
OK... I have everything (r=/sr=) except the moa= from you ...

Comment 26

[Roland Mainz]

Comment on attachment 49024 [details] [diff] [review]
New patch for 2001-09-08-08-trunk per jst's comments

summary:
r=kaie
sr=jst

Waiting for moa= ...

Comment 27

[dcone (gone)]

r=dcone

Comment 28

[Roland Mainz]

dcone:
Thanks!

----

CC:'ing [e-mail] for checkin.

Comment 29

[Mike Kaply [:mkaply]]

Fix checked in.

Comment 30

[Jon Smirl]

windows makefile in mozilla\content\base\src needs to have windowwatcher added 
to it's REQUIRES list.

Comment 31

[Roland Mainz]

Per
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/base/src&command=DIFF_FRAMESET&file=makefile.win&rev2=1.11&rev1=1.10
mkaply added "windowwatcher" to REQUIRES (thx! :-)

However, "intl" is still missing... mkaply, could you fix this, please (my tree
is still rebuilding and needs ~3-4 hours from now...) ?

Comment 32

[Roland Mainz]

mkaply:
Unix makefile (mozilla/content/base/src/Makefile.in) needs the "windowwatcher"
thing in REQUIRES, too... ;-(

Comment 33

[Mike Kaply [:mkaply]]

OK, really fixed now. REQUIRES change in.

Comment 34

[sujay]

Roland/Kaply, please verify and mark verified-fixed...thanks.

Comment 35

[Roland Mainz]

[e-mail]:
> Roland/Kaply, please verify and mark verified-fixed...thanks.

Uhm... you've marked it VERIFIED/FIXED already... :-)

Anyway... patch works OK except bug 101299 ("Cancelling print dialog crashes
mozilla" - caused by broken error checking in gfx/src/windows/ code. I have a
fix for that bug - but I need someone who tests it...).