Closed Bug 663789 Opened 9 years ago Closed 9 years ago

activation objects management might not correctly set actual argument number

Categories

(Core :: JavaScript Engine, defect)

5 Branch
PowerPC
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox6 --- fixed

People

(Reporter: spectre, Assigned: luke)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files)

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!
Adding Luke to CC per his request.
Do you happen to know the revision where TenFourFox branched from mozilla trunk?  Second, does TenFourFox build with --disable-methodjit and --enable-tracejit?
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.
Whew, so I wasn't going crazy :) yes, that is exactly how it is built.
(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.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #539235 - Flags: review?(jwalden+bmo)
I forgot to mention: thank you Cameron for the reproducible test-case, it really helped.
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.
Yes, that is the correct placement.
Do I need to do it to the others or just there?
Just that one change should do it; the bug is specific to the InvokeSessionGuard.
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.
Attachment #539399 - Flags: review?(jwalden+bmo)
Attachment #539399 - Flags: approval-mozilla-beta?
Attachment #539399 - Flags: approval-mozilla-aurora?
Er, not part of the *default* build.
Needs a review before it can get approvals.
Attachment #539235 - Flags: review?(jwalden+bmo) → review+
Attachment #539399 - Flags: review?(jwalden+bmo) → review+
http://hg.mozilla.org/tracemonkey/rev/3a70ffbb58a1
Whiteboard: fixed-in-tracemonkey
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
Attachment #539399 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Is this code not part of the build that we ship with the supported platforms in Firefox?
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?
Attachment #539399 - Flags: approval-mozilla-beta-
Attachment #539399 - Flags: approval-mozilla-beta+
Attachment #539399 - Flags: approval-mozilla-aurora?
Attachment #539399 - Flags: approval-mozilla-aurora+
Requesting checkin to mozilla-beta (6). I don't need it on mozilla-aurora (7) because it should already be there (comment 16).
Keywords: checkin-needed
Correction: Luke's patch is the one needed for mozilla-beta (6).
Attachment #539399 - Flags: approval-mozilla-beta+
Attachment #539399 - Flags: approval-mozilla-aurora+
Attachment #539235 - Flags: approval-mozilla-beta?
Release drivers are a little confused on the scope of this patch.  This seems like it affects all platforms, is this true?
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 22 sounds right
Attachment #539235 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.