155081 - (Most of) page is not shown - "Error: too many literals"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Mike Neman]

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

Comment 1

[Andrew Schultz]

the page has a JS function, "topic" which writes a table row. it calls that
function over 30,000 times.  The number of calls seems to be the problem, rather
than any one call.

In the Javascript console, it says "Error: too many literals", which is
generated at jsatom.c, line 852.

marking NEW
==> Javascript Engine (?)

Comment 2

[Phil Schwartau]

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.

Comment 3

[Stef van der Made]

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

Comment 4

[Andrew Schultz]

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.

Comment 5

[Brendan Eich [:brendan]]

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

Comment 6

[Andrew Schultz]

Attachment: "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".

Comment 7

[Brendan Eich [:brendan]]

As promised.

/be

Comment 8

[Jeremy Hannon]

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?

Comment 9

[Brendan Eich [:brendan]]

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

Comment 10

[Bob Clary [:bc] (inactive)]

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.

Comment 11

[Brendan Eich [:brendan]]

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

Comment 12

[Bob Clary [:bc] (inactive)]

like this? 
http://bclary.com/2004/10/03/js-tests/js1_5/Regress/regress-155081-2.js

Comment 13

[Bob Clary [:bc] (inactive)]

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

Comment 14

[Brendan Eich [:brendan]]

Attachment: fix the crash noted by bclary, but not the 64K literal limit

I'm checking this in now.  Full fix coming soon.

/be

Comment 15

[Brendan Eich [:brendan]]

Forgot to check that little crash-fix patch in -- done.

Big patch next.

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: 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

Comment 17

[Brendan Eich [:brendan]]

Bob, can you beat on this patch too?  Thanks,

/be

Comment 18

[Bob Clary [:bc] (inactive)]

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

Comment 19

[Bob Clary [:bc] (inactive)]

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?

Comment 20

[Brendan Eich [:brendan]]

Attachment: 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

Comment 21

[Bob Clary [:bc] (inactive)]

Is this worth trying to get in 1.8b2?

Comment 22

[Brendan Eich [:brendan]]

Yes, this should go into 1.8b2.  Just waiting for shaver to review.

/be

Comment 23

[Brendan Eich [:brendan]]

Shaver's gonna hit a cafe in Ottawa soon, I predict :-P.

/be

Comment 24

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

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

Comment 25

[Brendan Eich [:brendan]]

Comment on attachment 179188 [details] [diff] [review]
proposed fix, v2

This should go in for 1.8b2 to get the most soak time.

/be

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 179188 [details] [diff] [review]
proposed fix, v2

a=asa

Comment 27

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 28

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.x and trunk.