Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 663789 - activation objects management might not correctly set actual argument number
: activation objects management might not correctly set actual argument number
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 5 Branch
: PowerPC Mac OS X
: -- normal (vote)
: ---
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2011-06-13 05:41 PDT by Cameron Kaiser [:spectre]
Modified: 2011-07-13 14:46 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Incompletely reduced TenFourFox test case (does not fail on Firefox 5 yet) (1.05 KB, text/html)
2011-06-13 05:41 PDT, Cameron Kaiser [:spectre]
no flags Details
one-line fix and two tests (1.76 KB, patch)
2011-06-14 09:47 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
asa: approval‑mozilla‑beta+
Details | Diff | Splinter Review
"Branch" fix for 5.0/6.0 (1.89 KB, patch)
2011-06-14 19:07 PDT, Cameron Kaiser [:spectre]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Cameron Kaiser [:spectre] 2011-06-13 05:41:40 PDT
Created attachment 538871 [details]
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
         return hasArgsObj() ? argsObj().getArgsInitialLength() : args.nactual;
+        return hasArgsObj() ? argsObj().getArgsInitialLength() :
+            (args.nactual < 1024*1024) ? args.nactual :
+            numFormalArgs();
     return numFormalArgs();
 inline js::Value *
 JSStackFrame::actualArgs() const
     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 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?

Comment 1 Cameron Kaiser [:spectre] 2011-06-13 05:42:10 PDT
Adding Luke to CC per his request.
Comment 2 Luke Wagner [:luke] 2011-06-13 11:47:58 PDT
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] 2011-06-13 13:21:57 PDT
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] 2011-06-13 13:23:09 PDT
Whew, so I wasn't going crazy :) yes, that is exactly how it is built.
Comment 5 Luke Wagner [:luke] 2011-06-14 09:47:32 PDT
Created attachment 539235 [details] [diff] [review]
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] 2011-06-14 10:15:32 PDT
I forgot to mention: thank you Cameron for the reproducible test-case, it really helped.
Comment 7 Cameron Kaiser [:spectre] 2011-06-14 13:10:23 PDT
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_;

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

    /* Clear any garbage left from the last Invoke. */
    JSStackFrame *fp = frame_.fp();
    PutActivationObjects(cx, frame_.fp());
>>> markActivationObjectsAsPut(); <<<
    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] 2011-06-14 17:33:22 PDT
Yes, that is the correct placement.
Comment 9 Cameron Kaiser [:spectre] 2011-06-14 17:34:16 PDT
Do I need to do it to the others or just there?
Comment 10 Luke Wagner [:luke] 2011-06-14 18:02:01 PDT
Just that one change should do it; the bug is specific to the InvokeSessionGuard.
Comment 11 Cameron Kaiser [:spectre] 2011-06-14 19:07:47 PDT
Created attachment 539399 [details] [diff] [review]
"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] 2011-06-14 19:10:10 PDT
Er, not part of the *default* build.
Comment 13 Daniel Veditz [:dveditz] 2011-06-16 15:10:48 PDT
Needs a review before it can get approvals.
Comment 14 Luke Wagner [:luke] 2011-06-17 17:01:14 PDT
Comment 15 Johnathan Nightingale [:johnath] 2011-06-20 14:30:31 PDT
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) 2011-06-20 17:13:46 PDT
cdleary-bot mozilla-central merge info:
Comment 17 Christopher Blizzard (:blizzard) 2011-06-29 12:01:11 PDT
Is this code not part of the build that we ship with the supported platforms in Firefox?
Comment 18 Cameron Kaiser [:spectre] 2011-06-29 13:59:43 PDT
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] 2011-07-05 14:53:33 PDT
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] 2011-07-05 22:10:31 PDT
Correction: Luke's patch is the one needed for mozilla-beta (6).
Comment 21 JP Rosevear [:jpr] 2011-07-11 14:28:18 PDT
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] 2011-07-11 20:56:25 PDT
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] 2011-07-12 09:47:53 PDT
comment 22 sounds right

Note You need to log in before you can comment on or make changes to this bug.