309852 - E4X - JS_ASSERT @ js_Interpret, line 5070

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Bob Clary [:bc] (inactive)]

This assert began appearing recently in debug builds in
e4x/Regress/regress-301596.js. Related to bug 309850 ?

+	fp->sp	0x0041d9ec
+	sp	0x0041d9f0

NTDLL! 7c901230()
js_Interpret(JSContext * 0x000371b8, unsigned char * 0x00424a84, long *
0x0013ee24) line 5070 + 36 bytes
js_Execute(JSContext * 0x000371b8, JSObject * 0x000387c8, JSScript * 0x004249f8,
JSStackFrame * 0x00000000, unsigned int 0x00000000, long * 0x0013fecc) line 1393
+ 19 bytes
JS_ExecuteScript(JSContext * 0x000371b8, JSObject * 0x000387c8, JSScript *
0x004249f8, long * 0x0013fecc) line 3842 + 25 bytes
Process(JSContext * 0x000371b8, JSObject * 0x000387c8, char * 0x00032d56) line
223 + 22 bytes
ProcessArgs(JSContext * 0x000371b8, JSObject * 0x000387c8, char * * 0x00032cc4,
int 0x00000006) line 426 + 23 bytes
main(int 0x00000006, char * * 0x00032cc4, char * * 0x00033188) line 2552 + 21 bytes
JS! mainCRTStartup + 227 bytes
KERNEL32! 7c816d4f()

Comment 1

[Brendan Eich [:brendan]]

This one's not a bogus assertion, it's a nasty bug in how the JSOP_SETSP emitted
at the start of the catch and/or finally clauses mixes badly with E4X's
filtering predicate expression.  The code generator has to know that the
filtering predicate is executed by an inner interpreter activation that uses the
same operand stack as the outer interpreter activation (the one that executes
the script containing the filtering predicate).

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed minimal fix

This is a bit ugly, but it works.  The !mark => XML filtering predicate
execution coupling is an XXX to reconsider later.  This fix should go into
1.8b5 shortly.

/be

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #1)
> This one's not a bogus assertion, it's a nasty bug in how the JSOP_SETSP emitted
> at the start of the catch and/or finally clauses mixes badly with E4X's
> filtering predicate expression.

True so far.

> The code generator has to know that the
> filtering predicate is executed by an inner interpreter activation that uses the
> same operand stack as the outer interpreter activation (the one that executes
> the script containing the filtering predicate).

False diagnosis on my part -- the code generator can't help here, since only one
script is emitted for a function or top-level script that uses filtering
predicate expressions, and that script includes the expressions' bytecodes.  The
patch is based on the correct diagnosis: nesting an interpreter activation to
execute a filtering predicate means suppressing exception catching until we
unwind to the outer interpreter.

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 197246 [details] [diff] [review]
proposed minimal fix

r=mrbkap

Comment 5

[Brendan Eich [:brendan]]

Here is where I regret choosing to implement filtering predicates other than yb
generating bytecode in the containing script that does not require nesting an
interpreter activation that shares the top operand stack frame.  JSOP_FILTER
would change into a setup bytecode followed by an iterator update branch combo,
sort of like the for-in stuff.  JSOP_ENDFILTER would be a backward jump to the
iterator update test.

I may hack this up while traveling.

/be

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 197246 [details] [diff] [review]
proposed minimal fix

sr=shaver, want to file a bug on the XXX?

Comment 7

[Brendan Eich [:brendan]]

Fixed on the trunk.  Let's see how the followup bug 309894 goes before trying
for the 1.8 branch, so we can choose between minimal hack-fix (this bug's patch)
and the right, forward-compatible bytecoded fix.

/be

Comment 8

[Brendan Eich [:brendan]]

I just checked in fixes to JS_LIKELY abusage in jsinterp.c on the trunk:

revision 3.192
date: 2005/09/26 18:48:39;  author: brendan%mozilla.org;  state: Exp;  lines: +2 -2
Fix pointer testing via JS_LIKELY (rather fix a few callers than put cvs diff
jsinterp.c inside the macro...

/be

Comment 9

[Mike Shaver (:shaver emeritus)]

Brendan: we want to land this on the branch tomorrow morning (AM pacific, Sep
28), to take this assertion/crash off the radar for 1.5b2.  Do you have a
riskless patch up your sleeve for the bytecoded variant, or should we make that
change and the anyname-as-atom serialization change as part of the same XDR
version bump, on the trunk only?

Comment 10

[Mike Shaver (:shaver emeritus)]

Going to verify the delta between branch and trunk and land it on
MOZILLA_1_8_BRANCH in a little while.

Comment 11

[Mike Shaver (:shaver emeritus)]

Comment on attachment 197246 [details] [diff] [review]
proposed minimal fix

Approving on behalf of schrep and brendan and me.

Comment 12

[Asa Dotzler [:asa]]

Need get this landed before time runs out. 

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

It looks like shaver checked this into the branch already:
3.181.2.10 <[e-mail]> 2005-09-28 13:08
309852: hack around XML filtering predicate expression design flaw, patch from
/be, r=mrbkap, sr=shaver, a=schrep/shaver/brendan.