108.85 KB, application/x-bzip2
1.19 KB, patch
|Details | Diff | Splinter Review|
82.48 KB, patch
|Details | Diff | Splinter Review|
From Bugzilla Helper: User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0) Gecko/20020629 BuildID: 2002062907 This page is a forumpage called "Programming & webscripting". It is set to "show all topics" (which goes back to about december 2000). As you can see when you go there; it only shows the header and footer of the page but no contents whatsoever (takes 24 seconds). It DOES work fine for other pages on the same forum which are set to "show all topics". If instead you choose e.g. "Non-Windows Operating Systems" from the "Selecteer forum" drop-down menu and then select "Alle topics laten zien" from the adjacent "Oudere topics" drop-down menu you'll see it works fine (go grab some coffee at this point 'cuz on my Duron 900+DDR system it takes 4-5 minutes to render the page ;) It doesn't matter if you're logged-in or not. Opera 5 and Netscape 4.77 both crash and burn after making my CPU load 100% for a couple of minutes. Moz atleast hangs in there :) This bug is very similar to bug #105952 and more-or-less so to #152745 and #138821 Reproducible: Always Steps to Reproduce: 1.Go to the page 2.Follow above directions to compare to other pages
Assignee: attinasi → rogerl
QA Contact: petersen → pschwartau
Confirming bug; cc'ing Brendan, shaver on this one; is this a valid issue? Note the problems noted in other browsers by the reporter above. IE6 also does not load this page in any reasonable amount of time.
Assignee: rogerl → khanson
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: (Most of) page content is not shown → (Most of) page content is not shown - "Error: too many literals"
Page refered in bug no longer exists as backend software has been replaced with a new version. So reproducing this bug is not possible at this moment Bug is not touched for over 1 year closing
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → INVALID
reopening. this bug is still valid. I have the HTML page saved, and I'm whittling it down to a reduced testcase. I'll attach it shortly.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Beware: I'm going to WONTFIX this. Any script that thinks it needs > 64K literals deserves what it gets. Convince me I'm wrong, make me care. ;-) /be
Created attachment 140484 [details] "reduced" testcase the idea here is that there are > 65536 unique strings. the original page had nearly 100k literals. with linux trunk 20040201, JS Console says "Error: too many literals".
As promised. /be
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago → 15 years ago
Resolution: --- → WONTFIX
Other people have been experiencing this problem as well ( http://forums.mozillazine.org/viewtopic.php?t=212144 ). I think the current limit is reasonable if we are only talking normal websites, but XUL applications -- and some web applications -- may need more. I am not a guru on the codebase, but how hard would this be to change from a 16-bit int to a 32-bit int?
Changing all literal indexes from 16 to 32 bits would bloat bytecode unnecessarily. Adding an extended bytecode format is possible. Reopening due to real-world cases cropping up, which work in IE (how galling is that? ;-). /be
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee: khanson → brendan
Status: REOPENED → NEW
Priority: -- → P2
Target Milestone: --- → mozilla1.8beta
Target Milestone: mozilla1.8beta1 → mozilla1.8beta2
Andrew, I checked in a version of your reduced testcase as js/tests/js1_5/Regress/regress-155081.js. This crashes Mozilla and the js shell for builds from this morning.
QA Contact: pschwartau → moz
OS: Linux → All
Priority: P2 → P1
Hardware: PC → All
Summary: (Most of) page content is not shown - "Error: too many literals" → (Most of) page is not shown - "Error: too many literals"
bc: there are two bugs here. The crash bug on the trunk is due to backpatch delta overflow. Fixing that is entangled with fixing the 16-bit unsigned atom index limit, so I don't need a separate bug, but we need two testcases. If you remove the try/catch and just use straight-line code, you'll isolate the 16-bit atom index problem. /be
js/tests/js1_5/Regress/regress-155081-2.js checked in. Also tweaked regress-155081.js Checking in regress-155081.js; /cvsroot/mozilla/js/tests/js1_5/Regress/regress-155081.js,v <-- regress-155081.js new revision: 1.2; previous revision: 1.1 done
Created attachment 177253 [details] [diff] [review] fix the crash noted by bclary, but not the 64K literal limit I'm checking this in now. Full fix coming soon. /be
Attachment #177253 - Flags: review+
Forgot to check that little crash-fix patch in -- done. Big patch next. /be
Created attachment 179014 [details] [diff] [review] proposed fix The money shot, so to speak, is this comment in jsemit.c: + * Emit a bytecode and its 2-byte constant (atom) index immediate operand. + * If the atomIndex requires more than 2 bytes, emit a prefix op whose 24-bit + * immediate operand indexes the atom in script->atomMap. + * + * If op has JOF_NAME mode, emit JSOP_FINDNAME to find and push the object in + * the scope chain in which the literal name was found, followed by the name + * as a string. This enables us to use the JOF_ELEM counterpart to op. + * + * Otherwise, if op has JOF_PROP mode, emit JSOP_LITERAL before op, to push + * the atom's value key. For JOF_PROP ops, the object being operated on has + * already been pushed, and JSOP_LITERAL will push the id, leaving the stack + * in the proper state for a JOF_ELEM counterpart. + * + * Otherwise, emit JSOP_LITOPX to push the atom index, then perform a special + * dispatch on op, but getting op's atom index from the stack instead of from + * an unsigned 16-bit immediate operand. Note how the last approach, taken for opcodes that lack a JOF_ELEM counterpart, conserves code in jsinterp.c and jsopcode.c (look for BEGIN_LITOPX_CASE and END_LITOPX_CASE macros). This needs exhaustive testing via a travesty generator. More on that soon. /be
Bob, can you beat on this patch too? Thanks, /be
opt: passes js1_5/Regress/regress-155081.js, js1_5/Regress/regress-155081-2.js debug: fails/asserts js1_5/Regress/regress-155081.js, passes js1_5/Regress/regress-155081-2.js EmitAtomIndexOp(JSContext * 0x00037058, int 130, unsigned long 70012, JSCodeGenerator * 0x0013ed3c) line 1662 + 28 bytes default: JS_ASSERT(mode == 0); break; with mode == 32
I ran a 70 pages with and without this patch using a debug build. I didn't notice any overt problems but did notice a large increase in the mAllocCount - mFreeCount in the debug console output. It may be due to time based differences between the pages, but the difference increased from 207 in the unpatched build to 2892 in the patched build. Is there anyway this patch could be responsible?
Created attachment 179188 [details] [diff] [review] proposed fix, v2 I removed JOF_PROP from JSOP_INITCATCHVAR's format, it was miscueing that assertion by implying that JSOP_INITCATCHVAR had a JOF_ELEM counterpart. It does not, and there's no other reason to give it JOF_PROP mode, so its format is now simply JOF_CONST. No bogus assertion. Talked to bc, it doesn't sound like those nsString allocation stats can be affected by this patch. /be
Attachment #179188 - Flags: review?(shaver)
Is this worth trying to get in 1.8b2?
Yes, this should go into 1.8b2. Just waiting for shaver to review. /be
Shaver's gonna hit a cafe in Ottawa soon, I predict :-P. /be
Comment on attachment 179188 [details] [diff] [review] proposed fix, v2 No cafe, but I like what I see here. Even the decompiler parts made sense! r=shaver
Attachment #179188 - Flags: review?(shaver) → review+
Comment on attachment 179188 [details] [diff] [review] proposed fix, v2 This should go in for 1.8b2 to get the most soak time. /be
Attachment #179188 - Flags: approval1.8b2?
Comment on attachment 179188 [details] [diff] [review] proposed fix, v2 a=asa
Attachment #179188 - Flags: approval1.8b2? → approval1.8b2+
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago → 14 years ago
Resolution: --- → FIXED
verified fixed 1.8.x and trunk.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.