394551 - SourceForge page causes assert in JS_SaveFrameChain()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Matthew Cline]

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)

Comment 1

[Matthew Cline]

Attachment: gdb stack trace

Comment 2

[Igor Bukanov]

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.

Comment 3

[Igor Bukanov]

Attachment: frameless fast natives v1

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.

Comment 4

[Igor Bukanov]

Attachment: frameless fast natives v2

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 5

[Igor Bukanov]

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 6

[Brendan Eich [:brendan]]

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

Comment 7

[Igor Bukanov]

(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.

Comment 8

[Brendan Eich [:brendan]]

(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

Comment 9

[Igor Bukanov]

Attachment: frameless fast natives v3

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");

Comment 10

[Igor Bukanov]

Attachment: v2-v3 diff

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

Comment 11

[Brendan Eich [:brendan]]

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

Comment 12

[Igor Bukanov]

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

Comment 13

[Igor Bukanov]

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

Comment 14

[Igor Bukanov]

Attachment: v4

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 15

[Brendan Eich [:brendan]]

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

Comment 16

[Igor Bukanov]

(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? 

Comment 17

[Brendan Eich [:brendan]]

There's also the buildbot try server:

http://wiki.mozilla.org/Build:TryServer

This is probably the right answer.

/be

Comment 18

[Igor Bukanov]

(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.

Comment 19

[Igor Bukanov]

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

Comment 20

[Joachim Kuebart]

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.

Comment 21

[Joachim Kuebart]

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.

Comment 22

[Igor Bukanov]

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

Comment 23

[Igor Bukanov]

(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.