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

VERIFIED FIXED in mozilla1.8beta2

Status

()

P1
normal
VERIFIED FIXED
17 years ago
12 years ago

People

(Reporter: mbmarduk, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla1.8beta2
js1.5
Points:
---
Bug Flags:
blocking1.8b2 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

17 years ago
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

17 years ago
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 (?)
Assignee: attinasi → rogerl
Component: Layout → JavaScript Engine
QA Contact: petersen → pschwartau

Comment 2

17 years ago
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

Updated

16 years ago
Summary: (Most of) page content is not shown → (Most of) page content is not shown - "Error: too many literals"

Comment 3

15 years ago
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

Comment 4

15 years ago
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 → ---
(Assignee)

Comment 5

15 years ago
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

15 years ago
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".
(Assignee)

Comment 7

15 years ago
As promised.

/be
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → WONTFIX

Comment 8

14 years ago
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?
(Assignee)

Comment 9

14 years ago
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)

Updated

14 years ago
Assignee: khanson → brendan
Status: REOPENED → NEW
Keywords: js1.5
Priority: -- → P2
Target Milestone: --- → mozilla1.8beta
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

14 years ago
Target Milestone: mozilla1.8beta1 → mozilla1.8beta2

Comment 10

14 years ago
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
(Assignee)

Updated

14 years ago
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"
(Assignee)

Comment 11

14 years ago
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 13

14 years ago
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
(Assignee)

Comment 14

14 years ago
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+
(Assignee)

Comment 15

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

Big patch next.

/be
(Assignee)

Comment 16

14 years ago
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
(Assignee)

Updated

14 years ago
Attachment #179014 - Flags: review?(shaver)
(Assignee)

Comment 17

14 years ago
Bob, can you beat on this patch too?  Thanks,

/be

Comment 18

14 years ago
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

14 years ago
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?
(Assignee)

Updated

14 years ago
Attachment #179014 - Attachment is obsolete: true
Attachment #179014 - Flags: review?(shaver)
(Assignee)

Comment 20

14 years ago
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
(Assignee)

Updated

14 years ago
Attachment #179188 - Flags: review?(shaver)

Comment 21

14 years ago
Is this worth trying to get in 1.8b2?
(Assignee)

Comment 22

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

/be
Flags: blocking1.8b2+
(Assignee)

Comment 23

14 years ago
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+
(Assignee)

Comment 25

14 years ago
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 26

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

a=asa
Attachment #179188 - Flags: approval1.8b2? → approval1.8b2+
(Assignee)

Comment 27

14 years ago
Fixed.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Flags: testcase+

Comment 28

13 years ago
verified fixed 1.8.x and trunk.
Status: RESOLVED → VERIFIED

Updated

12 years ago
Depends on: 363988
You need to log in before you can comment on or make changes to this bug.