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

VERIFIED FIXED in mozilla1.4alpha

Status

Core Graveyard
File Handling
--
critical
VERIFIED FIXED
17 years ago
2 years ago

People

(Reporter: matxdr, Assigned: Dan M)

Tracking

({crash, testcase, topcrash+})

Trunk
mozilla1.4alpha
crash, testcase, topcrash+

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt3], crash signature, URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

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

Comment 2

17 years ago
WORKSFORME on NT4 with build 2001020904
(Reporter)

Comment 3

17 years ago
Sent TB26149258G.  

Comment 4

17 years ago
talkback says:

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

I think that's a windows library. not very informative :(
Assignee: asa → vishy
Component: Browser-General → XP Apps
QA Contact: doronr → sairuh
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!
Assignee: vishy → law
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

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

Updated

17 years ago
Keywords: nsbeta1- → nsbeta1

Comment 7

17 years ago
Created attachment 27033 [details]
Call stack showning where things go bad

Comment 8

17 years ago
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).
Assignee: law → danm

Comment 9

17 years ago
->0.9.2
Target Milestone: --- → mozilla0.9.2

Comment 10

17 years ago
->0.9.3 unless this is more widespread (Sorry, Cyndi!)
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Updated

17 years ago
Keywords: crash

Updated

17 years ago
Keywords: nsenterprise

Comment 11

17 years ago
*** Bug 89131 has been marked as a duplicate of this bug. ***
bug 89131 has a good test case.
OS: Windows ME → All
Hardware: PC → All

Comment 13

17 years ago
Adding talkback keyword topcrash, sig in the summary and cc'ing some talkback 
folks. Clutter everywhere.
Keywords: topcrash
Summary: Crash when trying to save an image that opens in a javascipt popup → Crash when trying to save an image that opens in a javascipt popup [@ nsVoidArray::RemoveElement]

Comment 14

17 years ago
Removing topcrash+signature. I think you've got the wrong bug.
Keywords: topcrash
Summary: Crash when trying to save an image that opens in a javascipt popup [@ nsVoidArray::RemoveElement] → Crash when trying to save an image that opens in a javascipt popup

Comment 15

17 years ago
Works for me on Windows. Anyone else?

Comment 16

17 years ago
I crash, current branch or trunk on win2k, for the steps to reproduce at the 
beginning of this bug.
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Updated

17 years ago
Keywords: nsenterprise → nsenterprise-

Comment 17

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

Updated

17 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Comment 18

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

Updated

17 years ago
Target Milestone: mozilla0.9.5 → mozilla0.9.6
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. :)
Component: XP Apps → File Handling

Updated

17 years ago
Blocks: 104166
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.6 → mozilla0.9.7
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.8

Comment 20

17 years ago
*** Bug 115542 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.8 → ---
(Assignee)

Updated

16 years ago
Target Milestone: --- → mozilla1.0.1

Comment 21

16 years ago
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 ?
*** Bug 134406 has been marked as a duplicate of this bug. ***

Comment 23

16 years ago
Crash with 2002040903/Win2K -> TB5004684W and TB5004607X.
(Assignee)

Updated

16 years ago
Target Milestone: mozilla1.0.1 → mozilla1.1beta

Comment 24

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

Comment 25

16 years ago
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.
Keywords: nsbeta1

Comment 26

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

Comment 27

16 years ago
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.
Keywords: testcase, topcrash+
Summary: Crash when trying to save an image that opens in a javascipt popup → Crash when trying to save an image that opens in a javascipt popup - M1BR Trunk [@ comdlg32.dll]

Comment 28

16 years ago
(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

16 years ago
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?
Whiteboard: [adt3] [ETA Needed]

Comment 30

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

16 years ago
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.
the screenshot on http://www.activestate.com/Products/Komodo/ will also crash
when trying to 'Save Image As...'

TB11737012H
QA Contact: sairuh → petersen

Comment 33

16 years ago
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.
Target Milestone: mozilla1.1beta → Future

Comment 34

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

Comment 35

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

Comment 36

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

Comment 37

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

Comment 38

16 years ago
No talkback incidents from recent builds, and not showing up on any talkback
reports.  Marking fixed.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 39

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

16 years ago
Confirming and reopening bug. Still reproducible with latest trunk on W2K using
testcase from bug 159793.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 41

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

Updated

15 years ago
Whiteboard: [adt3] [ETA Needed] → [adt3] [ETA: soon. ugly but working patch for Windows in hand]
(Assignee)

Comment 42

15 years ago
Created attachment 113553 [details] [diff] [review]
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.
(Assignee)

Comment 43

15 years ago
Created attachment 113868 [details] [diff] [review]
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.
Attachment #113553 - Attachment is obsolete: true
(Assignee)

Comment 44

15 years ago
Created attachment 114056 [details] [diff] [review]
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.
Attachment #113868 - Attachment is obsolete: true
Comment on attachment 114056 [details] [diff] [review]
suppress blur events from the filepicker

sr=jst
Attachment #114056 - Flags: superreview+
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.
Attachment #114056 - Flags: review+
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=kmcclusk@netscape.com for the widget module changes.
(Assignee)

Comment 48

15 years ago
surely
Whiteboard: [adt3] [ETA: soon. ugly but working patch for Windows in hand] → [adt3]
Target Milestone: Future → mozilla1.4alpha

Comment 49

15 years ago
*** Bug 162821 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 50

15 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago15 years ago
Resolution: --- → FIXED

Comment 51

15 years ago
Verified in the 2003-03-13-04 Win32 and 2003-03-13-03 Mach-0 OS X Trunk builds.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 52

15 years ago
*** Bug 196806 has been marked as a duplicate of this bug. ***
Crash Signature: [@ comdlg32.dll]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.