141056 - crash running some xpi [@JS_FrameIterator]

Status: RESOLVED FIXED

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

Description

[Geoff Smith]

Problems happens when you set a callback for install and redirect to a new page
in the callback.
The code in the above page is very simple.
The only workaround I could find was the writeln a page that did a META refresh
to the page I wanted to go to.  However, as of 0.9.9 even this workaround no
longer works. (which may be another bug)
Problem has been confirmed in linux and windows 98 using recent builds.

Comment 1

[Geoff Smith]

tb5752645g for the talkback

([e-mail] has it)

[@JS_FrameIterator]

xpconnect

Comment 2

[Jimmy Lee]

Build: 2002-04-29-08-1.0.0(WIN)

Ccing Grace.  I can reproduce the crash.  I could crash electing to Install or
Cancel.

Incident ID: 5780765

JS_FrameIterator [d:\builds\seamonkey\mozilla\js\src\jsdbgapi.c, line 622]
nsScriptSecurityManager::GetPrincipalAndFrame
[d:\builds\seamonkey\mozilla\caps\src\nsScriptSecurityManager.cpp, line 1453]
nsScriptSecurityManager::GetSubjectPrincipal
[d:\builds\seamonkey\mozilla\caps\src\nsScriptSecurityManager.cpp, line 1495]
nsScriptSecurityManager::CheckLoadURIFromScript
[d:\builds\seamonkey\mozilla\caps\src\nsScriptSecurityManager.cpp, line 823]
LocationImpl::CheckURL [d:\builds\seamonkey\mozilla\dom\src\base\nsLocation.cpp,
line 205]
LocationImpl::SetHrefWithBase
[d:\builds\seamonkey\mozilla\dom\src\base\nsLocation.cpp, line 517]
LocationImpl::SetHref [d:\builds\seamonkey\mozilla\dom\src\base\nsLocation.cpp,
line 469]
nsWindowSH::SetProperty
[d:\builds\seamonkey\mozilla\dom\src\base\nsDOMClassInfo.cpp, line 2956]
XPC_WN_Helper_SetProperty
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp,
line 793]
js_SetProperty [d:\builds\seamonkey\mozilla\js\src\jsobj.c, line 2666]
js_Interpret [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2586]
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 806]
js_InternalInvoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 881]
JS_CallFunctionValue [d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 3414]
handleTriggerEvent
[d:\builds\seamonkey\mozilla\xpinstall\src\nsXPITriggerInfo.cpp, line 174]
PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 597]
PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c,
line 530]
_md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line
1078]
nsAppShellService::Run
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp, line 309]
main1 [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1434]
main [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1769]
WinMain [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1787]
WinMainCRTStartup()
KERNEL32.DLL + 0x17d08 (0x77e97d08)

Comment 3

[Johnny Stenback (:jst)]

Jband, does this ring any bells for you?

Comment 4

[David Bradley]

FWIW the JSContext passed in is null. Looks like LocationImpl::CheckURL didn't
dig up a good JSContext via stack->Peek.

Comment 5

[John Bandhauer]

Yeah, from what dbradley said it sounds like it is likely bad assumptions about
always running from within a window and being able to construct relative URLs
based on the window's URL. Patrick Beard saw something like this in plugin land
a while back. In that case the plugin really *was* in a window, but was
responding to events on its own (no Window's JSContext pushed on the stack). He
added code that the plugin could manually call that would push and pop the
appropriate window's JSContext to bracket the processing of the event.

The normal DOM events *do* bracket event processing with a push and pop of the
window's JSContext. Perhaps handleTriggerEvent will need to do that. Does it
*have* an appropriate window to use? The context stack's safeContext might be
appropriate if nothing else is available. Though, the location code might still
puke if the JSContext is not a DOM JSContext. The safeContext is that of the
hidden window (a DOM context) in normal cases after the browser is all started
up and stuff. But might the xpi code sometimes be running with no windows?

Comment 6

[John Bandhauer]

Oh, (this is probably obvious) and maybe the LocationImpl::CheckURL should be
erroring out if cx == nsnull. This isn't going to make the caller much happier,
but it is better than crashing. The stack->Peek *does* yield nsnull without
returning an error code.

Comment 7

[Johnny Stenback (:jst)]

Attachment: Don't bother calling the security manager if there's no JS context on the stack.

This should fix the crash. This does not make resolving relative URL's in the
case where we crash work and I don't know that we need to make that work, if we
do, we need to add code that pushes/popps the JS context onto/from the context
stack in handleTriggerEvent() in nsXPITriggerInfo.cpp. Doing that is easy if
it's really needed. Dveditz, do we need that, or are the URL's in question
always absolute?

Comment 8

[John Bandhauer]

jst: I'm not convinced that change is safe. The principals of the JS code
determine if it ought to be trusted or not. The fact that some event can get
into JS code without going through the DOM does not necessarily mean that the JS
code is really to be trusted. It seems to me that your change might contribute
to a possible security exploit. It may be better for this code to fail and force
the code pushing events into the system to make sure a JSContext is on the stack
rather than just skip the security check. Convince me that I'm wrong.

Comment 9

[Johnny Stenback (:jst)]

We must support calling this code w/o JS on the stack anyway since that method
will be called when nsLocation::SetHref() is called from C++, Python, Perl, or
whatever (once we're able to call XPCOM interfaces from those methods). The fix
is safe, code that executes JS w/o pushing contexts onto the stack is not safe.
If all code that wants to execute JS would call through the script context we'd
be all set since it deals with pushing the JSContext onto the stack. IOW, this
was a crash waiting to happen when someone called nsLocation::SetHref() from
C++, we just didn't know about it yet.

Comment 10

[Geoff Smith]

Is a fix for this going to make it into RC3?  I think redirecting to a web page
after install of an .xpi is a pretty standard thing to do, so I imagine others
will be affected.  If a fix won't make it into RC3, does anyone know of a
workaround?  Right now I display a tacky looking alert box after install.

Comment 11

[Daniel Veditz [:dveditz]]

This will surely impact any site-side implementation of updates after an update
notification, or a security fix patch, maybe even the plugin finder depending on
how it's implemented.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 81883 [details] [diff] [review]
Don't bother calling the security manager if there's no JS context on the stack.

sr=dveditz

Comment 13

[Håkan Waara]

Comment on attachment 81883 [details] [diff] [review]
Don't bother calling the security manager if there's no JS context on the stack.

r=hwaara

Comment 14

[Syd Logan]

David, can you look into landing this? Thanks. Low priority, but it still should
get in sometime.

syd

Comment 15

[dprice (gone)]

The patch doesn't build.  Working on a new one.

Comment 16

[dprice (gone)]

actually I think it is the age of my tree that's causing my build problems.  It
should work after I update my whole tree

Comment 17

[dprice (gone)]

up to date tree == happy building.  asking drivers for approval

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 81883 [details] [diff] [review]
Don't bother calling the security manager if there's no JS context on the stack.

a=asa (on behalf of drivers) for checkin to 1.1

Comment 19

[dprice (gone)]

on the trunk.

Comment 20

[Jimmy Lee]

Build: 2002-09-10-14-trunk(WIN)

I am still able to crash after clicking BOOM button from URL mentioned above. 
Reopening bug. :-(

Incident ID: 10717425

Call Stack:     (Signature = JS_GetOptions 388e8644)
     JS_GetOptions [c:/builds/seamonkey/mozilla/js/src/jsapi.c, line 1004]
     nsJSUtils::GetDynamicScriptContext
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSUtils.cpp, line 231]
     GetDocumentCharacterSetForURI
[c:/builds/seamonkey/mozilla/dom/src/base/nsLocation.cpp, line 84]
     GetDocumentCharacterSetForURI
[c:/builds/seamonkey/mozilla/dom/src/base/nsLocation.cpp, line 84]
     LocationImpl::SetHrefWithBase
[c:/builds/seamonkey/mozilla/dom/src/base/nsLocation.cpp, line 468]
     LocationImpl::SetHref
[c:/builds/seamonkey/mozilla/dom/src/base/nsLocation.cpp, line 437]
     nsWindowSH::SetProperty
[c:/builds/seamonkey/mozilla/dom/src/base/nsDOMClassInfo.cpp, line 3047]
     XPC_WN_Helper_SetProperty
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 793]
     js_SetProperty [c:/builds/seamonkey/mozilla/js/src/jsobj.c, line 2700]
     js_Interpret [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 2648]
     js_Invoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 857]
     js_InternalInvoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 932]
     JS_CallFunctionValue [c:/builds/seamonkey/mozilla/js/src/jsapi.c, line 3433]
     handleTriggerEvent
[c:/builds/seamonkey/mozilla/xpinstall/src/nsXPITriggerInfo.cpp, line 174]
     PL_HandleEvent [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 644]
     PL_ProcessPendingEvents
[c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 577]
     _md_EventReceiverProc [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c,
line 1309]
     nsAppShellService::Run
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp, line 472]
     main1 [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1524]
     main [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1871]
     WinMain [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1889]
     WinMainCRTStartup()  
     KERNEL32.DLL + 0x1ca90 (0x77e9ca90)  

Comment 21

[Jimmy Lee]

Reassigning to Syd since David is not here.

Thanks!

Comment 22

[Sean Su]

over to me.

Comment 23

[Samir Gehani]

Installer triage team: nsbeta1-

Comment 24

[timeless]

Comment on attachment 81883 [details] [diff] [review]
Don't bother calling the security manager if there's no JS context on the stack.

mozilla/dom/src/base/nsLocation.cpp	1.101

Comment 25

[basic]

#0  0x40810006 in nanosleep () from /lib/i686/libc.so.6
#1  0xffffffa0 in ?? ()
#2  0x08062316 in ah_crap_handler(int) (signum=11) at nsSigHandlers.cpp:135
#3  0x41e2d1d6 in nsProfileLock::FatalSignalHandler(int) (signo=0)
    at /mnt/build/mozilla/profile/dirserviceprovider/src/nsProfileLock.cpp:209
#4  0x4022f4ec in __pthread_clock_settime () from /lib/i686/libpthread.so.0
#5  0x4078dca8 in __libc_sigaction () from /lib/i686/libc.so.6
#6  0x41af92ec in nsJSUtils::GetDynamicScriptContext(JSContext*) (aContext=0x0)
    at /mnt/build/mozilla/dom/src/base/nsJSUtils.cpp:222
#7  0x41af92ac in nsJSUtils::GetDynamicScriptGlobal(JSContext*) (aContext=0x0)
    at /mnt/build/mozilla/dom/src/base/nsJSUtils.cpp:213
#8  0x41af4781 in GetDocumentCharacterSetForURI (aHref=@0xbfffe9d0,
    aCharset=@0xbfffe7b0)
    at /mnt/build/mozilla/dom/src/base/nsLocation.cpp:88
#9  0x41af67c5 in LocationImpl::SetHrefWithBase(nsAString const&, nsIURI*, int)
    (this=0x86e6648, aHref=@0xbfffe9d0, aBase=0x8a131f0, aReplace=0)
    at /mnt/build/mozilla/dom/src/base/nsLocation.cpp:542
#10 0x41af661e in LocationImpl::SetHref(nsAString const&) (this=0x86e6648,
    aHref=@0xbfffe9d0)
    at /mnt/build/mozilla/dom/src/base/nsLocation.cpp:503
#11 0x41b11c41 in nsWindowSH::SetProperty(nsIXPConnectWrappedNative*,
JSContext*, JSObject*, long, long*, int*) (this=0x8621ad0, wrapper=0x8621c80,
		tcx=0x84b8558, obj=0x85fa9a0, id=141452872, vp=0xbfffed34,
    _retval=0x41cc9b88)
    at /mnt/build/mozilla/dom/src/base/nsDOMClassInfo.cpp:3225
#12 0x40b50b0c in XPC_WN_Helper_SetProperty (cx=0x84b8558, obj=0x85fa9a0,
    idval=135003780, vp=0xbfffed34)
    at /mnt/build/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:818
#13 0x4006e636 in js_SetProperty (cx=0x84b8558, obj=0x85fa9a0, id=136423896,
    vp=0xbfffed34) at /mnt/build/mozilla/js/src/jsobj.c:2836
#14 0x4005d841 in js_Interpret (cx=0x84b8558, result=0xbfffedec)
    at /mnt/build/mozilla/js/src/jsinterp.c:3044
#15 0x4005273c in js_Invoke (cx=0x84b8558, argc=2, flags=2)
    at /mnt/build/mozilla/js/src/jsinterp.c:958
#16 0x40052a41 in js_InternalInvoke (cx=0x84b8558, obj=0xfffffffc, fval=-4,
    flags=2, argc=2, argv=0x8a1bd90, rval=0xfffffffc)
    at /mnt/build/mozilla/js/src/jsinterp.c:1035
#17 0x4002b6d1 in JS_CallFunctionValue (cx=0x84b8558, obj=0x85fa9a0,
    fval=140489184, argc=2, argv=0x8a1bd90, rval=0xbfffef78)
    at /mnt/build/mozilla/js/src/jsapi.c:3589
#18 0x42485075 in handleTriggerEvent (event=0x89f2b50)
    at /mnt/build/mozilla/xpinstall/src/nsXPITriggerInfo.cpp:189
#19 0x4017d513 in PL_HandleEvent (self=0x89f2b50)
    at /mnt/build/mozilla/xpcom/threads/plevent.c:671
#20 0x4017d359 in PL_ProcessPendingEvents (self=0x8130a78)
    at /mnt/build/mozilla/xpcom/threads/plevent.c:606
#21 0x401802a3 in nsEventQueueImpl::ProcessPendingEvents() (this=0x80d6448)
    at /mnt/build/mozilla/xpcom/threads/nsEventQueue.cpp:391
#22 0x41348935 in event_processor_callback (source=0x8279468,
    condition=G_IO_IN, data=0x408919a0)
    at /mnt/build/mozilla/widget/src/gtk2/nsAppShell.cpp:67

Comment 26

[basic]

Attachment: patch

patch (by timeless's help and request)

Comment 27

[timeless]

Comment on attachment 142740 [details] [diff] [review]
patch

this of course generates noise when you actually trigger this case...

Comment 28

[Johnny Stenback (:jst)]

Attachment: Make XPInstall push a JS context on the cx stack before calling JS.

Does this also fix the crash?

Comment 29

[Johnny Stenback (:jst)]

Comment on attachment 142740 [details] [diff] [review]
patch

   rv = stack->Peek(&cx);
   NS_ENSURE_SUCCESS(rv, rv);
+  NS_ENSURE_TRUE(cx, NS_ERROR_FAILURE);

just do:

  if (!cx)
    return NS_OK;

since there's nothing wrong with not having a JSContext on the stack here.

r=jst, but I think the patch I attached is the more correct fix for this bug.

Comment 30

[basic]

(In reply to comment #28)
> Created an attachment (id=142780)
> Make XPInstall push a JS context on the cx stack before calling JS.
> 
> Does this also fix the crash?
Yes this also fix the crash.

Comment 31

[Johnny Stenback (:jst)]

Awesome. Let's get reviews n' all on these patches then.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 142780 [details] [diff] [review]
Make XPInstall push a JS context on the cx stack before calling JS.

r+sr=bzbarsky

Comment 33

[Johnny Stenback (:jst)]

Checked in both patches. FIXED.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 142780 [details] [diff] [review]
Make XPInstall push a JS context on the cx stack before calling JS.

moa=dveditz