Steps to reproduce: 1) With a debug build on a Linux machine, go to any SourceForge page with ads, without ad-blocking and with JavaScript turned on, and with Flash installed. 2) Things will go while loading the page, until FireFox becomes unresponive while chewing up 99% of the CPU. 3) Wait for a while, and you'll eventually get this assertion: Assertion failure: !(fp->flags & JSFRAME_IN_FAST_CALL), at /usr/home/matt/develop/mozilla/js/src/jsapi.c:5000 Program received signal SIGTRAP, Trace/breakpoint trap. JS_Assert (s=0xb7edc2e4 "!(fp->flags & JSFRAME_IN_FAST_CALL)", file=0xb7edc2b4 "/usr/home/matt/develop/mozilla/js/src/jsapi.c", ln=5000) at /usr/home/matt/develop/mozilla/js/src/jsutil.c:63 63 abort(); There was also a bunch of "WARNING: Tabs encountered in a situation where we don't support tabbing: file /usr/home/matt/develop/mozilla/layout/generic/nsTextFrameThebes.cpp, line 2475" before the assertion. My setup: * Source pulled from CVS around 5:50 PM Pacific Time, Aug 31. * Linux kernel 2.6.17-13 * Mandriva Linux release 2007.1 (Official) for i586 * libgcc1-4.1.2-1mdv2007.1 * libstdc++6-4.1.2-1mdv2007.1 * libglib2.0_0-2.12.11-1mdv2007.1 * glibc-2.4-8mdv2007.1 * gcc version 4.1.2 20070302 (prerelease) (4.1.2-1mdv2007.1)

To fix this I suggest to never create a frame for fast calls. This would remove the reason for the assert and simplify the calling of fast natives.

With the patch fast natives are always frameless even when called via js_Invoke. Effectively they are treated as extensions to the interpreter. That allowed to remove JSFRAME_IN_FAST_CALL flag as frame walking code no longer sees the fast native frames. As a consequence of the removal the patch uses PUSH_OPND(void), not PUSH(void) when extra native slots when calling the fast native so the decompiler can properly locate that void instead of skipping the frame. A substantiation portion of the patch is js_Invoke cleanup. Now the function never alters cx->fp->sp. Instead it relies on tvr/JSStackHeader lists to properly root fp->argv. It allowed to remove all frame lifting code from the callers of js_Invoke. This version of the patch passes the test suite for the shell but I have not done any testing with a browser build.

The new version allows to call js_Invoke with cx->fp == null and fixes the usage of the function in xpconnect code. Now it passes the test suite and simple browser tests.

Comment on attachment 279283 [details] [diff] [review] frameless fast natives v2 The patch touches array_extra from jsarray.c, a known source of regressions.

Comment on attachment 279283 [details] [diff] [review] frameless fast natives v2 >+ * The pseudo-frame is not created for fast natives as they >+ * are treated as interpreter's extensions and always trusted. s/interpreter's extensions/interpreted frame extensions/ >+ if (fp->fun->flags & JSFRAME_ROOTED_ARGS) >+ skip = 2 + fp->argc; > } >- TRACE_JSVALS(trc, nslots, vp, "arg"); >+ TRACE_JSVALS(trc, 2 + nslots - skip, fp->argv - 2 + skip, "operand"); Assert before the last line that 2 + nslots >= skip? >+ * Hit end of arena: we have to copy argv[-2..(argc+nslots-1)]. Here [m..n] is used for the fully-closed interval [m,n] in mathematical notation. >+ /* Root the extra slots that are not covered by vp..vp+2+argc. */ But here .. is used without enclosing [] to mean a half-open interval from vp: [0, 2+argc). Best to use the same notation everywhere. Probably best to use [m,n) because it avoids a -1. >+ i = rootedArgsFlag ? 2 + argc : 0; >+ JS_PUSH_TEMP_ROOT(cx, 2 + argc + nslots - i, argv - 2 + i, &tvr); >+ ok = ((JSFastNative) native)(cx, argc, argv - 2); >+ JS_POP_TEMP_ROOT(cx, &tvr); Any point in avoiding the tvr pushing and popping where possible? More codesize, better cycle times for common cases (predicted taken branch around tvr push and pop)? >+ /* >+ * Use PUSH_OPND so sp[..-depth] contains the Rogue .. above? Or did you mean to suggest an interval? >+#define JSFRAME_ROOTED_ARGS 0x2000 /* frame.argv is rooted by the caller */ Suggest JSFRAME_ROOTED_ARGV as the name. >+js_Invoke(JSContext *cx, uintN argc, jsval *vp, uintN flags); I forgot to comment on the guts of js_Invoke, the if (!ok) *vp = JSVAL_NULL; at the bottom. That should not be necessary. Are you thinking about garbage kept alive after an exception or error? Great patch again -- does it pass all tests including Minefield run on the bloat and other automated tests? I can a+ a final version. /be

(In reply to comment #6) > >+ i = rootedArgsFlag ? 2 + argc : 0; > >+ JS_PUSH_TEMP_ROOT(cx, 2 + argc + nslots - i, argv - 2 + i, &tvr); > >+ ok = ((JSFastNative) native)(cx, argc, argv - 2); > >+ JS_POP_TEMP_ROOT(cx, &tvr); > > Any point in avoiding the tvr pushing and popping where possible? More > codesize, better cycle times for common cases (predicted taken branch around > tvr push and pop)? When js_Invoke is called for a fast native, it would be precisely because nslots != 0 and the extra rooting is required. So the check to avoid tvr pushing would increase the number of cycles to burn.

(In reply to comment #7) > (In reply to comment #6) > When js_Invoke is called for a fast native, it would be precisely because > nslots != 0 and the extra rooting is required. Assert that? /be

The new patch adds/fixes comments and asserts that vp passed to js_Invoke come from the last JS stack arena. The patch does not assert in js_Invoke that, when the interpreter calls js_Invoke for a fast native, the number of extra slots to root via JSTempValueRooter is non-zero. That assert would not be correct since the interpreter calls js_Invoke directly for JSOP_SETCALL without any fast native optimizations/checks. I also have not added an assert that in the last line of the following fragment of js_TraceStackFrame 2 + nslots >= slots. That immediately follow from the code above the last line so I think an extra assert here just adds a noise: nslots = fp->argc; skip = 0; if (fp->fun) { minargs = FUN_MINARGS(fp->fun); if (minargs > nslots) nslots = minargs; if (!FUN_INTERPRETED(fp->fun)) { JS_ASSERT(!(fp->fun->flags & JSFUN_FAST_NATIVE)); nslots += fp->fun->u.n.extra; } if (fp->fun->flags & JSFRAME_ROOTED_ARGV) skip = 2 + fp->argc; } TRACE_JSVALS(trc, 2 + nslots - skip, fp->argv - 2 + skip, "operand");

This is a diff between v2 merged with the trunk changes and v3 to show the essence of the changes.

Comment on attachment 281169 [details] [diff] [review] frameless fast natives v3 Right, assertion is vacuous in full context (assuming no future changes rock the boat). /be

I checked in the patch from comment 9 to the trunk: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1190056080&maxdate=1190056290&who=igor%mir2.org

The last patch broke the build on Windows: xpconnect/src/XPCDispTearOff.cpp uses js_Invoke but is not compiled on Linux.

This is v3 + XPCDispatchTearOff changes bellow. I need to figure out how to enable that code on Linux since this is the second time I was bitten by miscompilation there. The changes are essentially the same as was done for xpconnect/src/xpcwrappedjsclass.cpp. +++ js/src/xpconnect/src/XPCDispTearOff.cpp 2007-09-17 21:40:01.000000000 +0200 @@ -311,17 +311,17 @@ STDMETHODIMP XPCDispatchTearOff::Invoke( nsCString msg("Unable to set property "); msg += name; return Error(E_FAIL, msg.get()); } } } else // We're invoking a function { - jsval* stackbase; + jsval* stackbase = nsnull; jsval* sp = nsnull; uint8 i; uint8 argc = pDispParams->cArgs; uint8 stack_size; jsval result; uint8 paramCount=0; nsresult retval = NS_ERROR_FAILURE; nsresult pending_result = NS_OK; @@ -440,36 +440,18 @@ pre_call_clean_up: goto done; // do the deed - note exceptions JS_ClearPendingException(cx); if(!JSVAL_IS_PRIMITIVE(fval)) { - // Lift current frame (or make new one) to include the args - // and do the call. - JSStackFrame *fp, *oldfp, frame; - jsval *oldsp; - - fp = oldfp = cx->fp; - if(!fp) - { - memset(&frame, 0, sizeof(frame)); - cx->fp = fp = &frame; - } - oldsp = fp->sp; - fp->sp = sp; - - success = js_Invoke(cx, argc, JSINVOKE_INTERNAL); - - result = fp->sp[-1]; - fp->sp = oldsp; - if(oldfp != fp) - cx->fp = oldfp; + success = js_Invoke(cx, argc, stackbase, JSINVOKE_INTERNAL); + result = stackbase[0]; } else { // The property was not an object so can't be a function. // Let's build and 'throw' an exception. static const nsresult code = NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED;

Comment on attachment 281221 [details] [diff] [review] v4 If you want a Windows box (or VMWare Fusion so you can run dual-boot on a Linux or Mac machine) please let me know. /be

(In reply to comment #15) > (From update of attachment 281221 [details] [diff] [review]) > If you want a Windows box (or VMWare Fusion so you can run dual-boot on a Linux > or Mac machine) please let me know. A Windows box would only help in this particular case if the patch would be developed on Windows. Honestly this is something I would prefer to avoid. Windows simply is not a good platform to write the code on... On the other hand it is useful to have an option to test patches that clearly contains platform-specific code like in bug 392263 or to check MSVC warnings. But for that it should be sufficient to run a Windows setup via VMWare without dual-booting. So are there any useful VMWare images configured for FF development available?

There's also the buildbot try server: http://wiki.mozilla.org/Build:TryServer This is probably the right answer. /be

(In reply to comment #17) > There's also the buildbot try server: > > http://wiki.mozilla.org/Build:TryServer Thanks for the tip, I will surely use it the next time.

I checked in the patch from comment 14 to the trunk: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1190100840&maxdate=1190100861&who=igor%mir2.org

I think there is a problem with the patch from comment 14: In js_Invoke() where a JSFUN_FAST_NATIVE is called, there is no check whether the argument vector got moved by the preceding block due to required extra slots. The native leaves the return value at argv[-2], which is different from *vp if (and only if) the argument vector was reallocated. In this case, the return value should be moved to the correct position, e.g. ok = ((JSFastNative) native)(cx, argc, argv - 2); + if (vp != argv - 2) + *vp = argv[-2]; This results in losing the return value of fast natives in the (rare and difficult to reproduce) case that the argument vector is moved.

Also, later in js_Invoke() where other natives are called, the code refers to cx->fp for the caller's varobj and scopeChain: if (cx->fp) { frame.varobj = cx->fp->varobj; frame.scopeChain = cx->fp->scopeChain; } else { frame.varobj = NULL; frame.scopeChain = NULL; } However, cx->fp has already been set to &frame, whose varobj and scopeChain members are NULL, and hence natives can never see their callers' varobj/scopeChain. At this point, the caller's frame pointer is only available in frame.down, which should be used instead of cx->fp in the code quoted above.

Thanks for catching the regressions, I filed bug 396684 for them.

(In reply to comment #21) > However, cx->fp has already been set to &frame, whose varobj and scopeChain > members are NULL, and hence natives can never see their callers' > varobj/scopeChain. > > At this point, the caller's frame pointer is only available in frame.down, > which should be used instead of cx->fp in the code quoted above. Given that the changes passed the test and smoke test, it seems that the value of varobj for a native call is irrelevant and scope chain can always be set to parent. But that is for another bug.