161934 - make Save as ... for mail attachments and messages, exporting addressbook call Init() correctly and handle Show() failing

Status: RESOLVED WORKSFORME

(Core Graveyard :: File Handling, enhancement)

Description

[Boris 'pi' Piwinger]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.1b) Gecko/2002080820

When I try to save an attachment of a mail message, Mozilla crashes. Open does
work without a problem save all does nothing.

pi

Comment 1

[Pratik]

I see this too. Build 2002080904 on linux

Comment 2

[Boris Zbarsky [:bzbarsky]]

How about a talkback id, eh?  ;)

Comment 3

[Andrew Schultz]

Attachment: stacktrace

(aFile is NULL in NS_NewFileSpecFromIFile)
got this in the terminal:
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "window is not defined" {file:
"file:///home/andrew/mozilla/dist/bin/components/nsFilePicker.js" line: 181}]'
when calling method: [nsIFilePicker::show]"  nsresult: "0x80570021
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame ::
chrome://messenger/content/msgHdrViewOverlay.js :: saveAttachment :: line 751" 
data: yes]
************************************************************

Comment 4

[Boris 'pi' Piwinger]

According to Christopher Blizzard we won't be able to provide talkback data from
redhat. Sorry.

pi

Comment 5

[Andrew Schultz]

do you mean you're using an RPM build?  If so, you can use gdb to do the
stacktrace yourself.  A crash bug is not very useful until it has a stacktrace
or talkback ID.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

can you who see this crash enable dump() output (in preferences/debug) and tell
me if you see a "unable to open file picker" output on stdout (or stderr, not sure)?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

->timeless

Comment 8

[Andrew Schultz]

there's no "unable to open file picker"

backing out bug 161722 fixes the crash.

Comment 9

[Andrew Schultz]

the patch from bug 161277 doesn't seem to be the problem (the problem just was
covered up before that patch).  The problem seems to be this code:


   } else if (window && typeof(window) == "object") {
      parent = window;

window is undeclared (as stated in the xpconnect error).  removing those two
lines also fixes the crash.  What is that code supposed to be doing?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Checkin comments say it's fixing bug 34328.  But that just changed the check
from "if (window)" to what the code is now.

Pav?  Do you recall why you did it this way?  Under what conditions is "window"
actually defined and non-null in this code?

Comment 11

[timeless]

ok, so there are three problems.
1. that the caller doesn't handle js throwing an exception (blame the caller):
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/src/nsMessenger.cpp&rev=1.248&mark=742-743,754,698-699#737
2. the callee would return null (0) returnOK in some strange failure mode. This
could also have led to the crash (it didn't in this case, thanks andrew for
tracking it down).
3. the callee has some code which never worked (which i unfortunately exposed).

Comment 12

[timeless]

Attachment: fix and cleanup callers

ok messenger messed up a few ways.
1. it should have called init with its window
2. it should have checked to see if show failed

i swept through the tree and fixed the callers to at least check for show
failing. note that many callers actually did check for/expect show to throw an
exception.

* addressbook didn't seem to have easy access to its own window (it has access
to the window's docshell, but i didn't want to fiddle).
* i couldn't figure out how to write code in venkman for this so i filed bug
162144.
* crypto tries to avoid having access to the gui thread and fun stuff like that
so i couldn't find its window either.

this patch drops the offending line from nsFilePicker and makes nsFilePicker
slightly more likely to throw exceptions in certain other failure cases.  In
those cases the current code would probably have crashed anyway so with the
fixed callers the story should be straight and everyone should be happy.

I'm going to need to get reviews from a bunch of people

I'm not certain about the transformiix changes, they're testing code but i'd
like pike to review them (they aren't essential so if i don't get a review i
just won't check them in).

Comment 13

[timeless]

oh right, print to file didn't appear to have access to its window either ...

Comment 14

[Andrew Schultz]

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

Comment 15

[Andrew Schultz]

from bug 162155: this bug also prevents saving messages as files!

the current patch is going to take a long time before it can be checked in due
to the number of review (and probably revisions) it will need.  Can we get a
small patch that restores functionality so that people can continue to use
MailNews until the big patch lands?

Comment 16

[Axel Hecht]

I don't like the changes to extensions/transformiix, an unhandled exception has
the same functionality, but is at least visible in the js console.
So please don't land these.

Comment 17

[Andrew Schultz]

exporting bookmarks also crashes

Comment 18

[Andrew Schultz]

Attachment: minimalist patch to get things working

this just removes the reference to |window| in nsFilePicker.js
it fixes the crash for saving attachments and exporting bookmarks, and also
fixes saving messages.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 94865 [details] [diff] [review]
minimalist patch to get things working

If we're going for a "quick fix", then back out the patch for bug 161722.  

Otherwise, please tell me exactly what filepicker users this patch breaks and
why it's OK to break them.

Comment 20

[Andrew Schultz]

> please tell me exactly what filepicker users this patch breaks

I added a dump() statement to the else branch and only callers that are
experiencing this bug hit the dump().

I checked:
Browser: File->Save As, Open File, Print (Choose File), Export/Import Bookmarks
Venkman: File->Open File and Save Profile Data
Import Addressbook
Composer: File->Save As, Attach File (and click in attachment area), Insert 
    Image (choose file), Print
Account Settings: choose a .signature file
Preferences: choose a helper app file (from New and Edit help app)
Certificate Manager: import

MailNews: File->Save As, Attachments->Save All, Save As (attachment) are 
    broken without the patch, but work with the patch.

looking at the code, it should only hit the else branch when init is called with
a null parent.  currently, only saving attachments and messages appear to do that.

so, in short, the patch does not affect callers that are not busted by this bug,
at least none that I can find


> If we're going for a "quick fix", then back out the patch for bug 161722.

sounds ok too.


I also can't reproduce what I was seeing with exporting bookmarks... dunno what
happened there before.

Comment 21

[R.K.Aa.]

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

Comment 22

[Andrew Schultz]

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

Comment 23

[Andrew Schultz]

exporting addressbook is the other operation that crashes (and is also fixed by
attachment 94865 [details] [diff] [review])

|window| in nsFilePicker is undeclared... how can any caller depend on |window|
functionality?  Am I missing something?

looking back at old versions in CVS I could not find any version of nsFilePicker
that had declared |window|.  Very old versions did not mention it at all.

Comment 24

[Kai Engert [:KaiE:]]

Timeless sent a mail message asking us to review, so I analyzed what is
happening here. I'm able to explain the problem.


First, I completely disagree with the patch "fix and cleanup callers". This
patch is changing a lot of code, but is not trying to find out what has
initially cause the regression.

I think, within this bug, we should only fix the regression, to minimize
confusion. Any suggested cleanup work should be done in separate patches.

Quick excursion re the many changes in the patch named "fix and cleanup callers:
I think we should not add blind exception checks everywhere. I agree with Axel
that we loose a lot of failure detection.)


Back to the original problem.
I am able to reproduce the "can not save message".
Backing out the patch from bug 161722 fixes the problem for me!

In bug 161722, you said you want to remove the outer try block, because it
seemed unreachable to you. However, you don't explain why you are 100% sure that
it won't be ever reached. If you are not sure, please let's add it back.

Second, and that's the real cause of the problem: You changed more than just the
removing the outer try block.

The additional change is:
-    } else if (typeof(window) == "object" && window != null) {
+    } else if (window && typeof(window) == "object") {

With this change, it is broken.
Without this change (original state) it works.

It seems, the old order of expressions is avoiding an exception.

It seems, the check "typeof(window)" is failing, and because of that, the second
check will not get executed. That's good. Because if the "window" or "window !=
null" check get's executed, an exeption "window is not defined" is thrown!

I'm not a JS expert, but I believe this is because of the special role of the
"window" identifier in JavaScript.


So I vote to simply revert the patch from bug 161722. I vote to re-think all the
cleanup work suggested and at least move it to a separate bug, and please do not
add the blind try{} catch() {/*nothing*/} blocks.


Instead of simply backing out the patch from bug 161722, one could also suggest,
why not use the second patch in this bug, called "minimalist patch to get things
working"?

That might be ok, but I don't know. Is there a change, that in some scenarios,
that original check might actually make sense? I don't know JavaScript good
enough to make a 100% statement. Unless somebody else is able to guarantee that 
      } else if (typeof(window) == "object" && window != null) {
        parent = window;
never makes sense, I suggest we leave in the code we don't understand :)

Comment 25

[Boris Zbarsky [:bzbarsky]]

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

Comment 26

[Kai Engert [:KaiE:]]

Timeless, thanks for your new patch/checkin with bug 161722. I can no longer
reproduce the problem, using mail save as for messages and attachments again
works for me.

Comment 27

[beanladen]

Will the patch go into 1.1 ? Otherwise that branch would be quite useless
for users.

Comment 28

[Boris Zbarsky [:bzbarsky]]

That would be true if bug 161722 had landed on the 1.1 branch.  It did not, however.

Comment 29

[Kai Engert [:KaiE:]]

In order to have an easier time doing the review, I created a
whitespace-ignore-patch.

Below security manager you make two different classes of changes:
1) multiple js code changes, added try {/*existing code*/} catch () {/* do
nothing*/}
2) change to nsCrypto.cpp, which did not check for an error return

I fully agree with your change 2) because without your change, we have the risk
of using the return code although it is invalid.

But I don't think 1) is necessary for the following reasons:
- it seems to me, fp.show() is now unlikely to fail (not that 161722 is corrected)
- even if fp.show() fails, there is no risk that erraneous data will be
accessed. An exception will be thrown and JS execution will stop.
- you wrap much more code in the try/catch block then necessary. You not only
protect the fp.show() call. You also protect existing other application logic
- by wrapping other existing code in the try/catch block, we loose the ability
to do run time analysis of other problems in the JS console.

I suggest we only use try/catch blocks in JS code, when we expect that
exceptions will be thrown during execution as a normal condition.

What do others think?

Comment 30

[Magnus Melin [:mkmelin]]

According to comment #26 the fix was checked in on trunk back in 2002...