139316 - Crash involving js_ReportIsNotDefined()

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Phil Schwartau]

Reported by rginda. Crash occurs while using the /eval command in
Chatzilla. Will try to obtain a debug stack trace and exact steps to
reproduce. In the meantime, here are two stack traces from Talkback:

---------------------------  Incident ID 5239571  ---------------------------
0x00000000 
JS_DefineProperty() 
InitExceptionObject() 
js_ErrorToException() 
ReportError() 
js_ReportErrorNumberVA() 
JS_ReportErrorNumber() 
js_ReportIsNotDefined() 
js_Interpret() 
js_Execute() 
obj_eval() 
js_Invoke() 
js_Interpret() 
js_Invoke() 
js_Interpret() 
js_Invoke() 
js_Interpret() 
js_Execute() 
JS_EvaluateUCScriptForPrincipals() 
nsJSContext::EvaluateString() 
GlobalWindowImpl::RunTimeout() 
GlobalWindowImpl::TimerCallback() 
nsTimerImpl::Fire() 
handleTimerEvent() 
PL_HandleEvent() 
PL_ProcessPendingEvents() 
nsEventQueueImpl::ProcessPendingEvents() 
event_processor_callback() 
our_gdk_io_invoke() 
libglib-1.2.so.0 + 0xff9e (0x4038df9e) 
libglib-1.2.so.0 + 0x11773 (0x4038f773) 
libglib-1.2.so.0 + 0x11d39 (0x4038fd39) 
libglib-1.2.so.0 + 0x11eec (0x4038feec) 
libgtk-1.2.so.0 + 0x94333 (0x402aa333) 
nsAppShell::Run() 
nsAppShellService::Run() 
main1() 
main() 
libc.so.6 + 0x1c507 (0x404d5507) 


---------------------------  Incident ID 5493893  ---------------------------
JS_DefineProperty() 
InitExceptionObject() 
js_ErrorToException() 
ReportError() 
js_ReportErrorNumberVA() 
JS_ReportErrorNumber() 
js_ReportIsNotDefined() 
js_Interpret() 
js_Execute() 
obj_eval() 
js_Invoke() 
js_Interpret() 
js_Invoke() 
js_Interpret() 
js_Invoke() 
js_Interpret() 
js_Execute() 
JS_EvaluateUCScriptForPrincipals() 
nsJSContext::EvaluateString() 
GlobalWindowImpl::RunTimeout() 
GlobalWindowImpl::TimerCallback() 
nsTimerImpl::Fire() 
handleTimerEvent() 
PL_HandleEvent() 
PL_ProcessPendingEvents() 
nsEventQueueImpl::ProcessPendingEvents() 
event_processor_callback() 
our_gdk_io_invoke() 
libglib-1.2.so.0 + 0xff9e (0x4038df9e) 
libglib-1.2.so.0 + 0x11773 (0x4038f773) 
libglib-1.2.so.0 + 0x11d39 (0x4038fd39) 
libglib-1.2.so.0 + 0x11eec (0x4038feec) 
libgtk-1.2.so.0 + 0x94333 (0x402aa333) 
nsAppShell::Run() 
nsAppShellService::Run() 
main1() 
main() 
libc.so.6 + 0x1c507 (0x404d5507)

Comment 1

[Robert Ginda]

I think this is still out there, and it's a lame one.

We take a *very* long time to create exception objects at best, and in the worst
case, we crash.  I'm spinning a new debug build now to try to get a stack with
line numbers.

Comment 2

[Robert Ginda]

Looks to me like jsexn.c:436 is trying to call toSource on every parameter. 
This  large objects easily make the problem apparent...

In chatzilla...

/eval try { foo } catch (e) { dumpObjectTree (e) }

+ message (string) 'foo is not defined'
+ fileName (string) 'chrome://chatzilla/content/handlers.js'
+ lineNumber (number) 1789
+ stack (string) 48831 chars
+ name (string) 'ReferenceError'
*

Comment 3

[Robert Ginda]

Attachment: proposed patch

proposed patch

for string arguments, use string source if the string is less than 75
characters,  otherwise use "%u chars".
for function arguments, use the function name, or "anonymous function".
for all other arguments, use argument converted to a string.

Comment 4

[Robert Ginda]

Attachment: test file

test file for the js shell (sorry phil, not a "real" testcase.)
This outputs:

makeError(function c, anonymous function, [object Object], null, undefined,
1.7976931348623157e+308, 0, 1, "hello world", 217 chars, newstring, true,
false, Thu May 02 2002 02:54:09 GMT-0700 (PDT))@exntest.js:8
anonymous function()@exntest.js:24
[e-mail]:28

notice that the parameter with the value of |new String("newstring")| is
printed without the surrounding quotes.  This doesn't bother me too much, some
might even consider it a feature :)

Comment 5

[Brendan Eich [:brendan]]

Let me take a swing at this, I have a better idea.

/be

Comment 6

[Robert Ginda]

> Let me take a swing at this, I have a better idea.

Care to expand on that?

I'd really hate to see RC2 go out with this problem.  I use chatzilla as a js
scratchpad all the time, and crashing mozilla instead of displaying an error
message is hardly acceptable.

Can we check this in, and switch to your better idea post RC2?

Comment 7

[Brendan Eich [:brendan]]

Attachment: brendan's proposed 1.0 branch fix

Use js_ValueToString, not js_ValueToSource.

I have a big patch for 1.1alpha that limits the number of characters in the
js_ValueToLimitedSource result string (and #defines js_ValueToSource in terms
of that new internal function -- same for js_ValueTo{,Limited}String).	But for
1.0 I think we want to do as little as possible.

I don't think the special casing in the other patch is desirable in the short
or long run.

/be

Comment 8

[Brendan Eich [:brendan]]

Looking for fast r= and sr= for the 1.0 branch only -- cc'ing asa so he can
track this one.

/be

Comment 9

[Robert Ginda]

I don't mind the small patch for the short term, but I would like to defend the
long term benefits of (some of) the special casing in my previous patch...

Printing the entire function content isn't going to help anyone, and is going to
cloud the *real* information in noise.

Not printing a name for toplevel and anonymous functions makes the output hard
to parse.  Especially for human readers, who are looking for that chunk of text
before the open paren as a visual marker for the beginning of a stack frame.

Comment 10

[Robert Ginda]

Comment on attachment 82301 [details] [diff] [review]
brendan's proposed 1.0 branch fix

r=rginda

Comment 11

[Brendan Eich [:brendan]]

Attachment: brendan's proposed 1.0 branch fix, v2

rginda helped debug and we found the source of the talkback crashes.  The
errObject newborn in js_ErrorToException was not rooted or locked, and its
cx->newborn root pigeon-hole could get overwritten by some other object birth
under InitExceptionObject (no doubt from a toString or toSource call of some
sort that allocates objects in the course of doing its work).

As a hack with which we will not keep compatibility, also based on rob's
feedback, I made a special case for function argument pretty-printing in the
exception stack property -- the identifier for named functions is used by
itself instead of its decompiled, pretty-printed with indentation and newlines,
 function declaration (what comes back from Function.prototype.toString).

Still a small patch, and I crave rginda's r= and shaver's sr= for 1.0.

/be

Comment 12

[Brendan Eich [:brendan]]

Rob, where did any version of the code fail to print the name for top-level
functions?  You wrote "[n]ot printing a name for toplevel and anonymous
functions  ..." but I don't see how top-level functions (which are named by
definition) can lack an identifier.

I'm hoping you will r= the latest patch.  It special-cases named function
arguments by printing only the function identifier, and uses toSource (which
does not pretty-print, so no newlines) for anonymous functions.  I think that's
reasonable for now -- anonymous functions are otherwise hard to distinguish, and
I don't think the chars in "anonymous function" are worth their weight in any
scenario.

/be

Comment 13

[Robert Ginda]

Comment on attachment 82307 [details] [diff] [review]
brendan's proposed 1.0 branch fix, v2

You'll need to convert |v| to a function to use it with |JS_GetFunctionId|. 
r=rginda with that change.

Anonyomous functions and top level *scripts* (sorry, I meant to say scripts,
not functions) are still printed without any identifier in the "frame name"
portion of the output, as in the follwing 3 frame call stack...


 makeError(c,(function () {}),[object
Object],null,undefined,1.7976931348623157e+308,0,1,hello
world,XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX,newstring,tru
e,false,Sat May 04 2002 15:36:06 GMT-0700 (PDT))@exntest.js:8  <- frame 0
()@exntest.js:24 <- frame 1
@exntest.js:28 <- frame 2

I find this makes it harder to read the output.   Just a thought.

Comment 14

[Brendan Eich [:brendan]]

Attachment: brendan's proposed 1.0 branch fix, v3

Duh -- fixed to use JS_ValueToFunction per rginda's review.  Shaver, can you
sr=?

I'm a less-is-more Unix kind of guy, so an empty string before the @ seems
better to me than "top-level-script" or some such string.  You'll get used to
it ;-).

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: brendan's proposed 1.0 branch fix, v4 (smaller)

Rather than spend space and time (and risk OOM failures) with a GC-thing lock,
why not JS_SetPendingException early, which roots errObject by definition?

/be

Comment 16

[Robert Ginda]

Comment on attachment 82553 [details] [diff] [review]
brendan's proposed 1.0 branch fix, v4 (smaller)

r=rginda

Comment 17

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 82553 [details] [diff] [review]
brendan's proposed 1.0 branch fix, v4 (smaller)

sr=shaver.

Comment 18

[Brendan Eich [:brendan]]

Fixed in trunk.  Asking drivers for approval for 1.0.

/be

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 82553 [details] [diff] [review]
brendan's proposed 1.0 branch fix, v4 (smaller)

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 20

[Brendan Eich [:brendan]]

Fix in branch too.

/be

Comment 21

[Phil Schwartau]

Verified on trunk and branch per discussion with rginda: the reported
crash no longer occurs. Adding fixed1.0.0 and verified1.0.0 keywords.

Comment 22

[Bob Clary [:bc] (inactive)]

Checking in regress-139316.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-139316.js,v  <--  regress-139316.js
initial revision: 1.1