Closed Bug 71587 Opened 24 years ago Closed 23 years ago

moz_2001-03-09-08-Mtrunk crashes when trying to print a page...

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.1

People

(Reporter: roland.mainz, Assigned: roland.mainz)

References

(
URL
)

Details

(Keywords: crash)

Attachments

(4 files)

Patch for 2001-04-17-cvs to fix bug 71587
12.50 KB, patch
Details | Diff | Splinter Review
/mozilla/gfx/src/xlib/nsPrintOptionsXlib.cpp (new file)
1.27 KB, text/plain
Details
/mozilla/gfx/src/xlib/nsPrintOptionsXlib.h (new file)
1.33 KB, text/plain
Details
Patch for 2001-04-23-cvs
31.08 KB, patch
Details | Diff | Splinter Review 
Xlib-Toolkit-based Mozilla crashes while attempting to print a page.
Stack trace from coredump looks like this:
-- snip --
detected a multithreaded program
t@1 (l@1) terminated by signal SEGV (no mapping at the fault address)
0xfed7ff14: _doprnt+0x0178:     ldsb    [%i0], %o1
Current function is nsDeviceContextSpecXlib::Init (optimized)
  167                   sprintf(mPrData.path, printfile);
(/opt/SUNWspro/bin/../WS6U2/bin/sparcv9/dbx) where
current thread: t@1
  [1] _doprnt(0x0, 0xffbec82c, 0x0, 0xffbec8a4, 0xffbec77c, 0x1c5c04), at
0xfed7ff14
  [2] sprintf(0x8bfe70, 0x7fffffff, 0xffbec8c0, 0x4, 0xffbec870, 0xfd6604ee), at
0xfed835bc
=>[3] nsDeviceContextSpecXlib::Init(this = ???, aQuiet = ???) (optimized), at
0xfd64676c (line ~167) in "nsDeviceContextSpecXlib.cpp"
  [4] nsDeviceContextSpecFactoryXlib::CreateDeviceContextSpec(0x8affe0,
0x8213b8, 0xffbec9e8, 0x0, 0xff3df650, 0x1fd) (optimized), at 0xfd645f44
  [5] DocumentViewerImpl::Print(this = ???, aSilent = ???, aFile = ???,
aPrintListener = ???) (optimized), at 0xfd09f3c4 (line ~2430) in
"nsDocumentViewer.cpp"
  [6] GlobalWindowImpl::Print(this = ???) (optimized), at 0xfde75b98 (line
~1749) in "nsGlobalWindow.cpp"
  [7] WindowInternalPrint(cx = ???, obj = ???, argc = ???, argv = ???, rval =
???) (optimized), at 0xfde61efc (line ~3550) in "nsJSWindow.cpp"
  [8] js_Invoke(cx = ???, argc = ???, flags = ???) (optimized), at 0xff044404
(line ~777) in "jsinterp.c"
  [9] js_Interpret(cx = ???, result = ???) (optimized), at 0xff050330 (line
~2670) in "jsinterp.c"
  [10] js_Invoke(cx = ???, argc = ???, flags = ???) (optimized), at 0xff044474
(line ~794) in "jsinterp.c"
  [11] js_InternalInvoke(cx = ???, obj = ???, fval = ???, flags = ???, argc =
???, argv = ???, rval = ???) (optimized), at 0xff0446e0 (line ~866) in
"jsinterp.c"
  [12] JS_CallFunctionValue(cx = ???, obj = ???, fval = ???, argc = ???, argv =
???, rval = ???) (optimized), at 0xff01cfc8 (line ~3268) in "jsapi.c"
  [13] nsJSContext::CallEventHandler(this = ???, aTarget = ???, aHandler = ???,
argc = ???, argv = ???, aBoolResult = ???, aReverseReturnResult = ???)
(optimized), at 0xfde550f8 (line ~939) in "nsJSEnvironment.cpp"
  [14] nsJSEventListener::HandleEvent(this = ???, aEvent = ???) (optimized), at
0xfdec202c (line ~149) in "nsJSEventListener.cpp"
  [15] nsEventListenerManager::HandleEventSubType(this = ???, aListenerStruct =
???, aDOMEvent = ???, aCurrentTarget = ???, aSubType = ???, aPhaseFlags = ???)
(optimized), at 0xfcd7fcc0 (line ~838) in "nsEventListenerManager.cpp"
  [16] nsEventListenerManager::HandleEvent(this = ???, aPresContext = ???,
aEvent = ???, aDOMEvent = ???, aCurrentTarget = ???, aFlags = ???, aEventStatus
= ???) (optimized), at 0xfcd81ef4 (line ~1720) in "nsEventListenerManager.cpp"
  [17] nsXULElement::HandleDOMEvent(this = ???, aPresContext = ???, aEvent =
???, aDOMEvent = ???, aFlags = ???, aEventStatus = ???) (optimized), at
0xfcf7eb5c (line ~3632) in "nsXULElement.cpp"
  [18] PresShell::HandleDOMEventWithTarget(this = ???, aTargetContent = ???,
aEvent = ???, aStatus = ???) (optimized), at 0xfc18723c (line ~4991) in
"nsPresShell.cpp"
  [19] nsMenuFrame::Execute(this = ???) (optimized), at 0xfc30288c (line ~1384)
in "nsMenuFrame.cpp"
  [20] nsMenuFrame::HandleEvent(this = ???, aPresContext = ???, aEvent = ???,
aEventStatus = ???) (optimized), at 0xfc2ff320 (line ~376) in "nsMenuFrame.cpp"
  [21] PresShell::HandleEventInternal(this = ???, aEvent = ???, aView = ???,
aFlags = ???, aStatus = ???) (optimized), at 0xfc187118 (line ~4965) in
"nsPresShell.cpp"
  [22] PresShell::HandleEvent(this = ???, aView = ???, aEvent = ???,
aEventStatus = ???, aForceHandle = ???, aHandled = ???) (optimized), at
0xfc186d24 (line ~4912) in "nsPresShell.cpp"
  [23] nsView::HandleEvent(this = ???, event = ???, aEventFlags = ???, aStatus =
???, aForceHandle = ???, aHandled = ???) (optimized), at 0xfc6cf538 (line ~359)
in "nsView.cpp"
  [24] nsView::HandleEvent(this = ???, event = ???, aEventFlags = ???, aStatus =
???, aForceHandle = ???, aHandled = ???) (optimized), at 0xfc6cf4a8 (line ~343)
in "nsView.cpp"
  [25] nsView::HandleEvent(this = ???, event = ???, aEventFlags = ???, aStatus =
???, aForceHandle = ???, aHandled = ???) (optimized), at 0xfc6cf4a8 (line ~343)
in "nsView.cpp"
  [26] nsView::HandleEvent(this = ???, event = ???, aEventFlags = ???, aStatus =
???, aForceHandle = ???, aHandled = ???) (optimized), at 0xfc6cf4a8 (line ~343)
in "nsView.cpp"
  [27] nsViewManager2::DispatchEvent(this = ???, aEvent = ???, aStatus = ???)
(optimized), at 0xfc6e77ac (line ~1417) in "nsViewManager2.cpp"
  [28] HandleEvent(aEvent = ???) (optimized), at 0xfc6ce8ac (line ~67) in
"nsView.cpp"
  [29] nsWidget::DispatchEvent(this = ???, aEvent = ???, aStatus = ???)
(optimized), at 0xfe8bc958 (line ~1287) in "nsWidget.cpp"
  [30] nsWidget::DispatchWindowEvent(this = ???, aEvent = STRUCT) (optimized),
at 0xfe8bc77c (line ~1186) in "nsWidget.cpp"
  [31] nsWidget::DispatchMouseEvent(this = ???, aEvent = STRUCT) (optimized), at
0xfe8bc658 (line ~1165) in "nsWidget.cpp"
  [32] nsAppShell::HandleButtonEvent(event = ???, aWidget = ???) (optimized), at
0xfe8ab7f8 (line ~788) in "nsAppShell.cpp"
  [33] nsAppShell::DispatchXEvent(event = ???) (optimized), at 0xfe8ab098 (line
~643) in "nsAppShell.cpp"
  [34] nsAppShell::Run(this = ???) (optimized), at 0xfe8aaa4c (line ~463) in
"nsAppShell.cpp"
  [35] nsAppShellService::Run(this = ???) (optimized), at 0xfea2e9b4 (line ~407)
in "nsAppShellService.cpp"
  [36] main1(argc = ???, argv = ???, nativeApp = ???) (optimized), at 0x16568
(line ~1014) in "nsAppRunner.cpp"
  [37] main(argc = ???, argv = ???) (optimized), at 0x16fb4 (line ~1310) in
"nsAppRunner.cpp"
-- snip --

CC blizzard the Xlib guru...
->blizzard
Assignee: trudelle → blizzard
Blocks: 76597
Blizzard, do you accept this bug or should I fix this ?
I'd like to see Xlib-toolkit fixed for 0.9 milestone if possible...
You can fix it.
1. OT: Is it possible that all these Xlib-toolkit fixes catches the 0.9 train ?
2. Overtaking...
Assignee: blizzard → Roland.Mainz
Target Milestone: --- → mozilla0.9.1
Accepting - but I need you (blizzard) to checkin the patch, please !!
Status: NEW → ASSIGNED
These patches update print dialog (prefs) handling for Xlib-toolkit.
Note that this patch includes the fix for bug 76597 !!

Need r=/sr=
+  nsDeviceContextSpecXlib *spec=(nsDeviceContextSpecXlib *)aDevice;

Please use an NS_STATIC_CAST(nsDeviceContextSpecXlib *, aDevice) there.  Also,
are you sure that's always an nsDeviceContextSpecXlib?  Static casts like that
are usually bad mojo.

Other than that it looks OK to me.

r=blizzard
> +  nsDeviceContextSpecXlib *spec=(nsDeviceContextSpecXlib *)aDevice;
>
> Please use an NS_STATIC_CAST(nsDeviceContextSpecXlib *, aDevice) there.  Also,
> are you sure that's always an nsDeviceContextSpecXlib?

AFAIK yes.

> Static casts like that are usually bad mojo.

Why ?

----

1. Thanks !
2. Should I file new patches to fix these issues or should this be put into a
new bug ?
3. Who can do sr= for Xlib-toolkit stuff ?
4. Can this catch the 0.9 train, too ?
5. Who can check this in ?
Whiteboard: want for mozilla 0.9
Here you're making two string copies:

+         // convert Unicode strings to cstrings
+         nsAutoString cmdStr;
+         nsAutoString printFileStr;
+         cmdStr        = command;
+         printFileStr  = printfile;
+         char *       pCmdStr       = cmdStr.ToNewCString();
+         char *       pPrintFileStr = printFileStr.ToNewCString();

instead you could simplify this to:

char* pCmdStr = ToNewCString(nsLocalString(command));
char* pPrintFileStr = ToNewCString(nsLocalString(printfile));


+#ifdef USE_XPRINT
+#include "nsDeviceContextSpecXlib.h"

Please don't |#ifdef| a header include when the header is available
unconditionally.  It's a recipe for build bustage.


You've introduced some tabs in nsDeviceContextSpecXlib.cpp.  Please convert
tabs to spaces on at least the lines that you're changing anyway.


+  NS_WITH_SERVICE(nsIPrintOptions, printService, kPrintOptionsCID, &rv);

Equivalent and preferred is:
nsCOMPtr<nsIPrintOptions> printService(do_GetService(kPrintOptionsCID, &rv));


+    nsIDeviceContextXP *dcxp;
+  
+    rv = nsComponentManager::CreateInstance(kCDeviceContextXP,
+                                            nsnull,
+                                            NS_GET_IID(nsIDeviceContextXP),
+                                            (void **)&dcxp);

nsCOMPtr<nsIDeviceContextXP> dcxp(do_CreateInstance(kCDeviceContextXP, &rv));
is preferred (and you would remove the NS_RELEASE(dcxp) below as well since
the nsCOMPtr manages the refcount for you).


+    NS_ASSERTION(NS_SUCCEEDED(rv), "Couldn't create XP Device context");

might you also want:
if (NS_FAILED(rv)) return rv;
here so that you don't crash on out-of-memory?


+    rv = dcxp->QueryInterface(NS_GET_IID(nsIDeviceContext),
+                              (void **)&aContext);

rv = CallQueryInterface(dcxp, aContext);
is a safer form that may be preferred.


Other than that (and blizzard's comment) r=dbaron.
Going to file new patches when my build is "done" here (~14 hours due makedepend
issue on Solaris)...
Target Milestone: mozilla0.9.1 → mozilla0.9
> Here you're making two string copies:
> 
> +         // convert Unicode strings to cstrings
> +         nsAutoString cmdStr;
> +         nsAutoString printFileStr;
> +         cmdStr        = command;
> +         printFileStr  = printfile;
> +         char *       pCmdStr       = cmdStr.ToNewCString();
> +         char *       pPrintFileStr = printFileStr.ToNewCString();
> 
> instead you could simplify this to:
> 
> char* pCmdStr = ToNewCString(nsLocalString(command));
> char* pPrintFileStr = ToNewCString(nsLocalString(printfile));

a) Why is this a simplification ? Your code looks much more compliciated than
the original one... ;-(
b) It does not work because it cannot find ToNewCString() - and lxr isn't a
great help with this... ;-((

> +#ifdef USE_XPRINT
> +#include "nsDeviceContextSpecXlib.h"
> 
> Please don't |#ifdef| a header include when the header is available
> unconditionally.  It's a recipe for build bustage.

Fixed. But I cannot remove all conditional #include's as this causes build
bustage when --with-xprint is _not_ given to configure...

> You've introduced some tabs in nsDeviceContextSpecXlib.cpp.  Please convert
> tabs to spaces on at least the lines that you're changing anyway.
> 
> +  NS_WITH_SERVICE(nsIPrintOptions, printService, kPrintOptionsCID, &rv);
> 
> Equivalent and preferred is:
> nsCOMPtr<nsIPrintOptions> printService(do_GetService(kPrintOptionsCID, &rv));

Fixed.

> +    nsIDeviceContextXP *dcxp;
> +  
> +    rv = nsComponentManager::CreateInstance(kCDeviceContextXP,
> +                                            nsnull,
> +                                            NS_GET_IID(nsIDeviceContextXP),
> +                                            (void **)&dcxp);
> 
> nsCOMPtr<nsIDeviceContextXP> dcxp(do_CreateInstance(kCDeviceContextXP, &rv));
> is preferred (and you would remove the NS_RELEASE(dcxp) below as well since
> the nsCOMPtr manages the refcount for you).

Fixed.

> +    NS_ASSERTION(NS_SUCCEEDED(rv), "Couldn't create XP Device context");
> 
> might you also want:
> if (NS_FAILED(rv)) return rv;
> here so that you don't crash on out-of-memory?

Fixed.

> +    rv = dcxp->QueryInterface(NS_GET_IID(nsIDeviceContext),
> +                              (void **)&aContext);
> 
> rv = CallQueryInterface(dcxp, aContext);
> is a safer form that may be preferred.

See my comment in patch. Something with XPCOM stuff is wrong. I added the new
code but |#if 0|ed it for now...

I am going to file a new patch which fixes this issue for both Xlib-toolkit and
GTK+-toolkit. Qt-toolkit is not included as I do not have Qt on my box - should
I file a seperate bug for this ?



    
    

    
  
Attached new patch (with diff -u -N to include new files). I also killed tabs in
the files I am patching...

Need r= / sr= / a= and someone who checks this in, please...

    
    

    
  
> a) Why is this a simplification ? Your code looks much more compliciated than
> the original one... ;-(

It replaces two string copies with one.

> b) It does not work because it cannot find ToNewCString() - and lxr isn't a
> great help with this... ;-((

#include "nsReadableUtils.h"

But if you wanted to you could probably also replace the whole bit:
+          nsAutoString cmdStr;
+          nsAutoString printFileStr;
+          cmdStr        = command;
+          printFileStr  = printfile;
+          char *pCmdStr = cmdStr.ToNewCString();
+          char *pPrintFileStr = printFileStr.ToNewCString();
+
+          sprintf( mPrData.command, pCmdStr );
+          sprintf( mPrData.path, pPrintFileStr);
+          nsMemory::Free(pCmdStr);
+          nsMemory::Free(pPrintFileStr);


with:
        nsCAutoString commandStr, printFileStr;
        commandStr.AssignWithConversion(command);
        printFileStr.AssignWithConversion(printfile);
        PL_strncpyz(mPrData.command, commandStr.get(), PATH_MAX);
        PL_strncpyz(mPrData.path, pPrintFileStr.get(), PATH_MAX);

which will also avoid a potential buffer overflow, although it would be
even nicer if you just changed UnixPrData::command and UnixPrData::path
to be nsCStrings instead.  Such a change would also fix some potential
buffer overflows here:

+        if ( ( path = PR_GetEnv( "PWD" ) ) == (char *) NULL )
+          if ( ( path = PR_GetEnv( "HOME" ) ) == (char *) NULL )
+            strcpy( mPrData.path, "mozilla.ps" );
+        if ( path != (char *) NULL )
+          sprintf( mPrData.path, "%s/mozilla.ps", path );
+        else
+          return NS_ERROR_FAILURE;

(and in this bit "nsnull" would be preferable to "(char *) NULL)" and
(!(path = ...)) might be better still (although that's somewhat a matter
of preference).  (Yes, I know all you did was reindent this.)


> See my comment in patch. Something with XPCOM stuff is wrong. I added the new
> code but |#if 0|ed it for now...

How does it "not work"?  I'd think nsISupportsUtils.h would already
be included.


    
    

    
  
> > a) Why is this a simplification ? Your code looks much more compliciated
than
> > the original one... ;-(
> 
> It replaces two string copies with one.

And becomes more unreadable...
...question is if this is really neccesary to optimize the hell out of code
which is only called _once_ per print request, copies small strings and is
killed until milestone 0.9.2 anyway... looks like an overkill optimisation for
me...
 
> > b) It does not work because it cannot find ToNewCString() - and lxr isn't a
> > great help with this... ;-((
> 
> #include "nsReadableUtils.h"
> 
> But if you wanted to you could probably also replace the whole bit:
> +          nsAutoString cmdStr;
> +          nsAutoString printFileStr;
> +          cmdStr        = command;
> +          printFileStr  = printfile;
> +          char *pCmdStr = cmdStr.ToNewCString();
> +          char *pPrintFileStr = printFileStr.ToNewCString();
> +
> +          sprintf( mPrData.command, pCmdStr );
> +          sprintf( mPrData.path, pPrintFileStr);
> +          nsMemory::Free(pCmdStr);
> +          nsMemory::Free(pPrintFileStr);
> 
> 
> with:
>         nsCAutoString commandStr, printFileStr;
>         commandStr.AssignWithConversion(command);
>         printFileStr.AssignWithConversion(printfile);
>         PL_strncpyz(mPrData.command, commandStr.get(), PATH_MAX);
>         PL_strncpyz(mPrData.path, pPrintFileStr.get(), PATH_MAX);
> 
> which will also avoid a potential buffer overflow, although it would be
> even nicer if you just changed UnixPrData::command and UnixPrData::path
> to be nsCStrings instead.  Such a change would also fix some potential
> buffer overflows here:

Agreed. But the code is going to vanish anyway with the new print dialog I am
working on. PATH_MAX is usually in the range between 255-1024 which is far away
from what normal user type into that field (isn't the XUL string input limited
to 255 chars anyway ?)...

> +        if ( ( path = PR_GetEnv( "PWD" ) ) == (char *) NULL )
> +          if ( ( path = PR_GetEnv( "HOME" ) ) == (char *) NULL )
> +            strcpy( mPrData.path, "mozilla.ps" );
> +        if ( path != (char *) NULL )
> +          sprintf( mPrData.path, "%s/mozilla.ps", path );
> +        else
> +          return NS_ERROR_FAILURE;
> 
> (and in this bit "nsnull" would be preferable to "(char *) NULL)" and
> (!(path = ...)) might be better still (although that's somewhat a matter
> of preference).  (Yes, I know all you did was reindent this.)

Gnnnn. OK... valid argument...

> > See my comment in patch. Something with XPCOM stuff is wrong. I added the
new
> > code but |#if 0|ed it for now...
> 
> How does it "not work"?  I'd think nsISupportsUtils.h would already
> be included.

As discussed in IRC #mozilla - this does not work for some stupid reason.
Sun Workshop 6 fails with 
-- snip --
"...mozilla/gfx/src/xlib/nsDeviceContextXlib.cpp", line 400: Error: Could not
find a match for
CallQueryInterface<DestinationType>(nsCOMPtr<nsIDeviceContextXP>,
nsIDeviceContext*).
-- snip --

I suggest to keep that |#if 0|-'ed code as a reminder that there is a better way
to do this and try to find the real issue after 0.9 has branched...


    
  
Blocks: 77344

    
    

    
  
Is this still wanted for 0.9? If so, what has to happen next?

    
  
Keywords: crash

    
  
Whiteboard: want for mozilla 0.9
Target Milestone: mozilla0.9 → mozilla0.9.1

    
    

    
  
This bug has been fixed as part of bug 78548.

The only open issue here is that CallQueryInterface<DestinationType> does not
work... any I still have no clue why... ;-(

Anyway... most of the print dialog code will be re-vamped again when the new
unix print dialog will land...

Marking bug as "fixed" to remove it from the 0.9.1 radar...
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
The CallQueryInterface doesn't work because you need |&aContext| rather than
|aContext|.

Did you ever fix the buffer overruns?

    
    

    
  
> The CallQueryInterface doesn't work because you need |&aContext| rather than
> |aContext|.

Mhhh... Workshop usually gives such hints when it doesn't find a match. Seems
this feature didn't work for this case... ;-((

> Did you ever fix the buffer overruns?

Eeeeeekks... I _knew_ that I forgot something... ;-((((( 
I still do 
-- snip --
strcpy(mPrData.command, NS_ConvertUCS2toUTF8(command).get());  
strcpy(mPrData.path,    NS_ConvertUCS2toUTF8(printfile).get());
-- snip --
which is _bad_ if a user types more than some hundred chars in the widgets...

Should I file a new bug for these issues or can I wait until the new print
dialog lands (which will (AFAIK) kill this issue anyway ?) ?


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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: