Closed Bug 57571 Opened 25 years ago Closed 25 years ago

Seamonkey crashes when following file is attached to a sent email

Categories

(MailNews Core :: Networking, defect, P1)

Product:
MailNews Core
Component:
Networking
Platform:
x86
Windows NT
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: jefft, Assigned: jefft)

Details

(Keywords: crash, Whiteboard: [rtm-] r=brendan, sr=mscott)

Attachments

(5 files)

Crash page used for Expense Report demo
20.86 KB, text/html
Details
A fix
8.55 KB, patch
Details | Diff | Splinter Review
More bullet proof fix with diff -u
9.22 KB, patch
Details | Diff | Splinter Review
an improved patch; please review ASAP; hope to get in today
6.81 KB, patch
Details | Diff | Splinter Review
new patch with nsresult passed in for NS_GetWeakReference()
7.17 KB, patch
Details | Diff | Splinter Review
This bug should be in bugzilla instead of bugscape. Steps: 1) Launch Communicator Messenger 2) Attach file provided in this bug and send email to yourself 3) Launch Seamonkey Mail 4) Open email with attachment Actual Results: Crash! Expected Results: Read email and get attachment Build Date/Platform: WinNT using 2000101308 ------- Additional Comments From bijals@netscape.com 2000-10-18 16:54 ------- Created an attachment (id=497) Crasher page used for Expense Report demo ------- Additional Comments From Steve Elmer 2000-10-18 18:56 ------- Jeff, is this one for you? Time is short to repro, debug and fix. Please move to proper owner ASAP if it's not yours. Thanks! Steve ------- Additional Comments From Jeff Tsai 2000-10-18 19:37 ------- Looks like we have a staled aImapMailFolderSink. Adding mscott. This may have something to do with nsImapMockChannel. - aImapMailFolderSink 0x0012e274 - 0x063870f4 - nsISupports {...} - __vfptr 0xdddddddd [0x0] CXX0030: Error: expression cannot be evaluated [0x1] CXX0030: Error: expression cannot be evaluated [0x2] CXX0030: Error: expression cannot be evaluated + *aImapMailFolderSink 0x063870f4 + this 0x06387640 ns_if_addref(nsIImapMailFolderSink * 0x063870f4) line 1101 + 15 bytes nsImapUrl::GetImapMailFolderSink(nsImapUrl * const 0x06387640, nsIImapMailFolderSink * * 0x0012e274) line 165 + 19 bytes nsImapProtocol::SetupSinkProxy() line 458 + 50 bytes nsImapProtocol::LoadUrl(nsImapProtocol * const 0x00e8a028, nsIURI * 0x06387644, nsISupports * 0x00000000) line 1343 nsImapIncomingServer::GetImapConnectionAndLoadUrl(nsImapIncomingServer * const 0x04d0a458, nsIEventQueue * 0x00a81070, nsIImapUrl * 0x06387640, nsISupports * 0x00000000) line 417 + 36 bytes nsImapMockChannel::AsyncRead(nsImapMockChannel * const 0x063874a0, nsIStreamListener * 0x03c6c190, nsISupports * 0x00000000) line 6896 + 55 bytes nsDocumentOpenInfo::Open(nsIChannel * 0x063874a0, int 0x00000000, const char * 0x00000000, nsISupports * 0x03c6b3a4) line 200 + 21 bytes nsURILoader::OpenURIVia(nsURILoader * const 0x01a4cae0, nsIChannel * 0x063874a0, int 0x00000000, const char * 0x00000000, nsISupports * 0x03c6b3a4, unsigned int 0x00000000) line 608 + 29 bytes nsURILoader::OpenURI(nsURILoader * const 0x01a4cae0, nsIChannel * 0x063874a0, int 0x00000000, const char * 0x00000000, nsISupports * 0x03c6b3a4) line 507 ImageNetContextImpl::GetURL(ilIURL * 0x06387780, ImgCachePolicy DONT_USE_IMG_CACHE, ilINetReader * 0x03c6df10, int 0x00000000) line 819 + 56 bytes IL_GetImage(const char * 0x06387df0, _IL_GroupContext * 0x040e5d10, OpaqueObserverList * 0x06387d10, _NI_IRGB * 0x0012e654, unsigned int 0x00000000, unsigned int 0x00000000, unsigned int 0x00000000, void * 0x040e59f0, nsIImageRequestObserver * 0x06387f14) line 2053 + 37 bytes ImageRequestImpl::Init(void * 0x040e5d10, const char * 0x06387df0, nsIImageRequestObserver * 0x06387f14, const unsigned int * 0x04151524, unsigned int 0x00000000, unsigned int 0x00000000, unsigned int 0x00000000, ilINetContext * 0x040e59f0) line 260 + 53 bytes ImageGroupImpl::GetImage(const char * 0x06387df0, nsIImageRequestObserver * 0x06387f14, const unsigned int * 0x04151524, unsigned int 0x00000000, unsigned int 0x00000000, unsigned int 0x00000000) line 283 + 46 bytes nsFrameImageLoader::Init(nsFrameImageLoader * const 0x06387f10, nsIPresContext * 0x0630c8b0, nsIImageGroup * 0x040e7dc0, const nsString & {...}, const unsigned int * 0x04151524, const nsSize * 0x00000000, nsIFrame * 0x043b2574, nsImageAnimation eImageAnimation_Normal, unsigned int (nsIPresContext *, nsIFrameImageLoader *, nsIFrame *, void *, unsigned int)* 0x00000000, void * ...) l nsPresContext::StartLoadImage(nsPresContext * const 0x0630c8b0, const nsString & {...}, const unsigned int * 0x04151524, const nsSize * 0x00000000, nsIFrame * 0x043b2574, unsigned int (nsIPresContext *, nsIFrameImageLoader *, nsIFrame *, void *, unsigned int)* 0x00000000, void * 0x00000000, void * 0x043b2574, nsIFrameImageLoader * * 0x0012e844) line 1006 + 66 bytes nsCSSRendering::PaintBackground(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, nsIFrame * 0x043b2574, const nsRect & {...}, const nsRect & {...}, const nsStyleColor & {...}, const nsStyleSpacing & {...}, int 0x00000000, int 0x00000000) line 2108 + 80 bytes nsTableCellFrame::Paint(nsTableCellFrame * const 0x043b2574, nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 311 + 37 bytes nsTableRowFrame::PaintChildren(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 599 nsTableRowFrame::Paint(nsTableRowFrame * const 0x043b2524, nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 552 nsTableRowGroupFrame::PaintChildren(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 273 nsTableRowGroupFrame::Paint(nsTableRowGroupFrame * const 0x043b24e0, nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 227 nsContainerFrame::PaintChild(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x043b24e0, nsFramePaintLayer eFramePaintLayer_Underlay) line 211 nsContainerFrame::PaintChildren(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 155 nsTableFrame::Paint(nsTableFrame * const 0x043b2410, nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1370 nsContainerFrame::PaintChild(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x043b2410, nsFramePaintLayer eFramePaintLayer_Underlay) line 211 nsTableOuterFrame::Paint(nsTableOuterFrame * const 0x043b23bc, nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 352 nsContainerFrame::PaintChild(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x043b23bc, nsFramePaintLayer eFramePaintLayer_Underlay) line 211 nsBlockFrame::PaintChildren(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 6408 nsBlockFrame::Paint(nsBlockFrame * const 0x043b21b4, nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 6286 nsContainerFrame::PaintChild(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x043b21b4, nsFramePaintLayer eFramePaintLayer_Underlay) line 211 nsBlockFrame::PaintChildren(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 6408 nsBlockFrame::Paint(nsBlockFrame * const 0x043b1f60, nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 6286 nsContainerFrame::PaintChild(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x043b1f60, nsFramePaintLayer eFramePaintLayer_Underlay) line 211 nsBlockFrame::PaintChildren(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 6408 nsBlockFrame::Paint(nsBlockFrame * const 0x04279ef0, nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 6286 nsContainerFrame::PaintChild(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x04279ef0, nsFramePaintLayer eFramePaintLayer_Underlay) line 211 nsContainerFrame::PaintChildren(nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 155 nsHTMLContainerFrame::Paint(nsHTMLContainerFrame * const 0x04278ffc, nsIPresContext * 0x0630c8b0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 108 PresShell::Paint(PresShell * const 0x03cc6cd4, nsIView * 0x040b8f80, nsIRenderingContext & {...}, const nsRect & {...}) line 4242 + 34 bytes nsView::Paint(nsView * const 0x040b8f80, nsIRenderingContext & {...}, const nsRect & {...}, unsigned int 0x00000080, int & 0x100283b5) line 284 nsViewManager2::RenderDisplayListElement(DisplayListElement2 * 0x06386550, nsIRenderingContext & {...}) line 859 nsViewManager2::RenderViews(nsIView * 0x040bc6b0, nsIRenderingContext & {...}, const nsRect & {...}, int & 0x00000000) line 806 nsViewManager2::Refresh(nsIView * 0x040bc6b0, nsIRenderingContext * 0x06385eb0, const nsRect * 0x0012f6f8, unsigned int 0x00000001) line 686 nsViewManager2::DispatchEvent(nsViewManager2 * const 0x03c19de0, nsGUIEvent * 0x0012f838, nsEventStatus * 0x0012f73c) line 1352 HandleEvent(nsGUIEvent * 0x0012f838) line 68 nsWindow::DispatchEvent(nsWindow * const 0x040b9054, nsGUIEvent * 0x0012f838, nsEventStatus & nsEventStatus_eIgnore) line 682 + 10 bytes nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f838, nsEventStatus & nsEventStatus_eIgnore) line 708 nsWindow::OnPaint() line 3703 + 28 bytes nsWindow::ProcessMessage(unsigned int 0x0000000f, unsigned int 0x00000000, long 0x00000000, long * 0x0012fbe8) line 2806 + 17 bytes nsWindow::WindowProc(HWND__ * 0x00410300, unsigned int 0x0000000f, unsigned int 0x00000000, long 0x00000000) line 951 + 27 bytes USER32! 77e719d0() USER32! 77e71982() NTDLL! 77f763a3() ------- Additional Comments From Jeff Tsai 2000-10-19 12:29 ------- Looks like we were cooking up wrong image url in nsTableCellFrame::Paint(). Which cause a new ImapMailFolder gets created and destroyed, hence the nsImapMailFolderSink. Adding pnunn & rhp so they may have some ideas what went wrong.... - mBackgroundImage {...} + basic_nsAWritableString<unsigned short> {...} - nsStr {...} mLength 61 mCapacity 61 mCharSize eTwoByte mOwnsBuffer 1 + mStr 0x03c93130 "i" + mUStr 0x03c93130 "imap://jefft@nsmail-2:143/fetch%3EUID%3E/images/cathedral.jpg" mCursor 1 '' nsFrameImageLoader::Init(nsFrameImageLoader * const 0x03e63da0, nsIPresContext * 0x02c4f960, nsIImageGroup * 0x03e46290, const nsString & {...}, const unsigned int * 0x03c94d64, const nsSize * 0x00000000, nsIFrame * 0x013e34bc, nsImageAnimation eImageAnimation_Normal, unsigned int (nsIPresContext *, nsIFrameImageLoader *, nsIFrame *, void *, unsigned int)* 0x00000000, void * ...) l nsPresContext::StartLoadImage(nsPresContext * const 0x02c4f960, const nsString & {...}, const unsigned int * 0x03c94d64, const nsSize * 0x00000000, nsIFrame * 0x013e34bc, unsigned int (nsIPresContext *, nsIFrameImageLoader *, nsIFrame *, void *, unsigned int)* 0x00000000, void * 0x00000000, void * 0x013e34bc, nsIFrameImageLoader * * 0x0012e844) line 1006 + 66 bytes nsCSSRendering::PaintBackground(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, nsIFrame * 0x013e34bc, const nsRect & {...}, const nsRect & {...}, const nsStyleColor & {...}, const nsStyleSpacing & {...}, int 0, int 0) line 2108 + 80 bytes nsTableCellFrame::Paint(nsTableCellFrame * const 0x013e34bc, nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 311 + 37 bytes nsTableRowFrame::PaintChildren(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 599 nsTableRowFrame::Paint(nsTableRowFrame * const 0x013e346c, nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 552 nsTableRowGroupFrame::PaintChildren(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 273 nsTableRowGroupFrame::Paint(nsTableRowGroupFrame * const 0x013e3428, nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 227 nsContainerFrame::PaintChild(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x013e3428, nsFramePaintLayer eFramePaintLayer_Underlay) line 211 nsContainerFrame::PaintChildren(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 155 nsTableFrame::Paint(nsTableFrame * const 0x013e3358, nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1370 nsContainerFrame::PaintChild(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x013e3358, nsFramePaintLayer eFramePaintLayer_Underlay) line 211 nsTableOuterFrame::Paint(nsTableOuterFrame * const 0x013e3304, nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 352 nsContainerFrame::PaintChild(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x013e3304, nsFramePaintLayer eFramePaintLayer_Underlay) line 211 nsBlockFrame::PaintChildren(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 6408 nsBlockFrame::Paint(nsBlockFrame * const 0x013e30fc, nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 6286 nsContainerFrame::PaintChild(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x013e30fc, nsFramePaintLayer eFramePaintLayer_Underlay) line 211 nsBlockFrame::PaintChildren(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 6408 nsBlockFrame::Paint(nsBlockFrame * const 0x013e2ea8, nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 6286 nsContainerFrame::PaintChild(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x013e2ea8, nsFramePaintLayer eFramePaintLayer_Underlay) line 211 nsBlockFrame::PaintChildren(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 6408 nsBlockFrame::Paint(nsBlockFrame * const 0x013efe20, nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 6286 nsContainerFrame::PaintChild(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x013efe20, nsFramePaintLayer eFramePaintLayer_Underlay) line 211 nsContainerFrame::PaintChildren(nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 155 nsHTMLContainerFrame::Paint(nsHTMLContainerFrame * const 0x013eef2c, nsIPresContext * 0x02c4f960, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 108 PresShell::Paint(PresShell * const 0x032da7d4, nsIView * 0x03c195c0, nsIRenderingContext & {...}, const nsRect & {...}) line 4242 + 34 bytes nsView::Paint(nsView * const 0x03c195c0, nsIRenderingContext & {...}, const nsRect & {...}, unsigned int 128, int & 268600245) line 284 nsViewManager2::RenderDisplayListElement(DisplayListElement2 * 0x03e62cf0, nsIRenderingContext & {...}) line 859 nsViewManager2::RenderViews(nsIView * 0x03c127a0, nsIRenderingContext & {...}, const nsRect & {...}, int & 0) line 806 nsViewManager2::Refresh(nsIView * 0x03c127a0, nsIRenderingContext * 0x03e63aa0, const nsRect * 0x0012f6f8, unsigned int 1) line 686 nsViewManager2::DispatchEvent(nsViewManager2 * const 0x032da180, nsGUIEvent * 0x0012f838, nsEventStatus * 0x0012f73c) line 1352 HandleEvent(nsGUIEvent * 0x0012f838) line 68 nsWindow::DispatchEvent(nsWindow * const 0x03c12ee4, nsGUIEvent * 0x0012f838, nsEventStatus & nsEventStatus_eIgnore) line 682 + 10 bytes nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f838, nsEventStatus & nsEventStatus_eIgnore) line 708 nsWindow::OnPaint() line 3711 + 28 bytes nsWindow::ProcessMessage(unsigned int 15, unsigned int 0, long 0, long * 0x0012fbe8) line 2814 + 17 bytes nsWindow::WindowProc(HWND__ * 0x0021047a, unsigned int 15, unsigned int 0, long 0) line 958 + 27 bytes USER32! 77e71303() USER32! 77e71962() NTDLL! 77f763ef() ------- Additional Comments From Steve Elmer 2000-10-19 12:52 ------- Let's see if we can get a short safe fix for this one. Marking rtm need info. ------- Additional Comments From Jeff Tsai 2000-10-19 15:06 ------- Pam, any idea how StyleColorImpl::mBackgroundImage url gets generated? Thanks, To see the problem you have to save the attachment as a html file then attach to your mail. ------- Additional Comments From Jeff Tsai 2000-10-19 17:33 ------- Created an attachment (id=519) use nsWeakPtr to prevent the crash ------- Additional Comments From Jeff Tsai 2000-10-19 17:49 ------- Thanks to Seth's help suggesting using nsWeakPtr to get rid of the crash. There are couple problem found here: 1) The original html has few relative image url in it. Either mime or image lib convert the relative url into an imap absolute url point to a non-existing mailbox folder. 2) Because of the non-existing imap mail folder url causes serveral dangling imap sink pointers gets attached to the imap url. We crash when try to run the imap url with the dangling sink pointers. Using weak pointer fix the crashing problem. However, the user also receives an error message from the server complaining about non-existing mailbox folder. The crash problem can be easily created with relative images in a html page and then attach to a mail message. ------- Additional Comments From Jeff Tsai 2000-10-20 08:04 ------- Rich, do we munge the relative image url into a absolute image url in mime? Where is the code? Thanks, ------- Additional Comments From Jeff Tsai 2000-10-20 10:20 ------- Adding jst... soliciting for help... Looks like the relative url is resolved in nsHTMLContentSink instead of libmime. jst, do you know where I should look into in order to figure out how we resolve the relative url into a absolute url? Thanks, ------- Additional Comments From pnunn 2000-10-20 11:05 ------- Jeff: Url resolutions happen in netlib and I have just seen another bug where we resolve an image url on a page incorrectly. (#56934). But since this is mail/attachments....I dunno. There may well be another mechanism in addition to the netlib url resolution. -p ------- Additional Comments From Johnny Stenback 2000-10-20 15:10 ------- The code that does relative --> absolute url conversion when loading images is the image loader, it's called at layout/html/base/src/nsImageFrame.cpp:184 (among other places), and the image loader (nsHTMLImageLoader) does a NS_MakeAbsoluteURI()... ------- Additional Comments From Jeff Tsai 2000-10-20 15:53 ------- Yes, I figured that out too. Thanks much, jst. CSSParserImpl::ParseURL() calls mURL->Clone() to create an instance of nsStdURL. It then calls NS_MakeAbsoluteURI() to resolve the relative url into a absolute url via nsStdURL::Resolve() method. Sounds to me to cleanly fix this problem would be to implement nsMsgMailNewsUrl::Clone() method instead of simply using the base class method. Of course nsMsgMailNewsUrl needs to have its own version of nsMsgMailNewsUrl::Resolve() method implemented. ------- Additional Comments From Jeff Tsai 2000-10-20 18:29 ------- Created an attachment (id=526) a save and good fix ------- Additional Comments From Jeff Tsai 2000-10-20 18:32 ------- We have to deal with all mailnews urls to resolve the relative url problem. Using weak pointers in imap url is a must. The fix looks much but essentially is not. It's straight forward and safe. Imap, Pop3 and Nntp all tested fine. ------- Additional Comments From Steve Elmer 2000-10-20 19:53 ------- Jeff, what are the worst possible side-effects if there's a serious bug hidden in this patch? I think we should get this on the trunk and have the QA team hit it pretty hard just to be safe. Please include a description of the testing done to prove that this is the correct fix and that no regressions happen. Great job tracking down the cause and whipping up a fix! Please move it to rtm+ when the reviews are in. Thanks! ------- Additional Comments From Jeff Tsai 2000-10-21 10:00 ------- No hidden bug I can think of. Only the good things. Tests have been done by sending/posting a message with the Expense Report attachment to Imap, Pop3, and Nntp servers. We can all display the message without a hiccup. Sending/receiving/replying MHTML mail message also been tested fine. Without the fix, display the troublesome message, on Imap it crashes, on Pop3 it seems okay but hit a few assertions in debug build, on Nntp server it creates a bogus news group on the folder pane selecting the bogus newsgroup crashes the system. Query page Enter new bug Welcome to Bugscape.
Attached patch A fix — — Splinter Review
This bug is moved from bugscape # 2964.
Status: NEW → ASSIGNED
Keywords: crash, rtm
Priority: P3 → P1
Whiteboard: [rtm need info] fix in hand, reviewing...
Target Milestone: --- → M18
Adding bijals ...
Is there any connection to bug 53447?
No. This is a different bug. It has something to do with resolving the relative url of an attached html document.
great! sr=alecf Jeff Tsai wrote: Alec Flett wrote: > Ok, I'll sr= it once the usual set of questions have been answered: > > is the clone() method you're adding necessary to fix the bug? I don't > see any callers here.. > Yes. (The art of polymorphism.) The caller of the Clone() stems from CSSParserImpl::ParseURL() when it tries to resolve the relative url in the attached html attachment. It calls mURL->Clone() to duplicate the current running url (imapUrl/mailboxUrl/nntpUrl) and then call the cloned url's Resolve() method for the absolute url. Our original clone method is implemented in nsMsgMailNewsUrl which calls nsStdURL::Clone() to create an instance of nsStdURL. The nsStdURL::Resolve() put up a bad url. For the imap case, a relative image url "image/cathedral.gif" gets resolved into "imap://nsmail-2/Inbox%3Efetch%3E/image/cathedral.gif" which is wrong. Adding the clone() methods ensure we handle the Resolve() correctly. > Have you verified that all callers to > GetImapMailFoldersSink()/GetImapMessageSink()/GetImapExtensionsSink()/GetImapMiscellaneousSink() > will handle the case of returning nsnull ok? (because that's what > do_QueryReferent will do if the object has gone away) > Yes, we put out an assert and then return error gracefully. This happens not only in imap also in libmime as well. > Would there ever have been a time that a caller to one of the above > messages might have simply been checking for a sink, but not using it? > It wouldn't have crashed before, but it would have different behavior > than it does now, because now the getters are returning null... > No, we never get into this kind of operation unless we are executing an url which points to no where. With the clone methods in place we will seldom get into this kind of situation, but this definitely is a right way to prevent the unforseen crashes. Using the weak pointer is following the good example of nsImapServerSink (I believe you put it up for the use of nsWeakPtr). > What classes are we expecting to implement the weak reference? I can't > find any classes in mailnews/imap/src which inherit from > nsSupportsWeakReference - do all of the sink implementations derive > from existing nsMsgFolder/nsMsgIncomingServer classes? > nsImapMailFolder which is a subclass of nsMsgDBFolder, inherits from nsMsgFolder. It implments nsIImapMailFolderSink, nsIImapMessageSink, nsIImapExtensionSink & nsIImapMiscellaneousSink interfaces. Similar to nsImapIncomingServer which inherits from nsMsgIncomingServer and implements nsIImapServerSink interface. Hope this answers all questions. Thanks for reviewing .... -- Jeff > Thanks.. > Alec > > Jeff Tsai wrote: > >> A duplicate bug has been created on bugzilla - 57571. As brendan pointed >> out this bug should belong to bugzilla not bugscape. Can anyone out >> there give a thumb up or down? Thanks, >> >> -- Jeff >> >> Jeff Tsai wrote: >> >> > >> > http://bugscape.netscape.com/show_bug.cgi?id=2964 >> > >> > We have to deal with all mailnews urls to resolve the relative url >> > problem. >> > Using weak pointers in imap url is a must. This prevent the crash and >> > a right thing to do. The fix may looks much but it is not. It's >> > straight forward and safe. Imap, Pop3 and Nntp all tested fine. Plus >> > sending/replying mhtml messages. >> > >> > I need review and super review. Thanks much, >> > >> > -- Jeff >> > >> >>
Whiteboard: [rtm need info] fix in hand, reviewing... → [rtm need info] sr=alecf r=???
QA Contact: esther → pmock
Thanks for attaching the diff -u patch for me! It makes it so much easier to see what got changed. A couple of comments: You answered Alec's question about callers of the Getter methods: >Yes, we put out an assert and then return error gracefully. This happens >not only in imap also in libmime as well. I see tha the implementation of the get methods assert and return an error. That's great! But I think Alec meant, what about the callers of the get methods. It doesn't matter if we assert and return an error code, what really matters is if the callers attempt to dereference a null pointer =). I know it's a pain but if we are going to consider this for RTM, I think we need to hand check each of the callers of the following methods: GetImapMailFolderSink, GetImapMessageSink, GetImapServerSink, GetImapExtensionsSink, and GetImapMiscellaneousSink to make sure they all perform a null ptr check on the result returned. About the clone methods, I'm a bit confused. Can you show us a "before and after"? i.e. "imap://nsmail-2/Inbox%3Efetch%3E/image/cathedral.gif" is an example of a "before" url that the parser would resolve a relative url to. What does this url look like if you implement ::Clone for all our mailnews url subclasses? To my knowledge, by calling into the base class, we would have created a new url and set the spec on that url to match the current spec. So you'd get the same thing? I was confused here. Also, why do we have 3 identical implementations of clone (imap, mailbox, and news)? I think we should just have one method in the base class nsMsgMailNewsUrl, it would look like the following: GetScheme from the original url. Get the nsIOService and ask for a protocol handler for the given scheme. GetSpec from the original url. Ask the protocol handler to create a new url for the specified spec. One method instead of 3 identical methods. Thanks, -Scott
> GetImapMailFolderSink, GetImapMessageSink, GetImapServerSink, > GetImapExtensionsSink, and GetImapMiscellaneousSink > to make sure they all perform a null ptr check on the result returned. Yes, that what I meant too. They were checked in nsImapProtocol.cpp. > About the clone methods, I'm a bit confused. Can you show us a "before and > after"? i.e. "imap://nsmail-2/Inbox%3Efetch%3E/image/cathedral.gif" is an Before implementing Clone() method: a relative URL "image/cathedral.gif" gets nsStdURL::Resolve() into "imap://nsmail-2/Inbox%3Efetch%3E/image/cathedral.gif" After implementing Clone() methods in imap, pop3 & nntp: a relative URL "image/cathedral.gif" gets nsMsgMailNewsUrl::Resolve() into "image/cathedral.gif" > Also, why do we have 3 identical implementations of clone (imap, mailbox, and > news)? I think we should just have one method in the base class > nsMsgMailNewsUrl, it would look like the following: This is the question I ask you on Friday that I want to be able to create an instance of nsMsgMailNewUrl class. However, I couldn't even after adding class id and contactid for nsMsgMailNewUrl. The easiest way and to be more local to the mailnews I decided to implement three pretty much identical Clone() method. > GetScheme from the original url. > Get the nsIOService and ask for a protocol handler for the given scheme. > GetSpec from the original url. > Ask the protocol handler to create a new url for the specified spec. This does work, nsMsgMailNewsUrl::Clone() gets called to clone the url. However, we turn around calling m_baseURL->Clone() which creates an instance of nsStdURL class. And hence the snow ball.
>This is the question I ask you on Friday that I want to be able to create an >instance of nsMsgMailNewUrl class. However, I couldn't even after adding class >id and contactid for nsMsgMailNewUrl. The easiest way and to be more local to >the mailnews I decided to implement three pretty much identical Clone() method. Okay, trying to add classid and a progid for msgmailnews url isn't the best way to approach the problem. I'd suggest writing nsMsgMailNewsUrl::Clone like I suggested above, go through necko and have them create the url. All our url sub classes already have contract ids with necko based on the protocol scheme. No need to mess around with exposing nsMsgMailNewsUrl at all. I'm not sure why you though you'd be more "local to mailnews" by subclassing it for each of our subclasses. That's why we have nsMsgMailNewsUrl in the first place (to have common mailnews url routines that are shared across imap, mailbox, news urls). Please do that instead of having 3 copies of ::Clone which all look the same. Thanks for the explanation about the resolve problem! That helped.
That's the answer I want for the Friday's question.
=). So make that your implementation of nsMsgMailNewsUrl::Clone and get rid of the other 3 implementations. Sound good?
Yes, sounds perfect. Coded and testing now...
Adding potential reviewers ....
sr=mscott on the new patch at 15:14. nice job on the quick turn around.
rtm+ .....
Whiteboard: [rtm need info] sr=alecf r=??? → [rtm+] sr=alecf, mscott r=bienvenu
Please get this in on the trunk and get some focused testing on it for 24 hrs. Move back to [rtm+] when the test results are described in this bug.
Whiteboard: [rtm+] sr=alecf, mscott r=bienvenu → [rtm need info] sr=alecf, mscott r=bienvenu
Fix checked in to the trunk as wishes. Peter, let's get some tests going tomorrow. Thanks much.
Does the last patch need an r=? I took a look, have a question: + m_imapMailFolderSink = getter_AddRefs(NS_GetWeakReference(aImapMailFolderSink)); return NS_OK; should there be an nsresult passed as the optional (2nd, out) parameter to NS_GetWeakReference, and returned instead of NS_OK? BTW, it looks from http://lxr.mozilla.org/mozilla/source/xpcom/base/nsWeakReference.cpp#80 like the error being masked here, which should be NS_ERROR_OUT_OF_MEMORY, is instead NS_NOINTERFACE. Cc'ing scc for review of the patch, and a spin-off buglet if appropriate. /be
I am assuming no need for r=? since mscott's comments are on the Clone() methods. Good catch on NS_GetWeakReference() I was copying example code. In the case of setting the m_imapMailFolderSink or other sinks I think we should be okay even with a null sink.
All works as expected on trunk. Moving back to [rtm+]...
Whiteboard: [rtm need info] sr=alecf, mscott r=bienvenu → [rtm+] sr=alecf, mscott r=bienvenu
PDT marking [rtm need info]. If there's a change or question on the patch, then we'd like to do the code reviews again.
Whiteboard: [rtm+] sr=alecf, mscott r=bienvenu → [rtm need info]
Can I get another round of r= and sr=? Thanks,
This is really nit-picking, it's an FYI for future reference, not a comment to cause you to redo the patch: there's no point in initializing nsresult rv = NS_OK; before unconditionally calling NS_GetWeakReference(..., &rv) because the latter always sets that out param, if it is non-null. r=brendan@mozilla.org /be
sr=mscott
Thanks, brendan & mscott. Moving to [rtm+] again.
Whiteboard: [rtm need info] → [rtm+] r=brendan, sr=mscott
candidate limbo.
rtm-, not seeing an argument that this happens often.
Whiteboard: [rtm+] r=brendan, sr=mscott → [rtm-] r=brendan, sr=mscott
Umm...it doesn't happen "often" right now, because people aren't trying to openly attack people who are using Netscape 6. This is a highly visible exploit. Moving back to rtm+, and looking for someone else security minded to go up to bat with the PDT. If this gets the ultimate minus, then let us PLEASE make this bug offlimits to outsiders, (including myself :-( ) because otherwise, we're proving people with an IMMEDIATE exploit against NS6.
Whiteboard: [rtm-] r=brendan, sr=mscott → [rtm+] r=brendan, sr=mscott
Denial of service attacks are known for several mail readers. Obscuring the bug is not considered a reasonable approach to blocking even DOS attacks. This patch is fairly large, and we don't have time to get sufficient testing. That is why we were forced to RTM minus it. Pushing back again to RTM minus JCE: Please do not clog our radar by changing this again. If you have a small patch that is interesting, please encourage the owner to push for the patch, and renominate.
Whiteboard: [rtm+] r=brendan, sr=mscott → [rtm-] r=brendan, sr=mscott
jim: 1) The "interesting" patch is already attached. There's nothing I can do patch-wise. 2) Please name the mail-readers that have these "known" crash exploits. (To make my point clearer, please also state how soon afterwords an emergency fix was released by the company.) Once again, I'm not messing with your precious radar any more.
Fix checked in to trunk. Mark it as fixed.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Win32 (2001-07-10-05-0.9.2) This bug is gone.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: