44009 - assertion with window.toSource()

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Mitchell Stoltz (not reading bugmail)]

The following crashes Mozilla on Windows 98, don't know for other OS.
----------------
<SCRIPT>
alert(window.toSource());
</SCRIPT>
----------------

Comment 1

[cathy zhang]

confirmed, it crashes 6/27 build on windowsNT, the instruction at "0x60b5e669" 
referenced memory at 0x00000001". the memory could not be "written".

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Reassign to DOM 0.

Comment 3

[Johnny Stenback (:jst)]

Brendan, any ideas? Here's the stack for an assert I hit when executing
alert(window.toString());.

JS_Assert(const char * 0x00324fec, const char * 0x00324fc0, int 549) line 176
js_DropScopeProperty(JSContext * 0x0336d420, JSScope * 0x034e5460,
JSScopeProperty * 0x00000001) line 549 + 45 bytes
js_DropProperty(JSContext * 0x0336d420, JSObject * 0x02e630f0, JSProperty *
0x00000001) line 2767 + 19 bytes
MarkSharpObjects(JSContext * 0x0336d420, JSObject * 0x02e630f0, JSIdArray * *
0x00000000) line 355 + 37 bytes
MarkSharpObjects(JSContext * 0x0336d420, JSObject * 0x02e630c8, JSIdArray * *
0x00000000) line 362 + 34 bytes
MarkSharpObjects(JSContext * 0x0336d420, JSObject * 0x02df21b0, JSIdArray * *
0x0012e4f8) line 362 + 34 bytes
js_EnterSharpObject(JSContext * 0x0336d420, JSObject * 0x02df21b0, JSIdArray * *
0x0012e57c, unsigned short * * 0x0012e568) line 411 + 17 bytes
js_obj_toSource(JSContext * 0x0336d420, JSObject * 0x02df21b0, unsigned int 0,
long * 0x02ed60c8, long * 0x0012e664) line 526 + 21 bytes
js_Invoke(JSContext * 0x0336d420, unsigned int 0, unsigned int 0) line 716 + 23
bytes
js_Interpret(JSContext * 0x0336d420, long * 0x0012f01c) line 2520 + 15 bytes
js_Execute(JSContext * 0x0336d420, JSObject * 0x02df21b0, JSScript * 0x033acda0,
JSFunction * 0x00000000, JSStackFrame * 0x00000000, unsigned int 0, long *
0x0012f01c) line 887 + 13 bytes
JS_EvaluateUCScriptForPrincipals(JSContext * 0x0336d420, JSObject * 0x02df21b0,
JSPrincipals * 0x033ace70, const unsigned short * 0x0012f664, unsigned int 27,
const char * 0x033ace20, unsigned int 5, long * 0x0012f01c) line 2768 + 27 bytes
nsJSContext::EvaluateString(nsJSContext * const 0x0336d5b0, const nsString &
{len=27 "
alert(window.toSource());
"}, void * 0x02df21b0, nsIPrincipal * 0x033ace6c, const char * 0x033ace20,
unsigned int 5, const char * 0x00000000, nsString & {len=0 ""}, int *
0x0012f078) line 525 + 55 bytes
HTMLContentSink::EvaluateScript(nsString & {len=27 "
alert(window.toSource());
"}, nsIURI * 0x03740dd0, int 5, const char * 0x00000000) line 4455

Comment 4

[Brendan Eich [:brendan]]

Looks like something is broken in JS's Object.prototype.toSource native method. 
Mine, all mine!

/be

Comment 5

[leger]

Adding crash keyword

Comment 6

[Brendan Eich [:brendan]]

Is this still reproducing?  I need to update my two-week-old build.

/be

Comment 7

[Johnny Stenback (:jst)]

Yup, still crashing for me, I wasn't able to reproduce this in the commercial
Netscape bits but in my own debug build from a few days ago I still see the crash...

Comment 8

[Phil Schwartau]

Attachment: Here is the HTML file that causes the crash

Comment 9

[Phil Schwartau]

Using Mozilla tip builds 2000-08-07 on WinNT, Linux.

When I click on the attachment above, I do not crash right away. In fact, 
nothing happens: I do not even see the assert box that is supposed to come up. 
When I click the "Reload" button in the browser, I then crash on both Linux 
and WinNT. Stack trace to follow...

Comment 10

[Phil Schwartau]

Attachment: Linux stack trace

Comment 11

[Mike McCabe]

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

Comment 12

[Mike McCabe]

Please see 50042, marked as a dup. of this bug.  I tracked down the offending
checkin, which attempts to add outermost parens to the resulting string, and
adds the offending JS_ASSERT.  Removing the offending JS_ASSERT fixes this.  The
resulting string doesn't have outermost parentheses, but this is hardly a fatal
error.

Comment 13

[Brendan Eich [:brendan]]

This is not just a bogus assert.  Several things are going wrong: one is that a
GetProperty on a window property is failing deep in nsRDFService.cpp:1122,
nsRDFServiceImpl::GetDataSource, because aURI is null.  That leads to a bug in
jsobj.c, under toSource's native method a few calls (MarkSharpObjects) where the
sharpObjectMap is not cleaned up after error.  Oops.

I'll fix jsobj.c and close this bug.  It seems like GetDataSource is being
passed a null URI from some JS in the function starting at nsSidebar.js:57 named
nsSidebar().  Cc'ing rjc and slamm for insight on the propriety of that.

/be

Comment 14

[Brendan Eich [:brendan]]

Attachment: proposed fix: clean up the sharpObjectMap on outermost error

Comment 15

[Brendan Eich [:brendan]]

Looking for r=.  The crash happened on the second load of the test page, because
the first load led to an early error return in js_obj_toSource, due to !he from
js_EnterSharpObject.  The rule is that if js_EnterSharpObject returns null, the
JS error has already been reported and everything related to sharp objects has
been cleaned up.

That wasn't the case!  The sharpObjectMap.table JSHashTable contained an entry
for the window object, so the second call found that to be "sharp" already
(referenced by a #1= a la Common Lisp, as if there were a cycle or join-point in
the object graph).

The fix is to carefully clean up sharpObjectMap.table (destroy and clear) on
error return inside js_EnterSharpObject, but only if that call created the table
(i.e., only if sharpObjectMap.depth is 0).

/be

Comment 16

[Robert John Churchill]

Brendan, update your nsSidebar.js file... I believe slamm checked in a fix 
yesterday for the problem you are seeing with it.

Comment 17

[Brendan Eich [:brendan]]

rjc, thanks -- glad I didn't update, otherwise I might not have been able to
reproduce this bug!

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: slightly better control flow in js_EnterSharpObject

Comment 19

[Mike Shaver (:shaver emeritus)]

I'll buy that: r=shaver

Comment 20

[Steve Lamm]

rjc is right. I checked in a fix to nsSidebar.js on Wednesday, August 30.

Comment 21

[Brendan Eich [:brendan]]

Fix checked in, r=shaver.  Thanks,

/be

Comment 22

[Mike McCabe]

Reopening.

This bug still occurs on trunk builds; the js shell crashes on the following:

js> o = {}; o.foo = o; o.toSource()
Assertion failure: !outermost, at jsobj.c:575
zsh: abort (core dumped)  js

Comment 23

[Brendan Eich [:brendan]]

mccabe, little help?  I'm trying to land a big one.  Thanks,

/be

Comment 24

[Patrick C. Beard]

Can you look at this again? 

Comment 25

[Phil Schwartau]

Current results of mccabe's testcase above: o = {}; o.foo = o; o.toSource() 


Optimized JS Shell: no crash

[/d/js/src/WINNT4.0_OPT.OBJ] ./js
js>  o = {}; o.foo = o; o.toSource()
#1=({foo:#1#})



Debug JS Shell: crashes

NTDLL! 77f7629c()
js_obj_toSource(JSContext * 0x00301e80, JSObject * 0x002fab58, unsigned int 0, 
long * 0x00420064, long * 0x0012e3b4) line 590 + 35 bytes
js_Invoke(JSContext * 0x00301e80, unsigned int 0, unsigned int 0) line 777 + 23 
bytes
js_Interpret(JSContext * 0x00301e80, long * 0x0012fed8) line 2670 + 15 bytes
js_Execute(JSContext * 0x00301e80, JSObject * 0x002fa740, JSScript * 0x00306180, 
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0012fed8) line 956 + 13 
bytes
JS_ExecuteScript(JSContext * 0x00301e80, JSObject * 0x002fa740, JSScript * 
0x00306180, long * 0x0012fed8) line 3113 + 25 bytes
Process(JSContext * 0x00301e80, JSObject * 0x002fa740, char * 0x00000000) line 
372 + 22 bytes
ProcessArgs(JSContext * 0x00301e80, JSObject * 0x002fa740, char * * 0x00300164, 
int 0) line 508 + 17 bytes
main(int 0, char * * 0x00300164) line 2050 + 21 bytes
JS! mainCRTStartup + 227 bytes
KERNEL32! 77f1ba06()

Comment 26

[Brendan Eich [:brendan]]

Attachment: remove bogus assertion, avoid unnecessary uneval-parens around #1={foo:#1#} and the like

Comment 27

[Brendan Eich [:brendan]]

Looking for r= and sr= -- rogerl, shaver, jband, please help me get this easy
fix into mozilla0.8.

/be

Comment 28

[Brendan Eich [:brendan]]

I improved the comments, which should explain what's going on well enough to
review the fix.  I didn't expand tabs in lines that I didn't add; I can do that
as a final step before checkin if you like.

/be

Comment 29

[Mike Shaver (:shaver emeritus)]

sr=shaver, and please do tab-expand.  (Tabs in js/src? What's the world coming to?)

Comment 30

[rogerl (gone)]

r=rogerl

Comment 31

[Brendan Eich [:brendan]]

Fix is in.  Thanks,

/be

Comment 32

[Phil Schwartau]

Have added regression test to JS test suite for this bug - 

          js/tests/js1_5/Regress/regress-44009.js

Comment 33

[Phil Schwartau]

Here is the core of the testcase: 


var obj1 = {};
var sToSource = ''; 
var self = this; 


//--------------------------------------------------
test();
//--------------------------------------------------


// test various objects and scopes - 
function test() 
{
 var obj2 = {};

 testThis(self);
 testThis(this);
 testThis(obj1);
 testThis(obj2);
}


// Just testing that we don't crash by doing this - 
function testThis(obj)
{
 sToSource = obj.toSource();
 obj.prop = obj; 
 sToSource = obj.toSource();
}

Comment 34

[Phil Schwartau]

Reopening bug. The testcase crashes on WinNT and Linux with a DEBUG build
of the standalone JS shell. It does NOT crash with an optimized build.


The original HTML testcase no longer crashes. Nor does mccabe's testcase
at 2000-11-22 15:41 above. But there is something in my testcase that
is going wrong -

Comment 35

[Phil Schwartau]

Here is the error I get in the console on Linux:


BUGNUMBER: 44009
STATUS: Testing that we don't crash on obj.toSource()

Assertion failure: size > pool->arenasize, at jsarena.c:156
Aborted (core dumped)

Comment 36

[Phil Schwartau]

And here is a WinNT stack trace: 


NTDLL! 77f7629c()
JS_ArenaRealloc(JSArenaPool * 0x00301ef0, void * 0x00427840, unsigned int 1019, 
unsigned int 17) line 156 + 40 bytes
SprintAlloc(Sprinter * 0x0012d6a8, unsigned int 17) line 302 + 165 bytes
QuoteString(Sprinter * 0x0012d6a8, JSString * 0x002fb920, unsigned short 34) 
line 393 + 19 bytes
js_QuoteString(JSContext * 0x00301e80, JSString * 0x002fb920, unsigned short 34) 
line 431 + 18 bytes
js_ValueToSource(JSContext * 0x00301e80, long 3127588) line 2407 + 18 bytes
js_obj_toSource(JSContext * 0x00301e80, JSObject * 0x002fa740, unsigned int 0, 
long * 0x002fd904, long * 0x0012d814) line 703 + 17 bytes
js_Invoke(JSContext * 0x00301e80, unsigned int 0, unsigned int 0) line 777 + 23 
bytes
js_Interpret(JSContext * 0x00301e80, long * 0x0012e314) line 2670 + 15 bytes
js_Execute(JSContext * 0x00301e80, JSObject * 0x002fa740, JSScript * 0x0030ba60, 
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0012e314) line 956 + 13 
bytes
JS_ExecuteScript(JSContext * 0x00301e80, JSObject * 0x002fa740, JSScript * 
0x0030ba60, long * 0x0012e314) line 3117 + 25 bytes
Load(JSContext * 0x00301e80, JSObject * 0x002fa740, unsigned int 1, long * 
0x002fd7fc, long * 0x0012e3c8) line 638 + 22 bytes
js_Invoke(JSContext * 0x00301e80, unsigned int 1, unsigned int 0) line 777 + 23 
bytes
js_Interpret(JSContext * 0x00301e80, long * 0x0012fed8) line 2670 + 15 bytes
js_Execute(JSContext * 0x00301e80, JSObject * 0x002fa740, JSScript * 0x0030a990, 
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0012fed8) line 956 + 13 
bytes
JS_ExecuteScript(JSContext * 0x00301e80, JSObject * 0x002fa740, JSScript * 
0x0030a990, long * 0x0012fed8) line 3117 + 25 bytes
Process(JSContext * 0x00301e80, JSObject * 0x002fa740, char * 0x00000000) line 
372 + 22 bytes
ProcessArgs(JSContext * 0x00301e80, JSObject * 0x002fa740, char * * 0x00300164, 
int 0) line 530 + 17 bytes
main(int 0, char * * 0x00300164) line 2096 + 21 bytes
JS! mainCRTStartup + 227 bytes
KERNEL32! 77f1ba06()

Comment 37

[Brendan Eich [:brendan]]

WORKSFORME.

Phil, are you sure your shells are correctly built?  Did you clobber
Linux_All_DBG.OBJ before rebuilding, in particular?

/be

Comment 38

[Phil Schwartau]

I always do
                 make -f Makefile.ref clean

before building; is that enough? 

Comment 39

[Phil Schwartau]

On Linux, I deleted the js/src directory altogether, re-pulled js/src, 
and re-built. But I still get the same crash on this testcase...

Comment 40

[Brendan Eich [:brendan]]

Ah, got it by running the full testcase (not the cut-and-pasted one from this
bug's comments section).  This is real, it's a bogus assertion and possibly
other trouble due to JS_ARENA_GROW_CAST's roundup of incr (and the rounded by
definition) _a->avail) vs. the unrounded size + incr.  The rounded-up byte count
can exceed the arena size, while the unrounded sum does not.  Patch soon, plus
diagnosis of whether this is more than a bogus assertion.

/be

Comment 41

[Brendan Eich [:brendan]]

Of course, this is a different bug, but I'm easy about morphing a report to
track follow-on fixes for the same testcase.

/be

Comment 42

[Brendan Eich [:brendan]]

Ok, the patch I'm about to attach does the right obvious thing: it computes the
gross arena allocation size (rounded up to keep a->avail aligned per pool->mask)
by summing the net current size and growth increment, *then* rounding.  The bad
old code effectively rounded both size and incr, then summed.  Looking for r=
and sr= here and now.

/be

Comment 43

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 44

[Phil Schwartau]

I was curious to duplicate Brendan's experience, so I copied the
block of code at 2001-02-26 19:55 above and saved it as '44009.js'.


[/d/JS_TRUNK/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js
js> load('44009.js') 

      (no crash)


[/d/JS_TRUNK/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js
js> load('../../tests/js1_5/shell.js')
js> load('44009.js')

      (CRASH)



But why is that? 44009.js is a subset of the complete testcase, with all 
references to the utility functions in shell.js stripped out.


So what would loading the one file have to do with loading the other?

Comment 45

[Brendan Eich [:brendan]]

This latest assertbotch, which indicates a rounding bug that would sometimes
waste a bit of space (but do no worse) in optimized builds, depends on how space
is allocated and grown using a JSArenaPool.  Until the GC runs on a context,
arena pools for that cx are not "finished" -- they hang onto their arenas.

So if you run the shell with -f 44009.js alone (or type load, more or less
equivalently), the arena pools start allocating arenas from the malloc heap.  If
you run -f ../tests/js1_5/shell.js -f 44009.js, the shell "warms up" the arena
pools for the context used to evaluate both files, and 44009.js reuses pooled
arenas.  If one of those arenas happens to have just the right size, and holds a
pointer to an allocation that's GROWn, the assertion will botch.

/be

Comment 46

[Mike Shaver (:shaver emeritus)]

I like it.  r=shaver.

Comment 47

[Brendan Eich [:brendan]]

Upping Priority for checkin today.

/be

Comment 48

[John Bandhauer]

sr=jband I'm surprised you didn't apply your local-underscore-move-to-trailing 
campaign here :)

Comment 49

[Brendan Eich [:brendan]]

Minimal change, for reviewer sanity.  I may do a clean _foo to foo_ sweep later,
before 1.5 final.

/be

Comment 50

[Phil Schwartau]

Verified FIXED on WinNT and Linux. This HTML no longer crashes:

<SCRIPT>
alert(window.toSource());
</SCRIPT>


And running this test with the test driver no longer crashes: 

js/tests/js1_5/Regress/regress-44009.js