Seamonkey crashes when following file is attached to a sent email

VERIFIED FIXED in M18

Status

MailNews Core
Networking
P1
critical
VERIFIED FIXED
17 years ago
7 years ago

People

(Reporter: jefft, Assigned: jefft)

Tracking

({crash})

Trunk
x86
Windows NT
crash

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
Created attachment 17731 [details]
Crash page used for Expense Report demo
(Assignee)

Comment 2

17 years ago
Created attachment 17733 [details] [diff] [review]
A fix
(Assignee)

Comment 3

17 years ago
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
(Assignee)

Comment 4

17 years ago
Adding bijals ...

Comment 5

17 years ago
Is there any connection to bug 53447?
(Assignee)

Comment 6

17 years ago
No. This is a different bug. It has something to do with resolving the relative
url of an attached html document.
(Assignee)

Comment 7

17 years ago
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=???

Updated

17 years ago
QA Contact: esther → pmock
(Assignee)

Comment 8

17 years ago
Created attachment 17800 [details] [diff] [review]
More bullet proof fix with diff -u

Comment 9

17 years ago
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


(Assignee)

Comment 10

17 years ago
> 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.



Comment 11

17 years ago
>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.
(Assignee)

Comment 12

17 years ago
That's the answer I want for the Friday's question.

Comment 13

17 years ago
=). So make that your implementation of nsMsgMailNewsUrl::Clone and get rid of
the other 3 implementations. Sound good?
 
(Assignee)

Comment 14

17 years ago
Yes, sounds perfect. Coded and testing now...
(Assignee)

Comment 15

17 years ago
Created attachment 17813 [details] [diff] [review]
an improved patch; please review ASAP; hope to get in today
(Assignee)

Comment 16

17 years ago
Adding potential reviewers ....

Comment 17

17 years ago
sr=mscott on the new patch at 15:14. nice job on the quick turn around. 
(Assignee)

Comment 18

17 years ago
rtm+ .....
Whiteboard: [rtm need info] sr=alecf r=??? → [rtm+] sr=alecf, mscott r=bienvenu

Comment 19

17 years ago
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
(Assignee)

Comment 20

17 years ago
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
(Assignee)

Comment 22

17 years ago
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.
(Assignee)

Comment 23

17 years ago
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

Comment 24

17 years ago
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]
(Assignee)

Comment 25

17 years ago
Created attachment 18420 [details] [diff] [review]
new patch with nsresult passed in for NS_GetWeakReference()
(Assignee)

Comment 26

17 years ago
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

Comment 28

17 years ago
sr=mscott
(Assignee)

Comment 29

17 years ago
Thanks, brendan & mscott. Moving to [rtm+] again. 
Whiteboard: [rtm need info] → [rtm+] r=brendan, sr=mscott

Comment 30

17 years ago
candidate limbo.

Comment 31

17 years ago
rtm-, not seeing an argument that this happens often.
Whiteboard: [rtm+] r=brendan, sr=mscott → [rtm-] r=brendan, sr=mscott

Comment 32

17 years ago
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

Comment 33

17 years ago
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

Comment 34

17 years ago
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.
(Assignee)

Comment 35

17 years ago
Fix checked in to trunk. Mark it as fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 36

17 years ago
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.