663789 - activation objects management might not correctly set actual argument number

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Cameron Kaiser [:spectre]]

Attachment: Incompletely reduced TenFourFox test case (does not fail on Firefox 5 yet)

Per Luke's request in bug 636296 comment 29. To recapitulate, I am tracking a TenFourFox bug where the number of arguments in the argsobj is sometimes a ridiculously high number, causing malloc() in NewArguments() to fail. The large number, after analysis, seemed to have come from args.nactual in jsinterpinlines.h. This problem did not occur in Mozilla 2.0, leading me to suspect bug 636296 part 4 because that part reduced the number of situations where that value was updated. Our current wallpaper is this,

diff --git a/js/src/jsinterpinlines.h b/js/src/jsinterpinlines.h
--- a/js/src/jsinterpinlines.h
+++ b/js/src/jsinterpinlines.h
@@ -393,17 +393,23 @@ JSStackFrame::varobj(JSContext *cx) cons
     return isFunctionFrame() ? callObj() : cx->activeSegment()->getInitialVarObj();
 }
 
 inline uintN
 JSStackFrame::numActualArgs() const
 {
     JS_ASSERT(hasArgs());
     if (JS_UNLIKELY(flags_ & (JSFRAME_OVERFLOW_ARGS | JSFRAME_UNDERFLOW_ARGS)))
+#if(0)
         return hasArgsObj() ? argsObj().getArgsInitialLength() : args.nactual;
+#else
+        return hasArgsObj() ? argsObj().getArgsInitialLength() :
+            (args.nactual < 1024*1024) ? args.nactual :
+            numFormalArgs();
+#endif
     return numFormalArgs();
 }
 
 inline js::Value *
 JSStackFrame::actualArgs() const
 {
     JS_ASSERT(hasArgs());
     js::Value *argv = formalArgs();

which does work, but seems wrong. Is there, blindly, a better way?

I have attached the test case that we are using but it does not actually fail on Firefox 5 (it is from TED.com). Unfortunately this test case is not completely minimized (the HTML is, but I can only get it to trigger with a specific minified Dojo that I am trying to reduce further). However, we don't have mjit in TenFourFox, only tracejit, so it's possible that this is a situation that tracejit doesn't do right. I'm trying to see if I can make it fail with the supported browser.

If this is not a Firefox bug, and it might not be, is our putative solution correct? Is there a better way?

Thanks!

Comment 1

[Cameron Kaiser [:spectre]]

Adding Luke to CC per his request.

Comment 2

[Luke Wagner [:luke]]

Do you happen to know the revision where TenFourFox branched from mozilla trunk?  Second, does TenFourFox build with --disable-methodjit and --enable-tracejit?

Comment 3

[Luke Wagner [:luke]]

Ah, nm the question in comment 2, I found the problem and it is indeed something that wouldn't show up unless !defined(JS_METHODJIT).  Fix shortly.

Comment 4

[Cameron Kaiser [:spectre]]

Whew, so I wasn't going crazy :) yes, that is exactly how it is built.

Comment 5

[Luke Wagner [:luke]]

Attachment: one-line fix and two tests

(oops, I got pulled away right after saying "fix shortly" :)

The reason !defined(JS_METHODJIT) doesn't have the problem is that EnterMethodJIT calls markActivationObjetsAsPut on exit.

Comment 6

[Luke Wagner [:luke]]

I forgot to mention: thank you Cameron for the reproducible test-case, it really helped.

Comment 7

[Cameron Kaiser [:spectre]]

No worries and thanks very much for the patch. I'm sorry it wasn't completely reduced, but reducing minified JS really sucks.

I'm looking at the patch to see how to apply it to 5 for us in the meantime (note I'm not asking for beta+ since this probably only applies to us). We don't have js/src/vm/ in 5.0, obviously, so is this the right place (in js/src/jsinterpinlines.h):

inline bool
InvokeSessionGuard::invoke(JSContext *cx) const
{
    /* N.B. Must be kept in sync with Invoke */

    /* Refer to canonical (callee, this) for optimized() sessions. */
    formals_[-2] = savedCallee_;
    formals_[-1] = savedThis_;

#ifdef JS_METHODJIT
    void *code;
    if (!optimized() || !(code = script_->getJIT(false /* !constructing */)->invokeEntry))
#else
    if (!optimized())
#endif
        return Invoke(cx, args_, 0);

    /* Clear any garbage left from the last Invoke. */
    JSStackFrame *fp = frame_.fp();
    fp->clearMissingArgs();
    PutActivationObjects(cx, frame_.fp());
>>> markActivationObjectsAsPut(); <<<
    fp->resetInvokeCallFrame();
    SetValueRangeToUndefined(fp->slots(), script_->nfixed);

Does this need to be put in multiple places, too? I see four other places where it is called:

~/src/mozilla-beta/js/src/% grep PutActivationObjects *
jscntxt.cpp:    PutActivationObjects(cx, cx->fp());
jscntxtinlines.h:    PutActivationObjects(cx, fp);
jscntxtinlines.h:    PutActivationObjects(cx, fp);
jsinterp.h:PutActivationObjects(JSContext *cx, JSStackFrame *fp)
jsinterpinlines.h:     * Since the stack frame is usually popped after PutActivationObjects,
jsinterpinlines.h:    PutActivationObjects(cx, frame_.fp());

If there is desire to make this a formal patch for 5 beta, I'll write one up, but again I suspect it only applies to us in TenFourFox-land. If this seems right, I'll run off a test build and make sure it passes the new tests.

Comment 8

[Luke Wagner [:luke]]

Yes, that is the correct placement.

Comment 9

[Cameron Kaiser [:spectre]]

Do I need to do it to the others or just there?

Comment 10

[Luke Wagner [:luke]]

Just that one change should do it; the bug is specific to the InvokeSessionGuard.

Comment 11

[Cameron Kaiser [:spectre]]

Attachment: "Branch" fix for 5.0/6.0

That works and passes tests. Thank you, Luke! I went ahead and ran off a patch anyway since I'll need to do this on 6.0. It's not critical for 5.0 since it's not part of the build, but it might be nice if others are in my situation, so I'll ask for beta and aurora approval.

Comment 12

[Cameron Kaiser [:spectre]]

Er, not part of the *default* build.

Comment 13

[Daniel Veditz [:dveditz]]

Needs a review before it can get approvals.

Comment 14

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/3a70ffbb58a1

Comment 15

[Johnathan Nightingale [:johnath]]

Comment on attachment 539399 [details] [diff] [review]
"Branch" fix for 5.0/6.0

Can't re-spin for this in time for tomorrow's release - if there's something we should do on a rel-branch here, I recommend coordinating with legnitto

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/3a70ffbb58a1

Comment 17

[Christopher Blizzard (:blizzard)]

Is this code not part of the build that we ship with the supported platforms in Firefox?

Comment 18

[Cameron Kaiser [:spectre]]

It's not in 5 or 6, no. (I landed it in TenFourFox 5 as a foreign changeset.) Still hoping for aurora before the next train?

Comment 19

[Cameron Kaiser [:spectre]]

Requesting checkin to mozilla-beta (6). I don't need it on mozilla-aurora (7) because it should already be there (comment 16).

Comment 20

[Cameron Kaiser [:spectre]]

Correction: Luke's patch is the one needed for mozilla-beta (6).

Comment 21

[JP Rosevear [:jpr]]

Release drivers are a little confused on the scope of this patch.  This seems like it affects all platforms, is this true?

Comment 22

[Cameron Kaiser [:spectre]]

Luke is probably the best one to answer this, but the bug I reported was only visible on those systems where tracejit is enabled but methodjit is not (comment 3). No official build of Firefox ships this way; I only noticed it on TenFourFox because we ship our own PPC tracejit, but don't (yet) have a PPC methodjit. I don't know of any other platforms in that boat and certainly not x86, ARM or SPARC.

Comment 23

[Luke Wagner [:luke]]

comment 22 sounds right

Comment 24

[Luke Wagner [:luke]]

http://hg.mozilla.org/releases/mozilla-beta/rev/ba55146f4519