52827 - Crash after triggering after calling alert dialog from xpi package [@ js_AllocRawStack]

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect, P2)

Description

[Jimmy Lee]

Build: 2000-09-15-08-M18(WIN)

1. From http://jimbob/trigger3.html, click on Functional Test Case drop-down 
   menu and choose f_alertconfirm
2. Click Trigger case button
3. Click OK button from confirm dialog
4. Click OK button from alert dialog that appears generated by the install
   Notice the little untitled dialog appears in top-left corner
5. Click OK for next alert
6. Click OK or Cancel from confirm dialog
7. Click OK for next alert.  Install should be completed now.
8. Close little untitled dialog in top-left corner
9. Trigger any xpi package

RESULT:
Crash attempting to install next xpi package.

EXPECTED RESULT:
No crash.  No little untitled dialog appears in the first place.

TalkBack Incident ID = 17492975

Stack Trace:
js_AllocRawStack [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 354] 
js_AllocStack [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 387] 
JS_PushArgumentsVA [d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 293] 
JS_PushArguments [d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 268] 
nsInstallProgressDialog::Open 
[d:\builds\seamonkey\mozilla\xpinstall\src\nsInstallProgressDialog.cpp, line 
170] 
XPTC_InvokeByIndex 
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp, 
line 139] 
EventHandler [d:\builds\seamonkey\mozilla\xpcom\proxy\src\nsProxyEvent.cpp, line 
510] 
nsProxyObject::Post 
[d:\builds\seamonkey\mozilla\xpcom\proxy\src\nsProxyEvent.cpp, line 450] 
nsProxyEventObject::CallMethod 
[d:\builds\seamonkey\mozilla\xpcom\proxy\src\nsProxyEventObject.cpp, line 431] 
PrepareAndDispatch 
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcstubs.cpp, 
line 102] 
SharedStub 
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcstubs.cpp, 
line 125]

Comment 1

[Jimmy Lee]

Nominating for nsbeta3.  This affects all windows platforms.  The impact of 
putting alerts in installscripts is not clear to me, but the consequences of 
doing so are critical.

Because this bug resides in this engine now, upgrading from PR3 to any future 
milestone would be affected.  I do not know if Netcenter or any other site 
intends to use alerts in installations.

This bug may be related to Bug 50942.

Comment 2

[Daniel Veditz [:dveditz]]

This is more modal dialog cruft. We reall need to be able to use confirm() at 
least -- alert() and confirm() are the only script-generated UI possible in an 
install script.

Don -- nsXPInstallManager::DialogOpened() is called with a DOM window. Instead 
of throwing it away let's save it in a member variable, and then down in 
DownloadNext when we call softupdate->InstallJar() we could pass it in as an 
argument. From there add it into the InstallInfo structure and pretty much 
follow along what I had to do with the chrome registry.

Comment 3

[dbragg]

Accepting

Comment 4

[dbragg]

Attachment: Diff for fix of bug 52827

Comment 5

[Brendan Eich [:brendan]]

Did you leave a file out?  I don't see the definition of the modified (to take
an nsIDOMWindowInternal*, ugh) InitXPInstallObjects.  BTW, is that really the
name of an extern function?  It should have an NS_ prefix or somesuch.

/be

Comment 6

[dbragg]

Nope.  Didn't leave out any files but I understand your confusion.  The diff 
just cut off the function name.  InitXPInstallObjects is defined right in 
nsJSInstall.cpp.  Any functions that use it declare it as extern.

It's old code and should be re-written but as we all know this ain't the time.

Comment 7

[selmer (gone)]

pdt agrees P2.

Comment 8

[Bill Law]

It appears that there are some calls to InstallJar that pass 0 for the parent 
window.  I suppose no alert/confirm dialogs will ever appear in those cases?

Aside from that, r=law.

Comment 9

[Daniel Veditz [:dveditz]]

When the engine is called by the native install wizards (via xpistub.dll) we 
have no layout engine and the install engine does not do any UI.

Comment 10

[buster]

Is there any chance this checkin caused the big jump in leaks?  I see it uses an 
nsCOMPtr member variable to hold onto a nsIDOMWindowInternal in several places.  
Are we sure that the object that contains the mParentWindow is not leaked?
This is just a request for someone who knows the code to verify that the 
destructors get called during the smoketest. 

Comment 11

[Daniel Veditz [:dveditz]]

I doubt we're causing the t-box leaks because our objects wouldn't get created 
unless you're trying to install something (which the tests don't do) -- but 
you're right, Don found that it does cause a leak when we run our stuff.

Was this one checked in? The bug is not marked fixed. But there was a similar 
bug that caused the addition of the new member variable

Comment 12

[dbragg]

Fix checked in yesterday.  Now we're saving the parent window in mParentWindow 
in the nsXPInstallManager object and passing that to the dialog creation calls 
as the parent window.

Comment 13

[Jimmy Lee]

Build: 2000-09-21-09-M18(WIN), 2000-09-21-04-M18(MAC), 2000-09-21-08-M18(LINUX)

No problems now with Windows.  No side affects observed from Mac and Linux.

Marking Verified!