97540 - "call hook" called with partially initialized cx->fp

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robert Ginda]

js_Invoke calls the callHook with a partially initialized cx->fp.  If the
callHook should re-enter the js engine (via xpconnect, for example) we hit the
assert at jsinterp.c:396 ``JS_UPTRDIFF(*markp, fp->spbase) >= depthdiff''.

The proposed patch moves the call hook earlier in js_Invoke, to a position
before the soon-to-be-called frame has actually been appended to cx->fp.

Comment 1

[Robert Ginda]

Attachment: proposed patch, round one; call callHook before we set cx->fp

Comment 2

[John Bandhauer]

I'm a little puzzled about why this causes that assertion. The frame is *almost* 
completely filled in. It seems to lack only varobj, scopeChain, and (in the 
non-native case) callobj.

My original intention was that the hook could also see other things in the frame 
- like the args. I'm not so broken up about losing that. But, I'd like to 
understand just what there is about calling the hook at this point (frame.spbase 
is already set) that makes this cause a problem.

Comment 3

[Brendan Eich [:brendan]]

jband: I too want the hook to be able to see the current frame.  The trouble is,
js_AllocStack wants to be called only after js_Invoke has called js_Interpret
and the latter has allocate depth (actually, depth*2) jsvals on the stack, and
set fp->spbase such that at least depth jsvals lie between it and any new mark
made on cx->stackPool by js_Alloc(Raw)Stack.

More than that JS_ASSERT needs to change.  I'll attempt a patch.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix, no change to callHook semantics

Comment 5

[John Bandhauer]

Brendan: I continue to learn things I had not understood before.

Aren't you also going to need to switch on fp->pc in the code that uses
fp->script->depth in js_gc? If I'm getting it then you risk marking too many slots.

Either...
  depth = fp->pc ? fp->script->depth : 0;
Or...
  if (!fp->pc ||
      JS_UPTRDIFF(fp->sp, fp->spbase) < depth * sizeof(jsval))
No?

Comment 6

[Brendan Eich [:brendan]]

jband: good point -- i hadn't thought of GC being reached from those debugger
hooks, darnit.

/be

Comment 7

[Brendan Eich [:brendan]]

Perhaps it would be better to initialize spbase to null in a new frame, then the
existing fp->spbase tests would do.

/be

Comment 8

[Brendan Eich [:brendan]]

jband: actually, I see no problem in js_GC: it marks sp - spbase slots if that
delta is fewer than depth slots.  At the callHook site, js_Invoke has defaulted
spbase to sp, pending the (common to functions and scripts, therefore located in
js_Interpret) allocation of the 2*depth slots and resetting of spbase and sp in
js_Interpret.

/be

Comment 9

[Brendan Eich [:brendan]]

I also withdraw my null spbase idea, for the obvious reason that it would cause
the GC to mark all depth slots, wrongly.

I think the second patch in this bug is "it" -- any test results confirming me yet?

/be

Comment 10

[Phil Schwartau]

Ran the JS test suite on WinNT against the debug and optimized JS shells 
with Brendan's second patch applied. Only got the current known failures:

ecma_3/Function/regress-94506.js
ecma_3/RegExp/regress-72964.js
ecma_3/RegExp/regress-78156.js
ecma_3/RegExp/regress-87231.js
ecma_3/String/regress-83293.js
js1_5/Object/regress-90596-001.js
js1_5/Object/regress-90596-002.js
js1_5/Object/regress-90596-003.js
js1_5/Object/regress-96284.js
js1_5/Regress/regress-96128-n.js  <---- (this one fails in debug shell only)
js1_5/Regress/regress-97646-001-n.js
js1_5/Regress/regress-97646-002-n.js

Comment 11

[Robert Ginda]

Two issues...
We need to check that cx->runtime->callHook is still valid before we call it in
JSOP_RETURN.

I forced a GC from my callHook, and crashed in js_MarkGCThing.  The crash
happens during the JSOP_RETURN callHook, with either patch applied.

Comment 12

[Robert Ginda]

Attachment: stack trace of GC related crash during JSOP_RETURN callHook

Comment 13

[Brendan Eich [:brendan]]

Attachment: my final answer!

Comment 14

[Robert Ginda]

The inline return case now calls executeHook instead of callHook.  Other than
that, this fixes the crashes.  I'll run the full suite tonight and report back.

Comment 15

[Brendan Eich [:brendan]]

Attachment: I lied -- rob pointed out a copy-paste error (executeHook instead of callHook) -- one line fix to last patch, works for rginda

Comment 16

[John Bandhauer]

[3-way midair on the executeHook\callHook error. We all have eagle eyes!]

You reversed yourself on the spbase issue? I'll have to dig deeper to 
understand/verify that this is right.

Comment 17

[John Bandhauer]

-                if (fp->script) {
+                if (fp->script && fp->spbase) {
                     js_MarkScript(cx, fp->script, NULL);

Don't you risk the script not getting marked here? Shouldn't the 
  if (fp->spbase) 
nest inside after marking the script? Or is the script certain to be marked via 
some other path I'm not thinking of?

-                 fun->flags == 0 &&
+                 !(fun->flags & (JSFUN_HEAVYWEIGHT | JSFUN_BOUND_METHOD)) &&

Did this sneek in?

Comment 18

[John Bandhauer]

Otherwise the spbase-based scheme looks good to me (though, I'm not so sure I'd 
have thought of the wrinkle of clearing it at the bottom of the interpreter 
loop).

Comment 19

[Brendan Eich [:brendan]]

jband: thanks, I was rushing cuz rob was heading out.  Quite right, I need to
split the if condition in jsgc.c.  The fun->flags test is a shameful admission
that I and my super-review buddies blew it, several times.  Especailly when
JSFUN_LAMBDA was added, purely for decompilation purity.  All lambdas are
invoked via the heavyweight machinery.  I suck!

New patch coming up.

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: ok, this one's the bomb! r= and sr= please

Comment 21

[Brendan Eich [:brendan]]

My instinct toward minimizing terms in the logic favored testing fp->spbase, and
that was right.  The problem with testing pc was that on the way ouf of
js_Interpret, returning through js_Invoke, a call hook would see fp->pc as
non-null (and might want to, to get the return instruction if debugging an early
return situation).  Adding yet another flag was too noxious.  It made more
sense, once I'd surveyed uses of spbase, to keep it null except when the operand
stack and its "basement" of generating pc's was live, all within js_Interpret.

Please shower this patch with review love; we would like the full fix for 0.9.4.
/be

Comment 22

[Robert Ginda]

I've applied the latest patch here and it runs great in venkman.  I think I even 
notice a speedup from the lambda test.
My tree is too old (and I don't have cvs access from here) to run the full 
suite, but assuming it passes all tests, r=rginda.

If noone gets to it first, I'll run the full test against the tip tomorrow.

Comment 23

[John Bandhauer]

r/sr=jband

This looks right to me. 

I ran the JS tests on a non-debug NT jsshell build and
got the same results with and without the patch.

These are the ususal suspects, no?

# Retest List, smopt, generated Thu Aug 30 22:52:25 2001.
# Original test base was: All tests.
# 985 of 985 test(s) were completed, 10 failures reported.
ecma_3/Function/regress-94506.js
ecma_3/RegExp/regress-72964.js
ecma_3/RegExp/regress-78156.js
ecma_3/RegExp/regress-87231.js
ecma_3/String/regress-83293.js
js1_5/Object/regress-90596-001.js
js1_5/Object/regress-90596-002.js
js1_5/Object/regress-90596-003.js
js1_5/Regress/regress-97646-001-n.js
js1_5/Regress/regress-97646-002-n.js

Brendan loves the ternary operator!

Comment 24

[Phil Schwartau]

Correct - those are the known JS test failures. Differs slightly from
my list above because:

js1_5/Object/regress-96284.js      <---- I haven't checked this into CVS yet
js1_5/Regress/regress-96128-n.js   <---- this one fails in debug shell only

Comment 25

[Brendan Eich [:brendan]]

Rob, phil: if you could test-ify (heh, get it? unconditional r=rginda would be
even better) that everything looks good today, I'll go to drivers.

/be

Comment 26

[Robert Ginda]

I get the same results as jband on a debug linux build, r=rginda

Comment 27

[Phil Schwartau]

I am now running the full test suite on Linux and WinNT with the
"bomb" patch applied; will report in a few minutes -

Comment 28

[Phil Schwartau]

I get the same results as jband and rginda with the "bomb" patch applied.
No regressions were introduced when the full JS test suite was run under:

1. WinNT debug shell
2. WinNT optimized shell
3. Linux debug shell
4. Linux optimized shell

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 47772 [details] [diff] [review]
ok, this one's the bomb! r= and sr= please

a=asa on behalf of drivers

Comment 30

[Brendan Eich [:brendan]]

Fix checked in, thanks all.

/be

Comment 31

[Phil Schwartau]

Marking Verified -