From Bugzilla Helper: User-Agent: Mozilla/4.7 [en] (Win95; U) BuildID: 20000512 The above URL is an attachment to bug 34571 - a JS performance bug. Any of the bottom three tests on the left cause a crash: MOZILLA caused an invalid page fault in module JS3250.DLL at 014f:60af171a. Registers: EAX=0c74a690 CS=014f EIP=60af171a EFLGS=00010246 EBX=0c74a690 SS=0157 ESP=0068edf4 EBP=0068ee00 ECX=0068ee88 DS=0157 ESI=00000000 FS=1f1f EDX=00000000 ES=0157 EDI=00000000 GS=0000 Bytes at CS:EIP: f6 46 18 01 75 67 8b 46 1c 8b 04 85 dc e8 b1 60 Stack dump: 00000000 0089eea0 0c74a690 0068ee1c 60ae7203 0089eea0 0c74a690 00000000 00000000 00000000 0068ee64 60ae71b0 0089eea0 0c74a690 00000000 00000000 This regressed between 20000509 and 20000511. Gerv

Adding keywords. Gerv

Reproduced on PC/Linux with build 2000060308. All three testcases (var + something) are crashing with the same stack trace: #0 0x40107037 in js_ErrorToException () from libmozjs.so #1 0x400f80a0 in js_ContextIterator () from libmozjs.so #2 0x400f81a1 in js_ReportErrorVA () from libmozjs.so #3 0x400f3bc9 in JS_ReportError () from libmozjs.so #4 0x400f3cd7 in JS_ReportOutOfMemory () from libmozjs.so #5 0x40109b8c in js_AllocGCThing () from libmozjs.so #6 0x4011a174 in js_NewObject () from libmozjs.so #7 0x400f177c in JS_NewObject () from libmozjs.so #8 0x4040c3f8 in NS_NewScriptKeyEvent () from libjsdom.so #9 0x404086ee in nsJSEventListener::HandleEvent () from libjsdom.so Setting OS to All, adding name of crashing function to summary.

Thanks to Morten Wang (warnckew@online.no) for writing the above testcases. I have run them all within the JavaScript shell itself (i.e. not within the browser embedding). I pulled and built the JS shell on 06/01/00. The results are below. The tests are all loops and are patterned like this one for "two small strings" : function runTest() { start = new Date(); for(i = 0; i < UBound; i++) { x = "Small " + "string"; } end = new Date(); time = end.getTime() - start.getTime(); } All times are in milliseconds. Note that the time growth is roughly linear as we increase the number of iterations of each loop. We start getting out-of-memory errors at 10^6 iterations in the (var + something) testcases: Concatenation 10^3 iterations 10^4 10^5 10^6 two small strings 0 16 250 2453 three small strings 0 16 250 2453 four small strings 0 16 250 2453 five small strings 0 16 250 2453 two large strings 0 16 250 2453 three large strings 0 16 250 2469 two strings and a variable 0 63 688 out of memory two strings and two variables 0 94 969 out of memory two variables 0 47 469 out of memory

This bug depends on 40757, which must be fixed for js1.5 customers. 40757 in part involves the lack of a safe GC point within js_AllocGCThing, which is why I removed the "last-ditch" attempt to call js_GC from that routine when JS hits a per-runtime or malloc-failure memory limit. The JS testsuite depends on such a last-ditch attempt, and I'm going to fix 40757 in full, which will have the good effect of fixing this bug. Rather than DUP this against 40757, I'll reassign to myself. There's a related bug that rogerl still owns (avoiding error-to-exception object creation when out of memory). /be

Same patch as for 44376 coming up. /be

Oops, typo: the patch also fixes bug 44376. /be

Someone (anyone?) gimme an r=, ok? /be

Reviewing... -#ifdef TOO_MUCH_GC - atom->flags = ATOM_PINNED; -#else atom->flags = 0; -#endif Given that TOO_MUCH_GC is gone here, does it make sense to retain it in jsgc.c? What's the history of this define? + /* + * Don't scan beyond the current context's top of stack, + * because we may be nesting a GC from within a call to + * js_AllocGCThing, originating from a conversion call + * from js_Interpret() with local variables holding the + * only references to other, unrooted GC-things (e.g., + * a newly-converted-to-object operand that was just + * popped off the stack). + */ I'm not sure I understand this... Does it still do the right thing when we're not in the js_AllocGCThing case? Mike

> Given that TOO_MUCH_GC is gone here, does it make sense to retain it in > jsgc.c? What's the history of this define? I created it, it used to work (or seemed to, ahem), then it broke for sure when atoms became GC'd rather than ref-counted. I fixed it recently by pinning all atoms when running with TOO_MUCH_GC defined (only masochistic hackers would do that, so no problem bloating due to pinned atoms). A better fix is in the patch: don't sweep atoms when activating the GC on a context that's holding unrooted atoms (the compiler and API entry points atomize without rooting for a while; eventually, the compiler makes a script and roots it via its function object, or returns it to the API caller, who must root it via JS_NewScriptObject). An even better fix would be incremental GC and local roots for atoms as well as for newborns, but that's addressed by bug 40757. > I'm not sure I understand this... Does it still do the right thing when we're > not in the js_AllocGCThing case? It means that we may fail to collect GC-things referenced only by freed stack space above the current (homed in fp->sp) stack pointer, but below the avail cursor of the top arena containing the sp. We'll collect such garbage sooner or later, as the stack pulsates and contexts come and go, so it's not a leak, but it is a step back from exactness, toward conservative GC. Best that can be done for now, I think -- bug 40757 is the report to track a better exact/incremental solution. Sorry my comment wording was unclear, I actually revised it again after posting that last patch. Here's the latest: /* * Don't scan beyond the current context's top of stack, * because we may be nesting a GC from within a call to * js_AllocGCThing originating from a conversion call made * by js_Interpret with local variables holding the only * references to other, unrooted GC-things (e.g., a non- * newborn object that was just popped off the stack). */ I should add a paragraph noting that we may miss garbage temporarily, and how that's not exact GC. I'll do that and try to make the verbiage even clearer. Your comments are welcome. /be

In case it isn't obvious from my comments, I think bug 40757 may not be fixed before js1.5 is really done. For now, 40757 is keyworded js1.5 in the hope that someone finds time to address that bug. But this bug is sufficient to restore GC safety, imperfectly. mccabe, thanks again for the review. /be

Fix checked in. /be

Marking as verified - on both Linux and WinNT

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