Closed Bug 636296 Opened 9 years ago Closed 9 years ago

simplify activation object creating/putting logic

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(5 files, 7 obsolete files)

5.65 KB, patch
dvander
: review+
Details | Diff | Splinter Review
13.02 KB, patch
dvander
: review+
Details | Diff | Splinter Review
31.40 KB, patch
dvander
: review+
Details | Diff | Splinter Review
53.14 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
5.13 KB, patch
dvander
: review+
Details | Diff | Splinter Review
Working on bug 635811 and bug 634542, I saw a couple ways we could simplify the logic surrounding activation object creating/putting.  The patches got too big for FF4, so I had to make quick 'n dirty fixes instead, but I'd still like to finish and land the patches post 4.0.  In particular:
 - putting call objects when we pop the frame, not in the ScriptEpilogue (we create the call object when we push the frame, not in ScriptPrologue).
 - s/fp->hasCallObj/fp->hasOwnCallObj/ so that "if (fp->hasOwnCallObj()) PutCallObject(fp->callObj())" is always correct (as opposed to now with the whole non-strict eval situation)
Attached patch part 1: tidy up JSStackFrame (obsolete) — Splinter Review
Passes trace tests.  I won't bother anyone for reviews until after FF4.0, but I thought it would be good to leave these here.
Posting rebased patches and getting review.  The first two are really simple.  This patch just fixed a naming complaint Steve Fink had on infomonkey.
Attachment #514870 - Attachment is obsolete: true
Attachment #519293 - Flags: review?(dvander)
This splits prologue/epilogue into the parts that deal with debug stuff and the parts that deal with core semantics.
Attachment #514872 - Attachment is obsolete: true
Attachment #519294 - Flags: review?(dvander)
This patch gives 'hasCallObj' the meaning: if a frame 'hasCallObj', then the call obj should be put when the frame is popped (thereby eliminating the eval-frame corner case).
Attachment #514873 - Attachment is obsolete: true
Attachment #519295 - Flags: review?(jwalden+bmo)
Attachment #519294 - Attachment description: tidy up ScriptPrologue/Epilogue → part 2: tidy up ScriptPrologue/Epilogue
Attachment #514874 - Attachment is obsolete: true
Attachment #519296 - Flags: review?(dvander)
Oops, need to fix InjectJaegerReturn; will make a part 5 patch.
Actually, InjectJaegerReturn can just die; its equivalent to the forceReturnTrampoline, which works.
Attachment #519296 - Attachment is obsolete: true
Attachment #519296 - Flags: review?(dvander)
Attachment #519447 - Flags: review?(dvander)
Blocks: 641436
Fix bug (strict eval's call obj not getting put in initialVarObj).

With that, green on try.
Attachment #519295 - Attachment is obsolete: true
Attachment #519295 - Flags: review?(jwalden+bmo)
Attachment #519527 - Flags: review?(jwalden+bmo)
Attachment #519293 - Flags: review?(dvander) → review+
Attachment #519294 - Flags: review?(dvander) → review+
Comment on attachment 519447 [details] [diff] [review]
part 4: put activation objects when the frame is popped, not ScriptEpilogue

Nice, this is much simpler. One nit, markActivationObjectsAsHavingBeenPut is kind of verbose. How about markActivationObjectsAsPut?
Attachment #519447 - Flags: review?(dvander) → review+
Oh, hah, yeah, that is much preferable.
Comment on attachment 519527 [details] [diff] [review]
part 3: simplify meaning of hasCallObj

You have one "NB." which more typically is "NB:".  Other than that, much awesomeness to behold.
Attachment #519527 - Flags: review?(jwalden+bmo) → review+
Part 4 causes crash on Win64 and Solaris x86.  These platforms have to consider stack adjustment.
Attached patch crash fix for Win64 (obsolete) — Splinter Review
Attachment #521451 - Flags: review?(luke)
Comment on attachment 521451 [details] [diff] [review]
crash fix for Win64

Could you instead factor out this hack into JaegerCompartment so that it gets fixed in one place instead of 2+ ?
Attachment #521451 - Flags: review?(luke)
Oops, I meant to add: thank you for debugging and fixing :)
(In reply to comment #19)
> Comment on attachment 521451 [details] [diff] [review]
> crash fix for Win64
> 
> Could you instead factor out this hack into JaegerCompartment so that it gets
> fixed in one place instead of 2+ ?

PushActiveVMFrame set return function to JaegerTrampolineReturn. JaegerTrampolineReturn adjusts stack for non-FASTCALL ABI and Win64 ABIT.   This point needs restore stack.
I was referring to the duplication in stubs::Debugger and stubs::Trap.
Depends on: 645505
No longer depends on: 645505
(In reply to comment #22)
> I was referring to the duplication in stubs::Debugger and stubs::Trap.

It is necessary to add stack adjustment excepting to js_InternalThrow().

After my fix, it passes test even if on Win64 build.
I'm sorry, I was not communicating my intentions clearly.  Here is something like what I was asking for.
Attachment #521451 - Attachment is obsolete: true
Attachment #522395 - Flags: review?
Attachment #522395 - Flags: review? → review?(dvander)
Attachment #522395 - Flags: review?(dvander) → review+
Duplicate of this bug: 648777
I'm tracking a TenFourFox bug that seems to have been triggered by part 4. In our bug, the browser hiccoughs (and sometimes crashes) trying to malloc a ridiculously large amount of memory. Analysis shows that the actual failure is in NewArguments() being passed a huge argc, which multiplied by sizeof(Value) yields a huge number, and the argc is coming from args.nactual, which appears to be getting much less frequently updated as a result of that part. Our wallpaper patch is this very simple kludge,

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 repairs the problem. But I'm trying to figure out where it came from in the first place. Is this the right thing to do? The dynamics may be different for us because we're pure tracejit and have no methodjit yet.

4.0 didn't do this.
Cameron: is there a reproducible test-case for this hiccough/crash?  (Also, if its not too much trouble, could you file a new bug instead of extending this one and CC me?)
Done (bug 663789). Thanks, Luke!
You need to log in before you can comment on or make changes to this bug.