56361 - Crash when right click save as this image in an imap message

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

I'll attach the image.  it happened for me on winnt and linux.

to reproduce, send yourself the image and then in 6.0, right click on it to save as.

here's the stack:

ns_if_addref(nsIURI * 0x03145384) line 1101 + 15 bytes
nsImapMockChannel::GetURI(nsImapMockChannel * const 0x03146c10, nsIURI * *
0x0012e098) line 6734 + 19 bytes
XPTC_InvokeByIndex(nsISupports * 0x03146c10, unsigned int 11, unsigned int 1,
nsXPTCVariant * 0x0012e098) line 139
nsXPCWrappedNativeClass::CallWrappedMethod(JSContext * 0x04230630,
nsXPCWrappedNative * 0x04648920, const XPCNativeMemberDescriptor * 0x0108f9a8,
nsXPCWrappedNativeClass::CallMode CALL_GETTER, unsigned int 0, long *
0x00000000, long * 0x0012eb58) line 913 + 43 bytes
nsXPCWrappedNativeClass::GetAttributeAsJSVal(JSContext * 0x04230630,
nsXPCWrappedNative * 0x04648920, const XPCNativeMemberDescriptor * 0x0108f9a8,
long * 0x0012eb58) line 922
WrappedNative_GetProperty(JSContext * 0x04230630, JSObject * 0x00fa3978, long
45463760, long * 0x0012eb58) line 272 + 24 bytes
js_Interpret(JSContext * 0x04230630, long * 0x0012ece0) line 2468 + 1603 bytes
js_Invoke(JSContext * 0x04230630, unsigned int 1, unsigned int 2) line 837 + 13
bytes
js_InternalInvoke(JSContext * 0x04230630, JSObject * 0x00f20648, long 16393248,
unsigned int 0, unsigned int 1, long * 0x0012ee78, long * 0x0012ee08) line 909 +
20 bytes
JS_CallFunctionValue(JSContext * 0x04230630, JSObject * 0x00f20648, long
16393248, unsigned int 1, long * 0x0012ee78, long * 0x0012ee08) line 3193 + 31 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x042307e0, void * 0x00f20648,
void * 0x00fa2420, unsigned int 1, void * 0x0012ee78, int * 0x0012ee74, int 0)
line 907 + 33 bytes
nsJSEventListener::HandleEvent(nsIDOMEvent * 0x04646054) line 154 + 64 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x042851e0,
nsIDOMEvent * 0x04646054, nsIDOMEventTarget * 0x04230c90, unsigned int 1,
unsigned int 7) line 788 + 19 bytes
nsEventListenerManager::HandleEvent(nsIPresContext * 0x0427b0c0, nsEvent *
0x0012f3c8, nsIDOMEvent * * 0x0012f38c, nsIDOMEventTarget * 0x04230c90, unsigned
int 7, nsEventStatus * 0x0012f3ec) line 1365 + 39 bytes
GlobalWindowImpl::HandleDOMEvent(GlobalWindowImpl * const 0x04230c80,
nsIPresContext * 0x0427b0c0, nsEvent * 0x0012f3c8, nsIDOMEvent * * 0x0012f38c,
unsigned int 1, nsEventStatus * 0x0012f3ec) line 510
DocumentViewerImpl::LoadComplete(DocumentViewerImpl * const 0x0427a2b0, unsigned
int 0) line 669 + 47 bytes
nsWebShell::OnEndDocumentLoad(nsWebShell * const 0x03cc57a8, nsIDocumentLoader *
0x03cb57a0, nsIChannel * 0x04252230, unsigned int 0) line 954
nsDocLoaderImpl::FireOnEndDocumentLoad(nsDocLoaderImpl * 0x03cb57a0, nsIChannel
* 0x04252230, unsigned int 0) line 805
nsDocLoaderImpl::DocLoaderIsEmpty(unsigned int 0) line 612
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x03cb57a4, nsIChannel *
0x042bc110, nsISupports * 0x00000000, unsigned int 0, const unsigned short *
0x100a56c8 gCommonEmptyBuffer) line 555
nsLoadGroup::RemoveChannel(nsLoadGroup * const 0x03cb5740, nsIChannel *
0x042bc110, nsISupports * 0x00000000, unsigned int 0, const unsigned short *
0x100a56c8 gCommonEmptyBuffer) line 566 + 39 bytes
nsJARChannel::OnStopRequest(nsJARChannel * const 0x042bc114, nsIChannel *
0x042bec80, nsISupports * 0x00000000, unsigned int 0, const unsigned short *
0x100a56c8 gCommonEmptyBuffer) line 709
nsOnStopRequestEvent::HandleEvent(nsOnStopRequestEvent * const 0x042be720) line 302
nsStreamListenerEvent::HandlePLEvent(PLEvent * 0x042be6d0) line 97 + 12 bytes
PL_HandleEvent(PLEvent * 0x042be6d0) line 580 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00ac97b0) line 513 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x0c110170, unsigned int 49320, unsigned int 0,
long 11311024) line 1049 + 9 bytes
USER32! 77e71820()

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: the image

Comment 2

[(not reading, please use [e-mail] instead)]

mscott was suprised that right click on the image even works!

this worked in 4.x, so marking 4xp.

Comment 3

[David :Bienvenu]

yep, crashes for me too, with a release commercial build.

Comment 4

[David :Bienvenu]

nominating for rtm - didn't notice this was assigned to me...may not finish it
today.

Comment 5

[David :Bienvenu]

doesn't seem to crash any more - just brings up save dialog which spins forever.
Probably not worth fixing for rtm, if that's other people's experience  as well
(i.e., spins but doesn't crash).

Comment 6

[selmer (gone)]

rtm-, assuming the dialog responds to the canel button so there's a way out.  

Comment 7

[selmer (gone)]

OK, putting rtm- in the whiteboard now...

Comment 8

[laurel]

I hit a crash with different messages, different images (gifs) on both Mac and
Win98 today using the oct30 mn6 commercial branch build.  When right clicking
then saving, I saw a default image name such as INBOX>4330 and when it had such
a default name I crashed. After restarting my mac machine, I no longer would see
such default image names and would not crash when saving.  I submitted a couple
talkbacks for mac, here's its stack info, don't know if this would qualify as
the same problem as seth saw:

Incident 20083102

 Call Stack:     (Signature = 0x61aa61a8 ba983939)
     0x61aa61a8
     CODE.10
     XPTC_InvokeByIndex() [xptcinvoke_mac.cpp, line 129]
     nsXPCWrappedNativeClass::CallWrappedMethod() [xpcwrappednativeclass.cpp,
line 907]
     WrappedNative_GetProperty() [xpcwrappednativejsops.cpp, line 272]
     js_Interpret() [jsinterp.c, line 2468]
     js_Invoke() [jsinterp.c, line 837]

Incident 20081711

 Call Stack:     (Signature = 0x61aa61a8 ba983939)
     0x61aa61a8
     CODE.10
     XPTC_InvokeByIndex() [xptcinvoke_mac.cpp, line 129]
     nsXPCWrappedNativeClass::CallWrappedMethod() [xpcwrappednativeclass.cpp,
line 907]
     WrappedNative_GetProperty() [xpcwrappednativejsops.cpp, line 272]
     js_Interpret() [jsinterp.c, line 2468]
     js_Invoke() [jsinterp.c, line 837]
<../images/spacer.gif>

Comment 9

[David :Bienvenu]

changing summary, since it doesn't crash anymore, it just fails.

Comment 10

[pmock]

Crash is reproducible on macos and linux commercial build following original 
steps.  It crashes immediately.  The build I was testing with was 
 Macos commercial seamonkey build 2000-110800-mn6 and Linux commercial build 
2000-110807-mn6

Steps to reproduce:
1) Send yourself a mail message with an attach gif/jpeg attachment or an 
inserted gif/jpeg image. 
2) Retrieve mail message
3) View mail message
   I was viewing mail message in the message pane
4) Right click on the displayed image
   The save dialog appears with a file name such as "Inbox>2343..."
5) Save the image
   Crash.
   No file is saved on hard drive.

On Win32, it does not crash unless I try to change the default file name.  
Following the steps above, if I change the file name from "Inbox>2343..." to 
something else, I receive a application crash.  No file was saved on hard drive.

The win32 build that I was using 2000-110801-mn6.

Updating summary:
 Old summary - right click save as this image in an imap message doesn't work
 New summary - Crash when right click save as this image in an imap message 

Comment 11

[David :Bienvenu]

adding keywords

Comment 12

[[e-mail] (gone - use [e-mail] instead)]

I'll mark 63467 a DUP of this, even the stack trace is the same..

Comment 13

[[e-mail] (gone - use [e-mail] instead)]

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

Comment 14

[scottputterman]

marking nsbeta1+ and moving to mozilla0.8

Comment 15

[David :Bienvenu]

Attachment: proposed fix

Comment 16

[Scott MacGregor]

wow, that ref counting change was the only thing preventing this from working?
Interesting...

sr=mscott

Comment 17

[David :Bienvenu]

well, no, I'm just fixing the crash. Now, it saves the whole message.

Comment 18

[Navin Gupta]

r=naving

Comment 19

[Scott MacGregor]

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

Comment 20

[Phil Peterson]

Saves the whole message? Not just the image?

Comment 21

[David :Bienvenu]

right, I'm just fixing the crash; it saves the whole message.

Comment 22

[David :Bienvenu]

There's a problem if the message is a sent html page.
The attached patch causes a crash in the cookie service code because it doesn't
check for a null uri when asking the channel for a uri. Both
nsCookieService::GetCookieStringFromHTTP and nsCookieService::
SetCookieStringFromHttp need to check for a null aFirstUrl. The reason the uri
is null in the channel is because we null it out when we've finished running the
imap url. Some time after that, the cookie code asks our channel for the uri. I
could fix the cookie code not to crash (I'll attach a patch for that) but I'm
not sure if it's going to cause other problems.

Comment 23

[David :Bienvenu]

I can also fix this by making nsImapMockChannel return an error when it has a
null URI:

NS_IMETHODIMP nsImapMockChannel::GetURI(nsIURI* *aURI)
{
    *aURI = m_url;
    NS_IF_ADDREF(*aURI);
    // is it an error return a null uri? The cookie viewer code in particular
    // does not check for a null returned uri.
    return (*aURI) ? NS_OK : NS_ERROR_NULL_POINTER;
}

this fixes the crash, but causes a couple assertions when displaying html pages
in mail messages.

Comment 24

[Brendan Eich [:brendan]]

NS_ERROR_NULL_POINTER is supposed to indicate an invalid (because null, not
because garbage) pointer argument.  I don't think it should be overloaded to
mean "we would have returned a null result with NS_OK, but that crashes some
callers."

Why can't the channel URI persist after the IMAP operation has finished?  Isn't
that how all other Necko channel impls work?

/be

Comment 25

[David :Bienvenu]

Brendan, some of this is explained in the comments in the patch, but basically,
the channel and the imap url have strong references to each other (the channel
has to have a strong reference to the url to keep the url alive until it is
scheduled to run - why the imap mock channel has to have a strong reference to
the url, I don't know; that would be Scott's dept). Given that they have strong
references to each other, the cycle has to be broken, and the last chance we
have to break that cycle is when the url finishes running. There is an added
complication that last reference has to be released on the UI thread or else
Warren's false-sense-of-threadsafety assertions start firing.

Comment 26

[Brendan Eich [:brendan]]

Sorry, I should have read the patch.  Why can't the channel continue to refer
strongly to the URI after it has run, but once the URI finishes running, we cut
the strong ref from it to the channel?  Maybe only one link in the cycle needs
to be broken.  I'm still figuring out the mock vs. real relation, sorry if I'm
missing something.

/be

Comment 27

[David :Bienvenu]

I tried that orginally, but I needed to break the other link (from mock channel
to uri) in the UI thread or else the url would get deleted on the imap thread,
and trigger a bunch of asserts. Maybe there's some other way I can fix that...

Comment 28

[David :Bienvenu]

the assert only happens in a few situations, like copying messages. I might have
to proxy the clearing of the mock channel member variable in
nsImapProtocol::ReleaseUrlState() over to the UI thread, because this release
often causes a bunch of other objects to get freed on the imap thread that
should be released on the ui thread.

Comment 29

[Brendan Eich [:brendan]]

Does Necko well define the ability to get a valid URI from a channel while the
channel is alive?  Maybe warren knows; nsIChannel.idl doesn't clearly define the
validity (null vs. non-null) of the URI attribute.  The fact that code breaks
when URI becomes null on a live IMAP channel suggests we have a _de facto_
standard, albeit under- or un-documented.

Here's hoping the mock-channel clearing proxy idea works out.

/be

Comment 30

[Darin Fisher]

Since a channel is created from a URI, I think that a channel should always be
able to produce its URI.  Another way to look at it is that a channel is your
access to a resource, without a URI, how do you know what resource you're
accessing?  It seems to me that a channel is meaningless without an associated
URI... right?!?

Comment 31

[David :Bienvenu]

I fixed this by releasing the mock channel from the protocol before releasing
the last reference to the url from the protocol object (the latter is proxied).
Now,  the releases all end up happening on the UI thread. Previously, the
channel was released from the protocol at the end of
nsImapProtocol::ReleaseUrlState(); now it's released at the start of
nsImapProtocol::ReleaseUrlState().

Comment 32

[David :Bienvenu]

marking fixed. I'll open a new bug for it not working. bug 65329

Comment 33

[pmock]

Verified as fixed on win32, linux, and macos using the following builds:
 win32 commercial seamonkey build 2001-011708-mtrunk installed on P500 Win98
 linux commercial seamonkey build 2001-011708-mtrunk installed on P200 RedHat 
6.2
 macos commercial seamonkey build 2001-011708-mtrunk installed on G3/400 OS 9.04

It does not crash anymore.