activation objects management might not correctly set actual argument number

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: spectre, Assigned: luke)

Tracking

5 Branch
PowerPC
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox6 fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
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
 {
     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!
(Reporter)

Comment 1

6 years ago
Adding Luke to CC per his request.
(Assignee)

Comment 2

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

Comment 3

6 years ago
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.
(Reporter)

Comment 4

6 years ago
Whew, so I wasn't going crazy :) yes, that is exactly how it is built.
(Assignee)

Comment 5

6 years ago
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.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #539235 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 6

6 years ago
I forgot to mention: thank you Cameron for the reproducible test-case, it really helped.
(Reporter)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
Yes, that is the correct placement.
(Reporter)

Comment 9

6 years ago
Do I need to do it to the others or just there?
(Assignee)

Comment 10

6 years ago
Just that one change should do it; the bug is specific to the InvokeSessionGuard.
(Reporter)

Comment 11

6 years ago
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.
Attachment #539399 - Flags: review?(jwalden+bmo)
Attachment #539399 - Flags: approval-mozilla-beta?
Attachment #539399 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 12

6 years ago
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+
(Assignee)

Comment 14

6 years ago
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-
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/3a70ffbb58a1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Is this code not part of the build that we ship with the supported platforms in Firefox?
(Reporter)

Comment 18

6 years ago
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?

Updated

6 years ago
Attachment #539399 - Flags: approval-mozilla-beta-
Attachment #539399 - Flags: approval-mozilla-beta+
Attachment #539399 - Flags: approval-mozilla-aurora?
Attachment #539399 - Flags: approval-mozilla-aurora+
(Reporter)

Comment 19

6 years ago
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
(Reporter)

Comment 20

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

Updated

6 years ago
Attachment #539399 - Flags: approval-mozilla-beta+
Attachment #539399 - Flags: approval-mozilla-aurora+
(Reporter)

Updated

6 years ago
Attachment #539235 - Flags: approval-mozilla-beta?

Comment 21

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

Comment 22

6 years ago
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.
(Assignee)

Comment 23

6 years ago
comment 22 sounds right

Updated

6 years ago
Attachment #539235 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 24

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

Updated

6 years ago
status-firefox6: --- → fixed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.