Closed Bug 141056 Opened 22 years ago Closed 20 years ago

crash running some xpi [@JS_FrameIterator]

Categories

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

x86
All
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: geofforama, Assigned: dveditz)

References

()

Details

(Keywords: crash)

Crash Data

Attachments

(2 files, 1 obsolete file)

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.
tb5752645g for the talkback

(doron@netscape.com has it)

[@JS_FrameIterator]

xpconnect
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, stackwanted
Summary: SEGFAULT in XPInstall → crash running some xpi [@JS_FrameIterator]
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)
Jband, does this ring any bells for you?
FWIW the JSContext passed in is null. Looks like LocationImpl::CheckURL didn't
dig up a good JSContext via stack->Peek.
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?
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.
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?
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.
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.
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.
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.
Keywords: stackwantednsbeta1+
Whiteboard: [adt2 rtm]
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
Attachment #81883 - Flags: superreview+
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
Attachment #81883 - Flags: review+
David, can you look into landing this? Thanks. Low priority, but it still should
get in sometime.

syd
Assignee: dveditz → dprice
The patch doesn't build.  Working on a new one.
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
up to date tree == happy building.  asking drivers for approval
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
Attachment #81883 - Flags: approval+
on the trunk.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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)  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reassigning to Syd since David is not here.

Thanks!
Assignee: dprice → syd
Status: REOPENED → NEW
over to me.
Assignee: syd → ssu
Installer triage team: nsbeta1-
Assignee: ssu → dveditz
Keywords: nsbeta1+nsbeta1-
Whiteboard: [adt2 rtm]
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
Attachment #81883 - Attachment is obsolete: true
#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
Attached patch patchSplinter Review
patch (by timeless's help and request)
Comment on attachment 142740 [details] [diff] [review]
patch

this of course generates noise when you actually trigger this case...
Attachment #142740 - Flags: review?(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.
Attachment #142740 - Flags: review?(jst) → review+
(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.
Awesome. Let's get reviews n' all on these patches then.
Attachment #142780 - Flags: superreview?(bzbarsky)
Attachment #142780 - Flags: review?(bzbarsky)
Comment on attachment 142780 [details] [diff] [review]
Make XPInstall push a JS context on the cx stack before calling JS.

r+sr=bzbarsky
Attachment #142780 - Flags: superreview?(bzbarsky)
Attachment #142780 - Flags: superreview+
Attachment #142780 - Flags: review?(bzbarsky)
Attachment #142780 - Flags: review+
Checked in both patches. FIXED.
Status: NEW → RESOLVED
Closed: 22 years ago20 years ago
Resolution: --- → FIXED
Comment on attachment 142780 [details] [diff] [review]
Make XPInstall push a JS context on the cx stack before calling JS.

moa=dveditz
Crash Signature: [@JS_FrameIterator]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: