267406 - Trunk crash using Ctrl-S or File/Save Page As to save a web page [@ nsFilePicker::ShowW]

Status: VERIFIED FIXED

(SeaMonkey :: General, defect)

Description

[Stephen Moehle]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041102
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041102

I am reopening this bug since I am experiencing exactly the same problem. 
Mozilla crashes immediately upon typing Ctrl-S or picking the File/Save Page As
menu.

Tested with Seamonkey trunk 2004-11-02-13 build on Windows 2000 with both new
and existing profiles.

Talkback IDs: TB1687088K and TB1687069K.

Reproducible: Always
Steps to Reproduce:

Comment 1

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

nsFilePicker::ShowW 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsFilePicker.cpp,
line 127]
nsFilePicker::Show 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsFilePicker.cpp,
line 370]
XPTC_InvokeByIndex 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 102]
XPCWrappedNative::CallMethod 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,
line 2036]
[...]

sounds like bug 252058; esp. bug 252058 comment 5...

Comment 2

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

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

Comment 3

[Tracy Walker [:tracy]]

this is a smoketest blocker

Comment 4

[Boris Zbarsky [:bzbarsky]]

Phil, why are we setting a null displayDirectory, exactly?

Comment 5

[Andrew Schultz]

only drivers can set blocking flags

Comment 6

[Phil]

(In reply to comment #4)
> Phil, why are we setting a null displayDirectory, exactly?

IIRC, in my patch I don't specifically; it will be null if there is an error
retrieving "browser.download.dir". All you have to do it entirely remove this
setting and you'll see a problem. In the case of a problem, displayDirectory is
set to null. I guess I shouldn't do that (I was just being tidy ;-) ) so I'll
see if that's what causes the problem.

Comment 7

[Phil]

(In reply to comment #6)
> (In reply to comment #4)
> > Phil, why are we setting a null displayDirectory, exactly?
> 
> IIRC, in my patch I don't specifically; it will be null if there is an error
> retrieving "browser.download.dir". All you have to do it entirely remove this
> setting and you'll see a problem. In the case of a problem, displayDirectory is
> set to null. I guess I shouldn't do that (I was just being tidy ;-) ) so I'll
> see if that's what causes the problem.

Badly worded - I DO specifically if there is an error... leave this with me for
12 hours

Comment 8

[Phil]

(In reply to comment #4)
> Phil, why are we setting a null displayDirectory, exactly?

Well, I took that out (setting displayDirectory to null if browser.download.dir
doesn't exist), but the crash still occurs, and I have no idea why. Seems
setting it to null is inconsequential. Will have to dig out some old code.

Comment 9

[Phil]

Attachment: Fix for crash when browser.download.dir setting is missing

When browser.download.dir is missing, browser crashes (regression after fix for
bug 160454 checked in). Problem occurred after calling

    prefs.getComplexValue("dir", Components.interfaces.nsILocalFile)

When this was called in a separate method and returned to the poseFilePicker
method the problem occurred, but there is no need now for the separate method.
I've pulled that out, the code is simpler and it works (bonus ;) ).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 164967 [details] [diff] [review]
Fix for crash when browser.download.dir setting is missing

r=bzbarsky.  Neil, would you sr?

Comment 11

[[e-mail]]

Comment on attachment 164967 [details] [diff] [review]
Fix for crash when browser.download.dir setting is missing

Nice simplification :-)

Comment 12

[[e-mail]]

By the way, I assume you know about the assertions caused by
getDefaultExtension()'s method of getting an extension.

Comment 13

[Boris Zbarsky [:bzbarsky]]

There are assertions?  Could you paste in the assert text?

I just checked this patch in.  Marking fixed.  Thank you for fixing this, Phil!

Comment 14

[Phil]

(In reply to comment #13)
> There are assertions?  Could you paste in the assert text?
> 
> I just checked this patch in.  Marking fixed.  Thank you for fixing this, Phil!

No problem... it was my code, afterall ;-)

Comment 15

[[e-mail]]

Actually I got an assertion and a warning, although the assertion turned out to
be in exthandler and not related to this code. For your information:

WARNING: malformed url: no scheme, file nsStandardURL.cpp, line 741

I don't even know if it's a new warning, so maybe I should shut up now.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yeah, that one's known.  We have a bug on it somewhere....

Comment 17

[Jay Patel [:jay]]

Talkback data suggests that this crash is gone after 11/8 builds, so the fix
looks good.  Marking verified.