Closed Bug 624100 Opened 11 years ago Closed 11 years ago

"Assertion failure: fp->hasCallObj()" with strict mode in eval


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: jruderman, Assigned: dvander)



(Keywords: assertion, regression, testcase, Whiteboard: [fixed-in-tracemonkey][hardblocker])


(1 file, 2 obsolete files)

./js -j -m
eval("'use strict'; for(let j=0;j<9;++j) {} x;");

Assertion failure: fp->hasCallObj(), at jsinterpinlines.h:735

I think this is a regression from bug 514568 (rev 2e94f0b8d03c).
(Or its relanding, rev 1417c3808f04.)
Assignee: general → jwalden+bmo
I think I have this figured out -- running tests now to see whether the fix breaks anything.
blocking2.0: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla2.0b9
Somewhat.  But not completely.  And it's turning into a rat's-nest of potentially multiple problems, and it's past time to go to sleep, so look for a fix here later.
Attached patch Patch (obsolete) — Splinter Review
Actually, the complexity looks completely unrelated (only my luck to stumble on it), so I think this is good.  As far as I can tell the comment was mistaken, and thus far the only problem (ignoring assertions) with calling ScriptEpilogue twice was the hook (if present) getting called twice.  But I'm not 100% confident in this reading, so I'd prefer to not delay in fixing this -- if bug 514568 goes in b9, I think this should as well.

bc, how do I write tests that are expected to fail these days?  Putting the testcase inside a function, or inside a try-catch, or other abstractable places seems to break it for me.  (I'll add those tests, once I figure out some other issues, but the original one seems particularly important to exercise.)
Attachment #502205 - Flags: review?(dvander)
Attachment #502205 - Flags: feedback?(bclary)
Comment on attachment 502205 [details] [diff] [review]

Waldo, if I understand the situation correctly the above test passes in the shell when ReferenceError: x is not defined is thrown and the assertion doesn't fire but wrapping the test inside of other code prevents the assertion altogether even in an unfixed tree.

You could use expectExitCode() in the shell to cause one or more non-zero exit codes to be considered passing. Things have changed regarding assertions, especially on Windows, but it used to be the case that on Windows a shell script terminating due to an uncaught exception had the same exit code as a shell script terminating due to an assertion. I'm not sure of the current situation. You are the better judge of that I think. You don't have the potential confusion between uncaught exceptions and assertions in the browser though. :-)

The trick in the browser would be to do allow the uncaught exception to terminate the test script while still doing document.documentElement.className = '' (in the jsTestDriverEnd() call) after the test completes in order to inform reftest that the test has completed. You could possibly use a load handler. It may be the case that you can't write a browser based test that exhibits the assertion in the unfixed tree. I would consider excluding the test for the browser a reasonable choice if it is too difficult to get it to run meaningfully there.

Does that help?
Attachment #502205 - Flags: feedback?(bclary) → feedback+
It helps.  It's kind of sad that there seems to be no way to write a shell+browser test for this, only a shell test, but oh well.
Comment on attachment 502205 [details] [diff] [review]

This path is really hairy so I'm going to reason the current logic out loud:

 (1) On successful return, the JIT has called ScriptEpilogue().
 (2) On failure, the JIT has called js_InternalThrow(). If no handler is found, ScriptEpilogue() is called.
 (3) From (1) and (2), ScriptEpilogue() has always been called exiting the JIT.
 (4) The interpreter may invoke the method JIT. In this case, because of (3), it skips the top of the 
     "inline_return" label that would call ScriptEpilogue() again. (But it still pops the inline and continues
     as normal).

Your patch changes (2). That'll skip ScriptEpilogue() if the JIT throws an error after being called from within the interpreter. Test case: (it won't visibly fail, but at popInlineFrame the callobj still points to a frame):

 (function() { with(this) { (function() { eval(""); return x; } )(); } })();

In the original test case, ScriptEpilogue() gets called once from the interpreter (during recording, or something), which returns false. We THROW, which goes to js_InternalThrow(), which runs the epilogue again. This is a really strange edge case.

I think the fix is to skip parts of js_InternalThrow if fp->finishedInInterpreter() is set. But there are more bugs here, still looking.
Attachment #502205 - Flags: review?(dvander) → review-
blocking2.0: ? → betaN+
stealing with permission
Assignee: jwalden+bmo → dvander
(In reply to comment #6)
> It helps.  It's kind of sad that there seems to be no way to write a
> shell+browser test for this, only a shell test, but oh well.

You might be able to do it in the browser. Just set the onload handler to tell reftest the test is complete before the important part of your test executes and make everything straightline code not included in any functions. There is no guarantee but there is a good change it will reproduce in an unpatched browser.
Attached patch fix (obsolete) — Splinter Review
Attachment #502205 - Attachment is obsolete: true
Attachment #502593 - Flags: review?(lw)
Attached patch fix v2Splinter Review
refactored after talking to luke irl
Attachment #502593 - Attachment is obsolete: true
Attachment #502614 - Flags: review?(lw)
Attachment #502593 - Flags: review?(lw)
Attachment #502614 - Flags: review?(lw) → review+
Whiteboard: fixed-in-tracemonkey → [fixed-in-tracemonkey][hardblocker]
Closed: 11 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/jit-test/tests/jaeger/bug624100.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.