307678 - Dialog windows randomly freeze when closing using either standard buttons or titlebar widget

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Cedric Vivier [:cedricv]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

NB: iradio.exe is a customized version of bleeding-edge Firefox 1.5 Beta1 RC (in
part because it's faster for me since I'm actively working on this version), it
has a bug may *sometimes* occurs when closing dialogs (by clicking OK or X).

Reproducible: Sometimes

Steps to Reproduce:
1. Open a window through window.openDialog()
2. Close the opened window.
3. Repeat from 1 the window doesn't freeze (you may have to open/close up to 10
times)

Actual Results:  
The opened window freeze (not clickable, no repaint) and as such doesn't close.

Expected Results:  
Close the opened window.

Interestingly, this bug do not occurs when using window.open for the same window.

Comment 1

[Cedric Vivier [:cedricv]]

Attachment: testcase (1/2)

use this xul to open the window from firefox 1.5 beta 1

Comment 2

[Cedric Vivier [:cedricv]]

Attachment: testcase (2/2)

This is the window that will be opened by testdialog.xul

Comment 3

[Cedric Vivier [:cedricv]]

please ignore the first paragraph, copy/paste error <:-)

Comment 4

[Cedric Vivier [:cedricv]]

Attachment: testcase to unzip in chrome/

Put this in your Firefox chrome/ directory.
Testcase at location :	  chrome://testdialog/content/

Comment 5

[Cedric Vivier [:cedricv]]

Attachment: screenshot

screenshot of window freeze (no repaint)

Comment 6

[Cedric Vivier [:cedricv]]

Comment on attachment 195393 [details]
testcase to unzip in chrome/

unzip it

Comment 7

[Giorgio Maone [:ma1]]

Not sure if/why window.open is not affected (not verified), but I've seen this
one happening with my eyes and I've got dozens of reports from my users.

Additional observations:
1) Firefox is not blocked (you can go on browsing, even if you can't dispose the
dialog)
2) The "Cancel" button can produce the same freeze too.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 FireFox
Firefox/1.4

Comment 8

[Shayne Jewers]

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

Comment 9

[Robert Accettura [:raccettura]]

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

Comment 10

[Robert Accettura [:raccettura]]

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

Comment 11

[Robert Accettura [:raccettura]]

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

Comment 12

[Frank Wein [:mcsmurf]]

probably a dep

Comment 13

[Giorgio Maone [:ma1]]

(In reply to comment #12)
> probably a dep

Bug 306925 probably depends on this, if not a dupe (this one happens randomly on
*every* dialog, including preferences): inverting dependency.

I've just seen this happening also with dialogs opened with "window.open()" (not
only "openDialog()"): updating Summary.

Moving to "Widget: Win32" rather than "Widget", since all the dupes are reported
on Windows.

Comment 14

[Frank Wein [:mcsmurf]]

But the other bug was already in Widget: Win32... ;), so other way round and
that one contains more info.

Comment 15

[Mike Connor [:mconnor]]

I can't reproduce in current trunk, and without a regression window/possible fix
we're not going to block on this.

Comment 16

[Robert Accettura [:raccettura]]

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

Comment 17

[Cedric Vivier [:cedricv]]

(In reply to comment #15)
> I can't reproduce in current trunk, and without a regression window/possible fix
> we're not going to block on this.

I just tested the testcase again against :
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050927
Firefox/1.6a1
and the bug is still there.

It is random and can arise sometimes at the first dialog opened, sometimes at
the 20th or more.

Comment 18

[Cedric Vivier [:cedricv]]

Interestingly, I've found that the bug doesn't show up when you close the dialog
through ALT-F4 (or any key captured by onkeyup).
So this could well be a mouse events related problem I believe.
Especially a MouseMove thingie since when I move the testcase main window at the
center of the screen such as I can open/close by clicking buttons without moving
mouse, the bug doesn't show up either.

Comment 19

[Cedric Vivier [:cedricv]]

Amazingly the bug isn't reproducible when you use "Windows Classic" theme!
Mike, maybe you are using this theme?

Comment 20

[Asa Dotzler [:asa]]

if we can get some reliable testcase and more people are seeing this, please
renominate.

Comment 21

[Cedric Vivier [:cedricv]]

Attachment: destroywindow patch

DestroyWindow() - for some reason (threads-related probably) - didn't send
WM_DESTROY messages everytime when used from nsWindow::Destroy().

Moving it into nsWindow destructor solves the bug and the window gets destroyed
correctly :)

Comment 22

[Cedric Vivier [:cedricv]]

Attachment: destroywindow patch (v2)

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #22)
> Created an attachment (id=197887) [edit]
> destroywindow patch (v2)

Cedric, patches have to be created by diff'ing the modified source against the
cvs version (option -pu6 is recommended). Only these patches will be reviewed.
And please don't ask for approval until there is no r= and sr=.

Comment 24

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

this patch requires super-review too, and the review should be done by a win32
widget peer, or ideally, its owner (cf http://www.mozilla.org/owners.html)

Comment 25

[Charles Melhorn]

I see this problem, or something similar, consistently with my extension and the
latest 1.8 nightly (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b5)
Gecko/20050929 Firefox/1.4).

Steps to reproduce:

1. Install my extension: http://downloads.mozdev.org/mdhashtool/mdhashtool-0.4dp.xpi

2. Open the main dialog (Tools > MD Hash Tool)

3. Use the 'browse...' button to select a file

4. Use the 'browse...' button a second time to select another file

5. Attempt to close the main dialog window using the 'close' button

Dialog window will freeze as described above. No problems at all using this
dialog with Firefox 1.0.x.

Comment 26

[Cedric Vivier [:cedricv]]

(In reply to comment #25)
> I see this problem, or something similar, consistently with my extension
> ...
> 3. Use the 'browse...' button to select a file
> ...
> 4. Use the 'browse...' button a second time to select another file

Great find!
The bug is also randomly reproducible without "browse" by closing repeatedly but
opening a browse dialog seems to higher the bug ratio at 1/2 instead of
1/10-20... and it works with "Windows Classic" theme (XP theme, not FF) too.

With patch applied, I can't reproduce the bug either :)

Comment 27

[Adrian Kalla [:adriank]]

i can confirm this bug. This happens randomly for me, for example by clicking
the "OK" button in the preferences or in the about window of the extension
"noscript" by clicking the "ok" button. Ca. one of twenty cliks results in an
frozen window - the rest of my SeaMonkey 1.0a release functions without problems
- the frozen window get out only with closing the compleate Suite.

Comment 28

[Cedric Vivier [:cedricv]]

Attachment: diff -pu8N against cvs

Comment 29

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

hrm, so the window stays on screen until all references to it are released?

Comment 30

[Cedric Vivier [:cedricv]]

(In reply to comment #29)
> hrm, so the window stays on screen until all references to it are released?

I just checked this by adding nsIWindowMediator enumerator into the testcase,
the window is not referenced within Mozilla at all (anyway when you trace it the
destructor is called as normal everytime)

AFAIK, it gets destroyed at Mozilla closing because Windows internally closes
all windows of a given process when it terminates.

Comment 31

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

but any code could be holding a reference to that nsIWidget, could it not?

Comment 32

[Asa Dotzler [:asa]]

not a blocker for the beta but we'd consider an approval request once biesi's
comments have been addressed.

Comment 33

[Cedric Vivier [:cedricv]]

(In reply to comment #31)
> but any code could be holding a reference to that nsIWidget, could it not?

perhaps, but as the destructor is called even when it freezes, this can't be the
case or at least it isn't related to the bug isn't it?

Comment 34

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

I'm especially concerned that new code might be written that holds nsIWidget
pointers.

Comment 35

[Charles Melhorn]

Don't know if this adds anything to the discussion at this point, but for the
record:

1. Using the steps I outlined in comment #25, I can reproduce this bug every
time (under Windows 2000 Pro). When I replace the call to window.openDialog()
with window.open(), the problem with the dialog window freezing on close goes away.

2. Note that when the dialog is opened with openDialog(), the window still
freezes on close even if window.close() is called using setTimeout(), which may
indicate something about whether the problem is threads-related?

3. Before checking in the patch, it might be good to try and determine how the
problem was introduced into the code since this behavior isn't seen, to my
knowledge, in Firefox 1.0.x.

Comment 36

[Robert Accettura [:raccettura]]

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

Comment 37

[Charles Melhorn]

I went back and tested for this bug in earlier releases and nightlies using the
procedure I described in comment #25 (modifying the install manifest of my
extension as necessary). When I perform these steps with the Deer Park Alpha 2
release (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b3)
Gecko/20050712 Firefox/1.0+), there is a noticeable delay of ~2-3 seconds when
the window is closed before it is removed, but it does NOT freeze.

The delay in closing, which I don't see in Firefox 1.0.x, appears to go all the
way back to at least the first nightly 1.1 build (Mozilla/5.0 (Windows; U;
Windows NT 5.0; en-US; rv:1.8a6) Gecko/20041205 Firefox/1.0+).

Comment 38

[Ere Maijala (slow)]

This sounds very much the same as bug 307563, for which I now have a patch.

Comment 39

[Charles Melhorn]

Ere: I tested the fix for bug 307563 on the trunk (Mozilla/5.0 (Windows; U;
Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051015 Firefox/1.6a1).

There is still a slight problem: the dialog window will remain on the screen
after I perform the steps I describe in comment #25 when I drag/move the mouse
as I'm clicking on the close button. The window remains on the screen until I
move the mouse cursor off the window, after which it disappears.

So something about the mouse trailer code is still not quite right?

Comment 40

[Ere Maijala (slow)]

We should probably free holdMouseWindow and CaptureWindow when the window is
being destructed to be perfect. I'd like to test the steps in comment #25, but
the extension says it's only compatible with 1.4. Charles, could you create a
version compatible with 1.6a2 so that testing would be easier?

Comment 41

[Charles Melhorn]

Ere: Sure. Just a matter of bumping the target application max version number
for the extension. Try:

http://www.dourlad.com/dllaboratories/mdhashtool-0.4dp_16a.xpi

Note that all my testing has been under Windows 2000 Pro, using Firefox with the
classic theme.

Comment 42

[Ere Maijala (slow)]

One possibly stupid question: how do you drag while clicking? I thought you
might be using the keyboard to click the close button, but I didn't succeed in
dragging while doing that.

Comment 43

[Charles Melhorn]

Sorry for the imprecise description. What I mean is that sometimes I see the
behavior where the dialog window stays on the screen until I move the mouse
cursor off the window, and it seems to happen more often when I'm moving the
mouse from right to left as I'm clicking on the close button (after performing
the steps in comment #25). It's inconsistent and usually I have to repeat
opening and closing the dialog a few times, but I'm pretty sure there is
something that's still not quite right. Maybe it's not worth pursuing, though.

Comment 44

[Ere Maijala (slow)]

Attachment: Patch v1

This patch makes it so that the mouse trailer window is always nullified if the
window is being destroyed.

Comment 45

[Ere Maijala (slow)]

Fix checked in to trunk. Charles, could you verify so I could request branch
approval, please?

Comment 46

[Charles Melhorn]

That did the trick. Thanks, Ere.

Verified on the trunk: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1)
Gecko/20051018 Firefox/1.6a1

Comment 47

[Ere Maijala (slow)]

Comment on attachment 199944 [details] [diff] [review]
Patch v1

Seeking approval. Low risk potentially high visibility follow-up to the
previous mouse trailer fix. Windows only.

Comment 48

[Asa Dotzler [:asa]]

Ere, in addition to testing that this patch works, what areas would we need to
test to be sure nothing else regressed. Also, how common is this freeze? If the
hang isn't really there any more (and if Reporter is fully functional after your
other fix) then this isn't a compelling change for the branch.

Also, I'm resolving as fixed since the change has landed and been verified on
the trunk. 

Comment 49

[Ere Maijala (slow)]

Asa: while I believe this fix is very safe, to be perfect you should test all
window or dialog closures (the patch just makes nsWindow remove a possible
reference to itself when closing). I haven't seen this other than when I
deliberately tried, but it's probably more easily reproducable on slower or busy
machines (besides, I haven't been running branch versions much). 

Anyway, the problem could occur when closing a maximized window or the last
window. The former one could be remedied by bringing up another window, but the
latter case would cause failure to close the window and shut down the browser
correctly.

Comment 50

[Asa Dotzler [:asa]]

OK, let's get this verified on the trunk, testing all possible regression paths
and then we'll consider again for the branch.

Comment 51

[Asa Dotzler [:asa]]

Comment on attachment 199944 [details] [diff] [review]
Patch v1

after further consideration, we think it's too late for this remaining issue
which is very low impact.

Comment 52

[Boris Zbarsky [:bzbarsky]]

This looks like it caused bug 314543

Comment 53

[Ere Maijala (slow)]

The patch had to be backed out due to a severe regression. Work will continue in bug 312566.