127136 - Exception backtrace code dies recursive death on js_ValueToSource error

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Nesse (gone)]

The checkin for bug 123177 has caused (exposed ?) a bug on the Mac in both OS 9
and X. If you go to the URL above it will load a Java applet. Once it is loaded
move the mouse over the applet and wait. Momentarily the watch cursor will
appear, Mozilla will stop responding, and will eventually crash.

What is happening is that some tool tip code is being fired. This code is
causing a JS error to be thrown and this causes an infinite loop to occur in the
error handling which eventually blows out the stack.

Sorry for the lack of useful information (functions, stacks, etc.) Right now, I
am still rebuilding after backing out, testing, and reapplying the patch from
bug 123177. I will add additional information when I can.

This is essentially blocking all Mac java plugin work right now...

Comment 1

[timeless]

if you're going to mark this as a blocker, please push a copy of the testcase 
to gila, if you don't have access to gila, I'll gladly do it.

Comment 2

[Brian Nesse (gone)]

http://blues/users/bnesse/publish/applets/battleship/battleship.html

Isn't the only applet which exhibits this behaviour... it just happened to be
the one I was testing with.

I have changed the url to one not behind the firewall which will exhibit the
same behavior. Go to:
http://www.playsite.com/games/list.gsp?root=playsite.word.scrabble

And select "Scrabble Lobby" from the beginner section. You will get a "loading"
window, followed by a new page for the lobby, and then it will hang.

Comment 3

[Brian Nesse (gone)]

Attachment: Early in the recursion

Comment 4

[Brian Nesse (gone)]

Attachment: Already dead... just don't know it yet.

Comment 5

[Brian Nesse (gone)]

Attachment: We finally die trying to purge memory...

Comment 6

[Brian Nesse (gone)]

I have now tried reproducing this with a wider variety of applets. Some applets 
exhibit this problem, others don't. I don't know why. I guess I'll reduce the 
severity...

timeless, I don't have access to gila. If you'd like to put the test case there, 
I'll gladly email you a .sit archive. It is certainly a simpler test case than 
the playsite link.

Comment 7

[Brian Nesse (gone)]

Attachment: The original testcase

Hmm, I just discovered that the scrabble test case works in OS 9, but failed in
OS X. That might mean it's a different problem. Attaching the original
(battleship) test case (Mac stuffit archive.)

Comment 8

[Brendan Eich [:brendan]]

Badness: js_ErrorToException needs to stop recursive runaways.  More in a bit.

/be

Comment 9

[Stuart Parmenter]

I'm also seeing this on Win2k

Comment 10

[Brendan Eich [:brendan]]

Attachment: proposed fix

(diff -wu because I eliminated the last few tabs in jscntxt.h!)

This reuses the now-unused clearingScope JSPackedBool in JSContext, renaming it
creatingException, to prevent runaway recursion throughout the Exception ctor. 
I also took the liberty of fixing the XXXbe in the previous stack string
formatting code, namely, that it chopped everything to ISO-Latin-1 from UCS2. 
Finally, I fixed an unrelated, ancient glitch in js_DefineFunction where a user
of the JS_DefineFunction API might mix JSFUN_* flags with JSPROP_* ones and
unintentionally set aliasing JSPROP_* attributes that share bits with JSFUN_*.

Looking for fast testing and review for 0.9.9.

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: proposed fix

I attached a slightly out of date version of the patch (cosmetics and an
unnecessary null check before calling JS_free(cx, stackbuf) in the !ok case --
JS_free null-checks, so no need for an if in the caller).

/be

Comment 12

[Brian Nesse (gone)]

Mac OS 9 and OS X give this patch two thumbs up. :)

Comment 13

[Brendan Eich [:brendan]]

Attachment: proposed fix with ambiguity between top-level/evel'ed script and unnamed zero-args function eliminated

()@foo.js:55 could be a script eval, or it could be an unnamed,
zero-args-passed function call, in the previous patch.	This patch eliminates
the (), leaving the @ separator at the front of the line.

/be

Comment 14

[rogerl (gone)]

Comment on attachment 71958 [details] [diff] [review]
proposed fix with ambiguity between top-level/evel'ed script and unnamed zero-args function eliminated

(I see odd indenting at line 502). Ran the test suite on windows, all ok. 
Doesn't APPEND_STRING_TO_STACK need to call JSSTRING_CHARS rather than just use
->chars?

Comment 15

[Phil Schwartau]

The latest patch also passes the testsuite on Linux,
in both the debug and optimzed JS shell -

Comment 16

[Brendan Eich [:brendan]]

rogerl, thanks -- where is my brain (in 1999?  JSSTRING_CHARS and
JSSTRING_LENGTH are the one true way now, unnatural as they seem to historic
memory).

shaver and I are debating the merits of this mess I've wrought by constructing
an error object.  I hope to have a better patch soon.

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: final (?) proposed fix

I observed to shaver, ruefully, that my changes for bug 123177 exposed many new
failure points in the control flow under js_ErrorToException.  Previously, that
code assumed it would run only into out-of-memory errors, which never turn into
exceptions.  It may have been right -- it may have concluded correctly, based
on hard work by mccabe and rogerl and others, that due to careful design, OOM
was the only possible error that might occur under js_ErrorToException.  I
broke that assumption, or requirement.

My fix for bug 123177 made the Exception constructor, called on its own frame
pushed by js_ConstructObject/js_InternalConstruct, be an indirect subroutine of
js_ErrorToException.  This patch restores the old control flow, more or less:
js_ErrorToException never calls Exception; both of those functions call a new
InitExceptionObject helper.  Both use cx->creatingException to stop recursion
through themselves.  This seems safest to me.

Comments, review, testing still craved quickly for 0.9.9.

/be

Comment 18

[Brendan Eich [:brendan]]

bug 128219's patch, for jsobj.c, is rolled up here.

This patch is also bigger than it could be, because of (a) the Unicode purity
fix, which I think should go into 0.9.9 because it's easy to test; (b) factoring
"up" (toward the top of the file) of InitExceptionObject from Exception (pushing
the new subroutine down below its caller would minimize certain diffs, but would
require a static forward decl -- yuck).  I hope these comments help make it more
readable.  Applying it and reading thru jsexn.c might help too.

/be

Comment 19

[Brendan Eich [:brendan]]

Asa, another one for 0.9.9 on a short fuse.

/be

Comment 20

[rogerl (gone)]

Comment on attachment 71984 [details] [diff] [review]
final (?) proposed fix

Getting this warning that seems to speak to a cut'n'paste issue:
d:\ns_trunk\mozilla\js\src\jsexn.c(934) : warning C4700: local variable
'filenameStr' used without having been initialized

Comment 21

[Brendan Eich [:brendan]]

Attachment: final patch for today, and for 0.9.9/1.0 i hope!

gcc without -O doesn't do much to help me -- thanks rogerl.

/be

Comment 22

[Brendan Eich [:brendan]]

Attachment: proposed fix, plus fixes for

Latest and greatest, fixes two probs in the last patch:

1.  The stackbuf jschar array grows by powers-of-two (plus 1 jschar for the NUL
terminator), so it should be realloc'd to (stacklen+1) * sizeof(jschar) when we
are done -- when stacklen is finalized.  This requires some care due to FreeBSD
failure potential on a shrinking realloc, reported by tellme.com folks.

2.  fun_xdrObject was reversing arg and var property order when encoding, so a
decoder would fail to share property tree goodness.

The jsobj.c patch for bug 128219 is still here; shaver's hip to that.

/be

Comment 23

[Mike Shaver (:shaver emeritus)]

Comment on attachment 72108 [details] [diff] [review]
proposed fix, plus fixes for 

I like it. sr=shaver.

Comment 24

[Phil Schwartau]

Testcase added to JS testsuite:

          mozilla/js/tests/js1_5/Exceptions/errstack-001.js

The latest patch is not giving me the results I expect for the 
following two types of function calls:

A) myErr = function() { call some other function that returns an error } ();

B) var f = Function('call some other function that returns an error');
   myErr = f();


In case A), I expect that stack frame to begin with '()@'
Im getting this instead:                            '@./'

In case B), I expect that stack frame to begin with 'anonymous()@'
I'm getting this instead:                           'anonymous@./'


I'm probably misunderstanding Comment #13, but don't we want
empty parens to display? If parameters were passed, we'd display
parens with the parameter values; why wouldn't we want to display
parens if no parameters are passed?

Comment 25

[Phil Schwartau]

To amplify my previous comment: here's |myErr.stack| in each case:

Case A)
Error("meep!")@:0 
D(47,16)@./js1_5/Exceptions/errstack-001.js:90 
C(46,15)@./js1_5/Exceptions/errstack-001.js:83 
B(45,14)@./js1_5/Exceptions/errstack-001.js:78 
A(44,13)@./js1_5/Exceptions/errstack-001.js:73 
@./js1_5/Exceptions/errstack-001.js:160 
@./js1_5/Exceptions/errstack-001.js:160 


Case B)
Error("meep!")@:0 
D(47,16)@./js1_5/Exceptions/errstack-001.js:90 
C(46,15)@./js1_5/Exceptions/errstack-001.js:83 
B(45,14)@./js1_5/Exceptions/errstack-001.js:78 
A(44,13)@./js1_5/Exceptions/errstack-001.js:73 
anonymous@./js1_5/Exceptions/errstack-001.js:184 
@./js1_5/Exceptions/errstack-001.js:185


In Case A), are the bottom two frames BOTH at top-level???
They both begin with '@' !!!

In Case B), I'm asking for 'anonymous()' instead of 'anonymous'.
I believe that's what would happen for function A(), for example,
if we did not pass any parameters to it.

Comment 26

[Phil Schwartau]

Just checked, with the patch, a named function A() appears
like this if defined with parameters:

                 A(44,13)@*****************

but like this if not defined with parameters:

                 A@************************

I think the latter should be

                 A()@**********************

Comment 27

[Brendan Eich [:brendan]]

Attachment: stick a fork in me, i'm done

Thanks, Phil.  This patch fixes the zero-args-function case, and improves
commentsin jscntxt.h per private shaver comments, and commons an expression in
jsfun.c, and synchs up to the trunk (I just checked in two other patches).

/be

Comment 28

[Brendan Eich [:brendan]]

Attachment: plain diff, new on the left, of last two patches

Looking for fast shaver re-review; glad rginda hasn't started yet :-).

/be

Comment 29

[Mike Shaver (:shaver emeritus)]

Comment on attachment 72153 [details]
plain diff, new on the left, of last two patches

New on left!  You're a freak.

sr=shaver, as if anyone still thinks that means anything.

Comment 30

[Phil Schwartau]

I'm still getting the test failure described in Comment #24.
That is, the empty parentheses are not showing up.

This is with a fresh pull from CVS, and applying only the patch
(id=72152) from Comment #27.

Comment 31

[Phil Schwartau]

I think something may have gone wrong with my patch process this time.
I got a "Hunk #1 succeeded" and a "Hunk #3 succeeded" on jsfun.c.
But what about Hunk #2 ???

Brendan, does the test pass for you now? 

             js/tests/js1_5/Exceptions/errstack-001.js

Comment 32

[Brendan Eich [:brendan]]

Phil, that test passes for me.  What's more, the latest code will always print
the '(' and ')' if fp->fun -- if there's an active function.  Not sure why the
latest patch didn't apply cleanly for you.  `cvs stat` and `cvs update` say I'm
up to date still.

/be

Comment 33

[Phil Schwartau]

Hmm...tried all over again, but still getting that same message when
I try to apply the latest patch, and still failing on that testcase.
I'll just have to wait for the check-in to verify the fix -


[/mozilla/js/src] patch < 127136_72152.patch 
patching file `jscntxt.c'
patching file `jscntxt.h'
patching file `jsexn.c'
patching file `jsfun.c'
Hunk #1 succeeded at 1077 (offset 16 lines).
Hunk #3 succeeded at 1156 (offset 16 lines).
patching file `jsobj.c'

Comment 34

[Robert Ginda]

My patch applies with the same two 16 line offsets as phil's.  No test results yet.

Comment 35

[Robert Ginda]

After running all the testcases, I end up with the same 7 failures that are
present in the baseline results...

# Retest List, smdebug, generated Fri Mar  1 17:50:23 2002.
# Original test base was: All tests.
# 1024 of 1024 test(s) were completed, 7 failures reported.
ecma_3/RegExp/regress-123437.js
ecma_3/RegExp/regress-85721.js
js1_5/Array/regress-101964.js
js1_5/Exceptions/errstack-001.js
js1_5/Object/regress-90596-001.js
js1_5/Object/regress-90596-002.js
js1_5/Object/regress-90596-003.js

I'll be in transit for the next two hours, but will try to r= shortly after
coming to rest.

Comment 36

[Phil Schwartau]

There should be 6, not 7, test failures after the patch is applied.
Note that one of Rob's 7 failures is the same one I'm failing on:
the testcase for this bug, js1_5/Exceptions/errstack-001.js.

Here's how that failure looks in my console
(with the patch applied; producing the Hunk-offset messages above):


*-* Testcase js1_5/Exceptions/errstack-001.js failed:
Failure messages were:

FAILED!: Section 12 of test -
FAILED!: Expected value '()@', Actual value '@./'
FAILED!:  
FAILED!: Section 15 of test -
FAILED!: Expected value 'anonymous()@', Actual value 'anonymous@./'


I suppose it's just a problem Rob and I both had in applying the patch,
but I just wanted to show  what we're seeing -

Comment 37

[Brendan Eich [:brendan]]

Attachment: argh! my patch management is the sh!t$

Comment 38

[Phil Schwartau]

That last patch (id=72184) applies cleanly, and passes the key testcase.
Now I am running the entire JS testsuite against it; will report shortly -

Comment 39

[Phil Schwartau]

The last patch (id=72184) passes the entire JS testsuite on Linux, in
both the debug and optimized JS shells. I get only the 6 known failures -

Comment 40

[Mike Shaver (:shaver emeritus)]

Comment on attachment 72184 [details] [diff] [review]
argh! my patch management is the sh!t$

Moving ahead the super-review stamp (which, with $2.50, will get you a
smoothie).

Comment 41

[Robert Ginda]

Comment on attachment 72184 [details] [diff] [review]
argh! my patch management is the sh!t$

That took a little longer than I expected.  r=rginda.

Comment 42

[chris hofmann]

a=chofmann trunk and 0.9.9

Comment 43

[Brendan Eich [:brendan]]

Fixed in branch and trunk.

/be

Comment 44

[Phil Schwartau]

Brian, did this fix the crash? I was not able to reproduce this before,
so I cannot be sure now. If everything is OK now, could you mark this
bug Verified for me? Thanks -

Comment 45

[Brian Nesse (gone)]

Phil, I applied a couple of the earlier revisions with good results. I will
verify against the checked in code ASAP.

Comment 46

[Brian Nesse (gone)]

v. Mac OS 9 & Mac OS X.

Comment 47

[Brian Nesse (gone)]

*** Bug 126482 has been marked as a duplicate of this bug. ***