Closed Bug 71587 Opened 24 years ago Closed 24 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: 24 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: