Closed Bug 277069 Opened 20 years ago Closed 20 years ago

venkman/jsd exposed a rooting problem (last ditch gc?) [@ str_resolve]

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: timeless, Assigned: brendan)

References

Details

(Keywords: crash, fixed-aviary1.0.1, fixed1.7.6)

Crash Data

Attachments

(4 files, 2 obsolete files)

js3250.dll!str_resolve(JSContext * cx=0x07da1668, JSObject * obj=0x11175168,
long id=5)  Line 568 + 0x2	C
>	js3250.dll!js_LookupProperty(JSContext * cx=0x00000000, JSObject *
obj=0x11175168, long id=5, JSObject * * objp=0x0012df70, JSProperty * *
propp=0x0012df74)  Line 2451 + 0x10	C
 	js3250.dll!js_GetProperty(JSContext * cx=0x07da1668, JSObject *
obj=0x11175168, long id=5, long * vp=0x0012e0cc)  Line 2594 + 0x16	C
 	js3250.dll!js_Interpret(JSContext * cx=0x07da1668, long * result=0x00000000) 
Line 2820	C
 	js3250.dll!js_Invoke(JSContext * cx=0x00ad803b, unsigned int argc=131733096,
unsigned int flags=0)  Line 958 + 0xa	C
 	js3250.dll!js_Interpret(JSContext * cx=0x07da1668, long * result=0x00000000) 
Line 2971	C
 	js3250.dll!js_Invoke(JSContext * cx=0x00ad803b, unsigned int argc=131733096,
unsigned int flags=0)  Line 958 + 0xa	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x07da1694, JSObject *
obj=0x079ece08, long fval=180500104, unsigned int flags=0, unsigned int argc=2,
long * argv=0x093687b8, long * rval=0x0012e590)  Line 1035 + 0xe	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x07da1668, JSObject *
obj=0x079ece08, long fval=180500104, unsigned int argc=2, long *
argv=0x093687b8, long * rval=0x0012e590)  Line 3590 + 0x1a	C
 	gklayout.dll!nsJSContext::CallEventHandler(JSObject * aTarget=0x079ece08,
JSObject * aHandler=0x0ac23688, unsigned int argc=2, long * argv=0x093687b8,
long * rval=0x0012e590)  Line 1297 + 0x18	C++
 	gklayout.dll!GlobalWindowImpl::RunTimeout(nsTimeoutImpl * aTimeout=0x00000000)
 Line 5159	C++
 	gklayout.dll!GlobalWindowImpl::TimerCallback(nsITimer * aTimer=0x0a6315f0,
void * aClosure=0x0a631568)  Line 5518	C++
 	xpcom.dll!nsTimerImpl::Fire()  Line 382 + 0x7	C++
 	xpcom.dll!nsTimerManager::FireNextIdleTimer()  Line 616	C++
 	gkwidget.dll!nsAppShell::GetNativeEvent(int & aRealEvent=1, void * &
aEvent=0x011c9610)  Line 197	C++
 	jsd3250.dll!jsdService::EnterNestedEventLoop(jsdINestCallback *
callback=0x00000000, unsigned int * _rval=0x0012e6b8)  Line 2943	C++
 	xpcom.dll!XPTC_InvokeByIndex(nsISupports * that=0x009ec0a0, unsigned int
methodIndex=49, unsigned int paramCount=2, nsXPTCVariant * params=0x0012e6a8)
 Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...},
XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2057 + 0x16	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x0ac8dd00, JSObject *
obj=0x079b7580, unsigned int argc=1, long * argv=0x07ded51c, long *
vp=0x0012e90c)  Line 1287 + 0xa	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x00ad803b, unsigned int argc=131733096,
unsigned int flags=0)  Line 941 + 0x11	C
 	js3250.dll!js_Interpret(JSContext * cx=0x07da1668, long * result=0x00000000) 
Line 2971	C
 	js3250.dll!js_Invoke(JSContext * cx=0x00ad803b, unsigned int argc=131733096,
unsigned int flags=0)  Line 958 + 0xa	C
 	xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *
wrapper=0x0012df80, unsigned short methodIndex=32827, const nsXPTMethodInfo *
info=0x07da1668, nsXPTCMiniVariant * nativeParams=0x00000000)  Line 1339 + 0x10	C++
 	xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=3, const
nsXPTMethodInfo * info=0x0807f578, nsXPTCMiniVariant * params=0x0012ed18) 
Line 450	C++
 	xpcom.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x08316498, unsigned int
methodIndex=3, unsigned int * args=0x0012edd4, unsigned int *
stackBytesToPop=0x0012edc4)  Line 117 + 0x12	C++
 	xpcom.dll!SharedStub()  Line 147	C++
 	jsd3250.dll!jsds_ExecutionHookProc(JSDContext * jsdc=0x009e9b68,
JSDThreadState * jsdthreadstate=0x09d31250, unsigned int type=0, void *
callerdata=0x00000000, long * rval=0x0012efb4)  Line 678	C++
 	jsd3250.dll!jsd_CallExecutionHook(JSDContext * jsdc=0x009e9b68, JSContext *
cx=0x0ac8dd00, unsigned int type=1, unsigned int (JSDContext *, JSDThreadState
*, unsigned int, void *, long *)* hook=0x00df7dc9, void * hookData=0x00000000,
long * rval=0x0012efb4)  Line 178	C
 	jsd3250.dll!jsd_InterruptHandler(JSContext * cx=0x0ac8dd00, JSScript *
script=0x0c530950, unsigned char * pc=0x0c530980, long * rval=0x0012efb4, void *
closure=0x0b688a28)  Line 80 + 0x12	C
 	js3250.dll!js_Interpret(JSContext * cx=0x07da1668, long * result=0x00000000) 
Line 1519 + 0x14	C
 	js3250.dll!js_Invoke(JSContext * cx=0x00ad803b, unsigned int argc=131733096,
unsigned int flags=0)  Line 958 + 0xa	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x0ac8dd2c, JSObject *
obj=0x09ac4d10, long fval=174867712, unsigned int flags=0, unsigned int argc=1,
long * argv=0x0012f210, long * rval=0x0012f230)  Line 1035 + 0xe	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x0ac8dd00, JSObject *
obj=0x09ac4d10, long fval=174867712, unsigned int argc=1, long *
argv=0x0012f210, long * rval=0x0012f230)  Line 3590 + 0x1a	C
 	gklayout.dll!nsJSContext::CallEventHandler(JSObject * aTarget=0x09ac4d10,
JSObject * aHandler=0x0a6c4500, unsigned int argc=1, long * argv=0x0012f210,
long * rval=0x0012f230)  Line 1297 + 0x18	C++
 	gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x00000000) 
Line 184 + 0x49	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct *
aListenerStruct=0x00000037, nsIDOMEvent * aDOMEvent=0x0012df80,
nsIDOMEventTarget * aCurrentTarget=0x00ad803b, unsigned int aSubType=131733096,
unsigned int aPhaseFlags=0)  Line 1434 + 0xb	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsIPresContext *
aPresContext=0x00000000, nsEvent * aEvent=0x0cc70c70, nsIDOMEvent * *
aDOMEvent=0x0012f5f0, nsIDOMEventTarget * aCurrentTarget=0x1106fe00, unsigned
int aFlags=7, nsEventStatus * aEventStatus=0x0012f5d8)  Line 1527 + 0x21	C++
 	gklayout.dll!nsGenericElement::HandleDOMEvent(nsIPresContext *
aPresContext=0x00000037, nsEvent * aEvent=0x0012df80, nsIDOMEvent * *
aDOMEvent=0x00ad803b, unsigned int aFlags=131733096, nsEventStatus *
aEventStatus=0x00000000)  Line 1960	C++
 	gklayout.dll!nsGenericHTMLElement::HandleDOMEventForAnchors(nsIPresContext *
aPresContext=0x00000037, nsEvent * aEvent=0x0012df80, nsIDOMEvent * *
aDOMEvent=0x00ad803b, unsigned int aFlags=131733096, nsEventStatus *
aEventStatus=0x00000000)  Line 1344 + 0x17	C++
 	gklayout.dll!nsEventStateManager::DispatchNewEvent(nsISupports *
aTarget=0x0cc70c70, nsIDOMEvent * aEvent=0x09a05138, int *
aPreventDefault=0x0012f678)  Line 4449	C++
 	gklayout.dll!nsEventListenerManager::DispatchEvent(nsIDOMEvent *
aEvent=0x09a05138, int * _retval=0x0012f678)  Line 1811 + 0x15	C++
 	gklayout.dll!nsDOMEventRTTearoff::DispatchEvent(nsIDOMEvent * evt=0x09a05138,
int * _retval=0x0012f678)  Line 667 + 0xf	C++
 	xpcom.dll!XPTC_InvokeByIndex(nsISupports * that=0x125fc9b0, unsigned int
methodIndex=5, unsigned int paramCount=2, nsXPTCVariant * params=0x0012f668) 
Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...},
XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2057 + 0x16	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x093352b0, JSObject *
obj=0x09ac4d10, unsigned int argc=1, long * argv=0x00985390, long *
vp=0x0012f8cc)  Line 1287 + 0xa	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x00ad803b, unsigned int argc=131733096,
unsigned int flags=0)  Line 941 + 0x11	C
 	js3250.dll!js_Interpret(JSContext * cx=0x07da1668, long * result=0x00000000) 
Line 2971	C
 	js3250.dll!js_Invoke(JSContext * cx=0x00ad803b, unsigned int argc=131733096,
unsigned int flags=0)  Line 958 + 0xa	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x093352dc, JSObject *
obj=0x0320c760, long fval=170886280, unsigned int flags=0, unsigned int argc=1,
long * argv=0x0ca63de0, long * rval=0x0012fd50)  Line 1035 + 0xe	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x093352b0, JSObject *
obj=0x0320c760, long fval=170886280, unsigned int argc=1, long *
argv=0x0ca63de0, long * rval=0x0012fd50)  Line 3590 + 0x1a	C
 	gklayout.dll!nsJSContext::CallEventHandler(JSObject * aTarget=0x0320c760,
JSObject * aHandler=0x0a2f8488, unsigned int argc=1, long * argv=0x0ca63de0,
long * rval=0x0012fd50)  Line 1297 + 0x18	C++
 	gklayout.dll!GlobalWindowImpl::RunTimeout(nsTimeoutImpl * aTimeout=0x00000000)
 Line 5159	C++
 	gklayout.dll!GlobalWindowImpl::TimerCallback(nsITimer * aTimer=0x0cf488a8,
void * aClosure=0x025fb950)  Line 5518	C++
 	xpcom.dll!nsTimerImpl::Fire()  Line 382 + 0x7	C++
 	xpcom.dll!nsTimerManager::FireNextIdleTimer()  Line 616	C++
 	gkwidget.dll!nsAppShell::Run()  Line 142	C++
 	appshell.dll!nsAppShellService::Run()  Line 524	C++
 	mozilla.exe!main1(int argc=131733096, char * * argv=0x00000000, nsISupports *
nativeApp=0x00000000)  Line 1303 + 0x9	C++
 	mozilla.exe!main(int argc=2, char * * argv=0x002a46f8)  Line 1791 + 0x13	C++
 	mozilla.exe!WinMain(HINSTANCE__ * __formal=0x00400000, HINSTANCE__ *
__formal=0x00400000, char * args=0x0015233f, HINSTANCE__ * __formal=0x00400000)
 Line 1819 + 0x17	C++
 	mozilla.exe!WinMainCRTStartup()  Line 390 + 0x1b	C
 	kernel32.dll!7c816d4f() 	
 	kernel32.dll!7c8399f3() 	

top
+	filename	0x07f8eded "chrome://venkman/content/venkman-jsdurl.js"	const char *
	lineno	508	unsigned int
/*
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/venkman/resources/content/venkman-jsdurl.js&rev=1.11&mark=508#507
 * function colorizeSourceLine (line, previousState)
 */
+	filename	0x07f8eded "chrome://venkman/content/venkman-jsdurl.js"	const char *
	lineno	749	unsigned int
/*
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/venkman/resources/content/venkman-jsdurl.js&rev=1.11&mark=749#748
 * function processSourceChunk (start)
 */
bottom

	obj->slots[-1],x	0x10101003	long
-	obj->slots,10x	0x11174905	long *
	[0]	0x00000010	long
	[1]	0x00000000	long
	[2]	0x00000000	long
	[3]	0x00000000	long
	[4]	0x00000000	long
	[5]	0x00000000	long
	[6]	0x00000000	long
	[7]	0x00000000	long
	[8]	0x00000000	long
	[9]	0x00000000	long

	js_PCToLineNumber(cx,cx->fp->script,cx->fp->pc)	565	unsigned int
+	cx->fp->pc	0x0808d24b "7="	unsigned char *

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/venkman/resources/content/venkman-jsdurl.js&rev=1.11&mark=565#548

brendan: you think a last-ditch GC just ran, maybe?
timeless: it's probably a safe bet
brendan: this is old, very old
brendan: were you using venkman?
timeless: yes :(
brendan: yeah, i think that may be necessary to hit this
timeless: i've crashed way too often in venkman, but have generally failed to
spend the time chasing them down. glad we finally made time :)
timeless: i believe the last thing i did was a step in venkman (which probably
resulted in it trying to load another source file into its view)
Attached patch fixSplinter Review
Checking in now.

/be
Attachment #170301 - Flags: review+
Could put on active branches.  Fixed on trunk.  Thanks again, timeless.

/be
Status: NEW → RESOLVED
Closed: 20 years ago
Flags: blocking1.7.6?
Resolution: --- → FIXED
get is in soon on the branches
Flags: blocking1.7.6?
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.1+
Comment on attachment 170301 [details] [diff] [review]
fix

a=caillon for aviary 1.0.1 and 1.7.6.  Please land soon.
Attachment #170301 - Flags: approval1.7.6+
Attachment #170301 - Flags: approval-aviary1.0.1+
I think this is a faithful port.  I deemed the changes in ELEMENT_OP safe to
omit, because I believe that the expansion and addition of CHECK_ELEMENT_ID was
predicated by E4X, which isn't on this branch.

I'll be checking this into both branches presently.
Branches have the fix.  My work here is done.
Comment on attachment 174047 [details] [diff] [review]
fix, ported to aviary js/src

Balsa's been failing its bloat test since this patch got checked in on the
1.0.1 branch.  This patch might have caused a regression there.
That would very much surprise me; this shouldn't affect allocation behaviour at all.

Hrm, I do see that I checked in a bit of an unrelated patch, though, as I review
the diff again.  That _still_ doesn't add any allocations, but I will remove it
for the sake of purity and not having a totally broken 1.0.1.
I misinterpreted "failing its bloat test" to mean having some bad bloat
behaviour, and didn't actually bother to look at the logs.  My apologies; I've
reverted the accidental part of that patch on both 1.0.1 and 1.7.6 branches
Current 1.0.1 and 1.7 branch builds crash reliably on linux (x86_64 at least)
when reading news (probably in other situations as well), this fixes those
crashes by porting over code that didn't make it into the first patch here.
Attachment #174427 - Flags: superreview?(brendan)
Attachment #174427 - Flags: review?(shaver)
Attachment #174427 - Flags: approval1.7.6?
Attachment #174427 - Flags: approval-aviary1.0.1?
Comment on attachment 174427 [details] [diff] [review]
Port over missing code that set 'i'

Looks like a faithful port.  Pre-approving for the branches; not an optional
fix.
Attachment #174427 - Flags: review?(shaver)
Attachment #174427 - Flags: review+
Attachment #174427 - Flags: approval1.7.6?
Attachment #174427 - Flags: approval1.7.6+
Attachment #174427 - Flags: approval-aviary1.0.1?
Attachment #174427 - Flags: approval-aviary1.0.1+
Attachment #174427 - Flags: superreview?(brendan) → superreview+
Attached patch Two more changes were needed. (obsolete) — Splinter Review
Turns out there were more problems here, two more. The initial patch fixes the
crashes in mailnews/tb, but it doesn't fix a crash when loading
maps.google.com, or an asser really. With just my first patch, I hit this:

Assertion failure: OBJ_GET_CLASS(cx, propobj) == &prop_iterator_class, at
../../../mozilla/js/src/jsinterp.c:1948

thanks to shaver I was able to figure out that propobj's class is "Object", and
thus the mismatch.

The code that was added by the do_incop lable:

-	     if (i < 0)
-		 STORE_OPND(i, OBJECT_TO_JSVAL(obj));

is somehow not doing the right thing. I don't understand what it does or what
it's supposed to do, really, but if I don't remove that code we keep crashing
when loading maps.google.com. With that code removed, I guess I could remove
the code to set 'i' above, but I left it in there for now. The other problem is
that the code on the branches changed like this:

	       case JSOP_SETELEM:
		 rval = FETCH_OPND(-1);
		 i = -2;
		 FETCH_ELEMENT_ID(i, id);
	       gs_pop_lval:
-		 lval = FETCH_OPND(i-1);
-		 VALUE_TO_OBJECT(cx, lval, obj);
+		 FETCH_OBJECT(cx, -2, lval, obj);

where i could be -1 or -2, so that -2 needs t oremain i - 1.
Attachment #174427 - Attachment is obsolete: true
Attachment #174445 - Flags: superreview?(brendan)
Attachment #174445 - Flags: review?(shaver)
Attachment #174445 - Flags: approval1.7.6?
Attachment #174445 - Flags: approval-aviary1.0.1?
The interdiff against shaver's original patch shows what was missed, only some
of which was caught.

/be
Attachment #174453 - Flags: superreview?(jst)
Attachment #174453 - Flags: review?(shaver)
Attachment #174453 - Flags: approval1.7.6?
Attachment #174453 - Flags: approval-aviary1.0.1?
Is the "Two more changes" patch still needed?

we need to get some reviews on this.
Comment on attachment 174445 [details] [diff] [review]
Two more changes were needed.

This patch is not quite right, and not needed given the latest patch (the one
with the interdiff attached right after it).

/be
Attachment #174445 - Attachment is obsolete: true
Attachment #174445 - Flags: superreview?(brendan)
Attachment #174445 - Flags: review?(shaver)
Attachment #174445 - Flags: approval1.7.6?
Attachment #174445 - Flags: approval-aviary1.0.1?
Comment on attachment 174453 [details] [diff] [review]
trunk fix back-port, plus a few other fixes that are good to go

sr=jst, fwiw. I did test this out with all the scenarios where I crashes w/o
this patch and no more crashes.
Attachment #174453 - Flags: superreview?(jst) → superreview+
Comment on attachment 174453 [details] [diff] [review]
trunk fix back-port, plus a few other fixes that are good to go

Yeah, those all look familiar and safe.  Shoulda let you do this in the first
place! r=shaver
Attachment #174453 - Flags: review?(shaver) → review+
Comment on attachment 174453 [details] [diff] [review]
trunk fix back-port, plus a few other fixes that are good to go

a=mkaply for branch
Attachment #174453 - Flags: approval1.7.6?
Attachment #174453 - Flags: approval1.7.6+
Attachment #174453 - Flags: approval-aviary1.0.1?
Attachment #174453 - Flags: approval-aviary1.0.1+
*** Bug 282522 has been marked as a duplicate of this bug. ***
I just checked attachment 174453 [details] [diff] [review] into the 1.7 and aviary 1.0.1 branches.

/be
Flags: testcase-
Crash Signature: [@ str_resolve]
You need to log in before you can comment on or make changes to this bug.