207742 - Mozilla crash after closing blank window created when exiting screensaver

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[quantumet]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030529
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030529

After I return to my computer when my screensaver has kicked on, with Mozilla
running, a small blank mozilla window has opened up.  Closing this window will
cause Mozilla to crash when I click on a link on some other open page.  
The screensaver causing this interaction is the matrix_ks screensaver from
http://www.kellysoftware.com/ssaver/Matrix_ks.asp


Reproducible: Always

Steps to Reproduce:
1. Download and install screensaver from
http://www.kellysoftware.com/ssaver/Matrix_ks.asp
2. Open a mozilla window
3. Activate screensaver (preview in display options is sufficient)
4. Close screensaver

Actual Results:  
A small blank Mozilla window appears (with red dino logo).
Closing this window will cause a Mozilla crash upon clicking a link.


Expected Results:  
No blank window should appear.

Pinball theme, untested with default.

Comment 1

[quantumet]

Talkback crash ID for one incident: TB20604833H

Comment 2

[Matthias Versen [:Matti]]

Incident ID 20604833
Stack Signature 	js_SuspendThread da78cdac
Product ID 	Gecko1.4
Build ID 	2003052908
Trigger Time 	2003-05-31 02:35:10
Platform 	Win32
Operating System 	Windows NT 5.0 build 2195
Module 	js3250.dll
URL visited 	
User Comments 	
Trigger Reason 	Access violation
Source File Name 	d:/builds/seamonkey/mozilla/js/src/jslock.c
Trigger Line No. 	913
Stack Trace 	
js_SuspendThread [d:/builds/seamonkey/mozilla/js/src/jslock.c, line 913]
js_Enqueue [d:/builds/seamonkey/mozilla/js/src/jslock.c, line 957]
js_Lock [d:/builds/seamonkey/mozilla/js/src/jslock.c, line 994]
js_AtomizeString [d:/builds/seamonkey/mozilla/js/src/jsatom.c, line 573]
js_Atomize [d:/builds/seamonkey/mozilla/js/src/jsatom.c, line 655]
JS_NewFunction [d:/builds/seamonkey/mozilla/js/src/jsapi.c, line 2852]
XPCNativeMember::Resolve
[d:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativeinfo.cpp, line
147]
DefinePropertyIfFound
[d:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 442]
XPC_WN_ModsAllowed_Proto_Resolve
[d:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1415]
js_LookupProperty [d:/builds/seamonkey/mozilla/js/src/jsobj.c, line 2321]
js_GetProperty [d:/builds/seamonkey/mozilla/js/src/jsobj.c, line 2477]
js_Interpret [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 2684]
js_Invoke [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 861]
js_InternalInvoke [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 936]
JS_CallFunctionValue [d:/builds/seamonkey/mozilla/js/src/jsapi.c, line 3529]
nsJSContext::CallEventHandler
[d:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1114]
nsJSEventListener::HandleEvent
[d:/builds/seamonkey/mozilla/dom/src/events/nsJSEventListener.cpp, line 183]
nsEventListenerManager::HandleEventSubType
[d:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp, line
1192]
nsEventListenerManager::HandleEvent
[d:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp, line
2193]
nsXULElement::HandleDOMEvent
[d:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp, line 3302]
PresShell::HandleDOMEventWithTarget
[d:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6461]
nsPopupSetFrame::OnCreate
[d:/builds/seamonkey/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp, line 610]
nsPopupSetFrame::ShowPopup
[d:/builds/seamonkey/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp, line 352]
nsPopupBoxObject::ShowPopup
[d:/builds/seamonkey/mozilla/layout/xul/base/src/nsPopupBoxObject.cpp, line 194]
nsXULTooltipListener::LaunchTooltip
[d:/builds/seamonkey/mozilla/layout/xul/base/src/nsXULTooltipListener.cpp, line 516]
nsXULTooltipListener::ShowTooltip
[d:/builds/seamonkey/mozilla/layout/xul/base/src/nsXULTooltipListener.cpp, line 409]
nsTimerImpl::Fire [d:/builds/seamonkey/mozilla/xpcom/threads/nsTimerImpl.cpp,
line 383]
nsTimerManager::FireNextIdleTimer
[d:/builds/seamonkey/mozilla/xpcom/threads/nsTimerImpl.cpp, line 616]
nsAppShell::Run [d:/builds/seamonkey/mozilla/widget/src/windows/nsAppShell.cpp,
line 143]
nsAppShellService::Run
[d:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp, line 479]
main1 [d:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1284]
main [d:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1650]
WinMain [d:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1672]
WinMainCRTStartup()
KERNEL32.dll + 0x2847c (0x77ea847c)


dbradley: 
Sorry that I bug you again with this :-(
Do you have a clue who should get this ? (Events ?)

Comment 3

[David Bradley]

Attachment: Purify data on the crash

Looks like this is caused by XPConnect holding onto a dead context.

I think we have two issues here. One is the crash. The other is why is this
window being created. I can address the crash issue, but not sure I'll be able
to figure out why the window is being created.

Comment 4

[David Bradley]

I'll take this for now and get the crash addressed. This might be a good
reproduceable case for some other bugs dealing with dead JS contexts. We'll see
what turns up.

Comment 5

[David Bradley]

I've tracked this down to the fact that this screen saver is causing the hidden
window to be visible. Not sure how it's doing that exactly. I think the hidden
window should refuse any request to make it visible.

The hidden window's context is set as XPConnect's safe JSContext and because
this window becomes visible, if the user closes it, it pretty much hoses
XPConnect and causes a crash.

Comment 6

[Phil Schwartau]

Setting default QA -

Comment 7

[Dan M]

Well that's bloody rude, making the hidden window visible. An app can spew
WM_SHOWWINDOW messages anywhere, even to another app's window. But why would it,
unless it's just evil? Evil!?

I'm not sure how to fix this. For one thing I'm having trouble overriding a
window show. I think the WM_SHOWWINDOW message is just informative; it's not an
opportunity to cancel. The best I can think of is to position the hidden window
offscreen and re-hide it as soon as anyone slams it. But that's really not
pretty. Anybody know a better way to outclever this &*#%*&&#(% buggy screensaver?

Comment 8

[David Bradley]

Returning 0 from WM_SHOWWINDOW and not calling the default window proc should
tell Windows you've handled the message so it doesn't do anything, effectively
ignoring the request. But it's been a while since I toyed with processing raw
windows message. I have no idea how to implement that within mozilla's event
mechanism.

Comment 9

[Dan M]

That's done easily enough. But the problem remains that the WM_SHOWWINDOW
message seems to be merely informative. Windows doesn't care whether you process
it yourself or what you return if you do or even whether you post one of your own.

Comment 10

[David Bradley]

Actually I think I was thinking of WM_WINDOWPOSCHANGING. I spy'd the hidden
window and that's the second message that comes across, right after WM_APPACTIVATE.

It's still a bit odd, as I created an app with a hidden window, the screen saver
didn't make it appear, although it received the WM_WINDOWPOSCHANGING as well. So
I don't know if real problem is the way Mozilla handles WM_APPACTIVATE or
something else.

I did test my app, and setting the flag in the WINDOWPOS structure will prevent
the window from being displayed. 

Comment 11

[Dan M]

Yeah, that's it. Thanks David! It's WM_WINDOWPOSCHANGING that's important when a
window is being made visible, not WM_SHOWWINDOW. I guess I can't complain about
surprising APIs since I've made some myself for Mozilla.

Comment 12

[Dan M]

Attachment: test application (source and exe): unzip and execute

Windows test application that does the same thing. It's simpler to run this
than to install the screen saver. It hunts down an invisible top-level Mozilla
window and makes it visible. Its name is "rude.exe" because what else would you
call such a thing?

Comment 13

[Dan M]

Attachment: invisible window cannot be made visible (Windows OS)

Conceptually simple but in practice a little messy. This patch simply transmits
information to the widget level that the widget is supposed to be invisible and
then (Windows-only) overrides any attempt to make it visible. It transmits this
information by making cross-platform some appshell code, currently compiled
only on the Mac. Since this is implemented as a whole new widget type, the bulk
of this patch is teaching widget code what to think of the new widget type.

Comment 14

[Christopher Blizzard (:blizzard)]

What platforms create the hidden window?  I'm just wondering because if OSs
other than Mac and win32 don't, then there's no point in even touching the code
for those platforms, right?

Comment 15

[Dan M]

Everybody makes a hidden window. Those parts of the patch that affect the
gtk-like platforms are necessary because without them Mozilla doesn't know how
to realize the hidden window and hangs on startup.

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 127214 [details] [diff] [review]
invisible window cannot be made visible (Windows OS)

Looks ok. sr=bryner.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 127214 [details] [diff] [review]
invisible window cannot be made visible (Windows OS)

Blizzard's away unavoidably for a few days, but I can r= this.

Only thought for later is this: maybe we are conflating type and flags?  You
could imagine a flag for whether the window has a parent, a flag for
top-level-ness, and an invisible flag.	Using flags might simplify some of the
logic.

/be

Comment 18

[Dan M]

I agree: as implemented, eWindowType_invisible is a variant of
eWindowType_toplevel, so it doesn't sit well being noted as a unique type. It's
a little icky as it stands and it'll be a problem if someday we want to make
windows other than _toplevel invisible. But I don't anticipate ever wanting to
do that. In current usage _invisible really is a unique kind of window. It might
be more palatable renamed as eWindowType_hidden. But a separate flag would
probably be best.

The patch went in this morning and I haven't heard any complaints about broken
builds. (The trouble I was anticipating would have been that the app no longer
launched on some platforms. Hey no such luck, though.) Calling it fixed.

Comment 19

[Sergei Dolgov]

Dan M,
but does nsWindow existence require also creation of native counterparts, in
order to realize widget properly?
In BeOS we had rather several problems if we create BWindow/BView for those
invisible beasts, so i hope that if we create only nwWidget, it is sufficient.