68454 - Crash when trying to save an image that opens in a javascipt popup - M1BR Trunk [@ comdlg32.dll]

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[matxdr]

Using build 2001020904.

Go to http://cyndilauper.com/pictures_det.asp?shname=offic.  Click on any
picture, a pop up will open with the piicture in it.  Right click in that pop
up, select save as.

Result:  Crash
Expected:  Save image as dialog

Comment 1

[mar_garina]

in my place it doesn't crash, but indeed there's no 'save image as' dialog, just
'save (page) as'...

Comment 2

[HJ]

WORKSFORME on NT4 with build 2001020904

Comment 3

[matxdr]

Sent TB26149258G.  

Comment 4

[Asa Dotzler [:asa]]

talkback says:

   COMDLG32.DLL + 0x1adb0 (0x7fe1adb0)    
     0x2474ffff  

I think that's a windows library. not very informative :(

Comment 5

[sairuh (rarely reading bugmail)]

interesting. i'm using 2001.02.15.08 comm bits on linux --i don't get a crash. i
do get the Save File dialog, go thru the motions to save the file [select dir,
supply filename (fwiw, i saved it as gif), click OK], but when i check the
target dir, nothing is saved!

over to bill [tentatively] --is what i'm seeing a dup?

is anyone else able to reproduce either the crash, or the inability to save the
file? thx!

Comment 6

[Bill Law]

I can reproduce this (more or less) with a recent debug build.

Those popup pages are nasty.  They have an onblur handler that closes the
window.  Apparently, dismissing a context menu triggers that handler, which
closes the window.  When the context menu itself triggers actions that need the
window (e.g., Save As...), things go haywire.  At least that's my guess.
Probably an event/window kind of thing (for saair or danm), but I'll investigate
further.

This is pretty severe (content can encourage the user to crash the browser);
nominating for nsbeta1.

Comment 7

[Bill Law]

Attachment: Call stack showning where things go bad

Comment 8

[Bill Law]

I attached the call stack I get to when I recreate this in the debugger.  
Something is going very wrong as a result of the file picker's parent being 
closed out from under it.

Handing off to Dan Matejka (who used to be a big Cyndi Lauper fan, I hear).

Comment 9

[Peter Trudelle]

->0.9.2

Comment 10

[Peter Trudelle]

->0.9.3 unless this is more widespread (Sorry, Cyndi!)

Comment 11

[John Morrison]

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

Comment 12

[sairuh (rarely reading bugmail)]

bug 89131 has a good test case.

Comment 13

[greer]

Adding talkback keyword topcrash, sig in the summary and cc'ing some talkback 
folks. Clutter everywhere.

Comment 14

[John Morrison]

Removing topcrash+signature. I think you've got the wrong bug.

Comment 15

[Blake Ross]

Works for me on Windows. Anyone else?

Comment 16

[John Morrison]

I crash, current branch or trunk on win2k, for the steps to reproduce at the 
beginning of this bug.

Comment 17

[Joshua Sako]

I get this problem, too, only I am able to repeat on any arbitary image. I tried
it, in fact, on the main Mozilla banner at mozilla.org and the crash still happened.

Interestingly enough, I saved the image using IE 5.5, and Mozilla refused to
read it. I then tried to save the image using Netscape 4.7 and everything worked
fine. I'm not sure if that new information helps any.

Comment 18

[Jay Patel [:jay]]

I was digging around in Talkback data and came across a comment mentioning this
bug.  Here is the info:

===> 4   0x00000000 8560629a ===> 0x00000000 
 
 
     Crash date range: 2001-09-15 to 2001-09-18
     Min/Max Seconds since last crash: 157 - 150102
     Min/Max Runtime: 157 - 150102
 
     Count       Platform List 
   1 Linux 2.4.5
   2 Linux 2.4.9
   1 Windows NT  5.1 build 2600
 
     Count      Build Id List 
   4 2001091311
 
 Comment Keywords Count 
 
 Offset and and their stack traces 
===> 0x00000000 8560629a <===

	 0x00000000   
 
     (35549279)	URL: www.subaru.com
(35549279)
Comments: Clicking around caused a crash.  Site uses a lot of Flash and JavaScript.
     (35521442)	Comments: Just looking at the preferences. Didn't change anything!
     (35466168)	URL: http://cyndilauper.com/pictures_det.asp?shname=offic
(35466168)
Comments: Crash in response to bug #68454Mozilla 0.9.4 (build 2001091303) on
WindowsXP Pro
     (35417357)	Comments: in the preferences - in the "tree":expanded the whole tree
compressed the first folder againbang

Incident 35466168 will probably be of most interest.  The last comment about
preferences I think is actually a crash represented by bug 86723.  I can't see
this bug and that bug being related, but Talkback reported both of them with the
same stack signature and offset...weird.

Comment 19

[sairuh (rarely reading bugmail)]

spam: over to File Handling. i have not changed the assigned developer [or the
other fields for that matter], so if anyone realizes that a bug should have a
more appropriate owner, go ahead and change it. :)

Comment 20

[John Morrison]

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

Comment 21

[R.B.]

No crash with Linux 2002032306. (No option in the popup to save the pic either.)
I still crash when I use old builds. Has this been fixed ?

Comment 22

[Matthias Versen [:Matti]]

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

Comment 23

[Adam Hauner]

Crash with 2002040903/Win2K -> TB5004684W and TB5004607X.

Comment 24

[Kai Engert (:KaiE:)]

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

Comment 25

[Kai Engert (:KaiE:)]

Nominating.

See bug 159793 for an always reproducible test case.

This bug happens only on Windows. It crashes in the "file save as dialog"
provided by the windows operating system. We dereference a null pointer. It
seems, the dialog inside comdlg.dll received a pointer from Mozilla - but
because the Mozilla window closes, the common dialog crashes.

Comment 26

[David Bradley]

What I saw (bug 132532) was that the parent window handle specified was
destroyed or otherwise invalid.

Comment 27

[Jay Patel [:jay]]

Adding M1BR Trunk [@ comdlg32.dll] so summary and topcrash+, testcase keywords.
 This is a topcrasher on the Gecko 1.0 branch and there are also incidents on
the MozillaTrunk.  A reproducible case is noted in comment #25.

Comment 28

[John Morrison]

(er, noting that, on the 1.0 branch, 4 talkback incidents is enough to grab
the seventh spot on the talkback top of the pops, and, er, kaie submitted the
requisite 4 incidents.).

Comment 29

[Jaime Rodriguez, Jr.]

Marking as adt3 based on Comment #28 From John Morrison. 4 incidents, submitted
byt the same person, doesn't seem like a topcrasher IMHO. Are there any other
recent incidents other than kaie's in talkback?

Comment 30

[John Morrison]

Hmm, didn't want to discourage a fix. It's been a long-standing known problem
that bringing up a dialog from the context window for a window that sets 
'onblur="self.close()"' will crash.

Comment 31

[John Morrison]

Actually, when I went to the brief report on climate it only showed the four
entries by kaie. But when I ran this query myself, I got 143 crashes in 
comdlg32.dll since 07/01 on all builds (Gecko1.0, Trunk, 6.x). Not all the 
crashes are this one (some refer to printing) but many refer to saving an 
image (often also mentioning popup). 

I still think doing 'onblur="self.close()"' is a little sick, but it is a not
totally uncommon trap for the unwary.

Comment 32

[Chi-Thanh Christopher Nguyen]

the screenshot on http://www.activestate.com/Products/Komodo/ will also crash
when trying to 'Save Image As...'

TB11737012H

Comment 33

[Juan José Mata]

Moving milestone out to future since we've obviously missed the 1.1beta.

Dan can let us know when he expects this will get fixed when he gets around to
looking at it again.

Comment 34

[Andreas von Arb]

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

Comment 35

[Andrew Schultz]

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

Comment 36

[Chris Lyon]

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

Comment 37

[Chris Lyon]

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

Comment 38

[Jan Carpenter]

No talkback incidents from recent builds, and not showing up on any talkback
reports.  Marking fixed.

Comment 39

[greatmp3]

Still there in Mozilla 1.2.1

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2.1) Gecko/20021130

Reproduce as desribed.

Sent TB16453791H

Comment 40

[Kai Engert (:KaiE:)]

Confirming and reopening bug. Still reproducible with latest trunk on W2K using
testcase from bug 159793.

Comment 41

[Dan M]

Just a bit of status while I beat on this bug. Kai's testcase (comment 40) still
demonstrates the bug. But the original testcase currently appears fixed because
it's concealed by some very messed up event handling in the trunk. Cyndi's
popups don't crash because they never get blur events at all. See bug 191154.
(Even if you have a special window that gets blur events, it'll miss the first
one. See bug 166748. And once it starts getting events it'll get too many. Ssee
bug 164686.) So anyway, at least I understand why this crash happens less often
than it once did.

Comment 42

[Dan M]

Attachment: partial, windows-only fix candidate

This is a windows-only version of the fix to solicit reviews on the direction
I'm taking with this. The bug is, windows with blur handlers which close the
window do indeed close as soon as context-menu->SaveImageAs summons the
filepicker dialog. Ugliness ensues. I believe it's best to suppress blur events
from the filepicker. IE behaves this way.

bryner and I figured there were two places to do this, in nsEventStateManager
or in nsWindow. nsESM is probably more correct but that module is pretty much
hidden from nsFilePicker (widget) code. The nsWindow route requires individual
fixes for each platform but involves less lifting of skirts.

This patch fixes the crash on Windows. Barring allergic reactions from widget
people, I intend to check in this patch with some cleanup, along with *nix and
Mac versions. Still I'm more interested in approval of concept than a real
review at this point.

Comment 43

[Dan M]

Attachment: suppress blur events from the filepicker

Replacing the original patch. Once again this bug happens because we send blur
events to the window owning a filepicker window. If that window contains a blur
handler that closes itself, the window containing the data referenced by the
filepicker is destroyed out from underneath it.

IE doesn't send blur events in this case, and that seems the way to be. Sadly,
the Windows and gtk builds have completely different behaviour. The gtk build
uses a XUL filepicker; Windows a native one. I've chosen to suppress blur
events on XUL platforms within the DOM window, and on native platforms within
their respective widget libraries.

The two systems are completely independent and parallel. They could be hooked
together. However that would involve changing nsIWidget. There's resistance to
that (though it's not out of the question). It would also require getting at
Views from within the widget code. Again this is not impossible but the system
is designed to be opaque in that direction and I thought it best not to pierce
the veil. Oh, and Mac OSX build doesn't have the bug at all. Yay Mac OSX!

So here it is. It ain't pretty. It works.

Comment 44

[Dan M]

Attachment: suppress blur events from the filepicker

Much like the last patch (see comment 43) except the blurSuppression method has
been moved from nsIDOMWindowInternal to nsIBaseWindow.

Comment 45

[Johnny Stenback (:jst)]

Comment on attachment 114056 [details] [diff] [review]
suppress blur events from the filepicker

sr=jst

Comment 46

[Brian Ryner (not reading)]

Comment on attachment 114056 [details] [diff] [review]
suppress blur events from the filepicker

ew. nsIBaseWindow strikes again.  I really don't understand why we have this
interface or why we have so many implementations of it.  Anyway, r=me on this
patch.

Comment 47

[Kevin McCluskey (gone)]

Dan, Could you factor this code into a separate member function and call it from
both GetParent and BlurEventsSuppressed? 

 // this is GetParent(), but without the mIsTopWidgetWindow check
+  if (mIsDestroying || mOnDestroyCalled)
+    return PR_FALSE;
+
+  nsWindow *widget = 0;
+  if (mWnd) {
+    HWND parent = ::GetParent(mWnd);
+    if (parent) {
+      widget = GetNSWindowPtr(parent);
+      if (widget)
+        if (widget->mIsDestroying)
+          widget = nsnull;
+        else
+          NS_ADDREF(widget);
+    }
+  }

With that change r=[e-mail] for the widget module changes.

Comment 48

[Dan M]

surely

Comment 49

[Oliver Klee]

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

Comment 50

[Dan M]

Patch checked in 24 Feb. PS I didn't refactor GetParent (comment 47) but instead
went with the simple, tight (while ::GetParent(HWND)) loop already in use
throughout the file (seven other places). These (and two other instances that
aren't in a loop) could all probably stand to be rolled up into a GetParent(bool
stop at top window, bool immediate or keep going) method.

But anyway, bug's fixed.

Comment 51

[Chris Petersen]

Verified in the 2003-03-13-04 Win32 and 2003-03-13-03 Mach-0 OS X Trunk builds.

Comment 52

[Dan M]

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