190813 - Browser hangs indefinitely on above URL

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P4)

Description

[Brad Garcia]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030125
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030125

When I attempt to go to the above URL, the browser hangs.
I am not sure if it's the browser itself, or a plugin.

Reproducible: Always

Steps to Reproduce:
1. go to http://www.psecu.com

Actual Results:  
Browser hangs and needs to be killed.

Expected Results:  
Page should have been shown.

linux x86 2003-01-25-22

Comment 1

[WD]

WFM Win2k 2003012607

There is some javascript on the PSECU website which runs pretty slow.   If you
have a slow machine, it may seem like mozilla is hung.    

Reporter: if you leave mozilla open on the PSECU website for a while, does it
ever come back responsive?

Comment 2

[Brad Garcia]

I waited about 15 minutes last time before killing it.
"top" shows no CPU being used at the time by mozilla.
This was on a Pentium III 600MHz machine.

Comment 3

[Gene Stuckey]

I have the same problem.  I also see the same thing on another site that is not
publicly accessible.

Solaris8 2003012622

Comment 4

[R.K.Aa.]

i see the hang as described. Waited two minutes on a P4/1.8. Low or no CPU usage.
2003012711 Linux trunk

Comment 5

[Andrew Hagen]

Confirming on 2003012709, Windows 98. Loaded the first page fine, but it was
very slow to display. Then I clicked on one of the links through the dynamic
menus, and it locked up completely. No error message. Hard reset required.

Not sure what component this would go under. Maybe DOM Level 0?

Comment 6

[Andrew Schultz]

Attachment: testcase JS

this is the meat of the testcase, but actually load the second attachment.

The javascript here takes an huge string and search/replaces control characters
with short strings (a contorted method to compress html?).  Anyway, the hang
seems to be related to the size of the the string and/or the number of things
it has to search/replace.

The original javascript is here:
http://www.psecu.com/menu/iemenu_frames.js

Comment 7

[Andrew Schultz]

Attachment: testcase

ok, ignore the first attachment, except to read the info about the bug...
This is the HTML+JS.

Comment 8

[Andrew Schultz]

Attachment: stacktrace during hang

Assertion failure: _PT_PTHREAD_MUTEX_IS_LOCKED(lock->mutex), at ptsynch.c:207

Comment 9

[Andrew Schultz]

the testcase regressed between linux 2003010705 and 2003010805.  the regression
window and the stack trace implicate bug 186592.
==> Javascript Engine

Also, the "correct" behavior for the testcase is to display a blank page...
buggy behavior is to immeadiately hang.

Comment 10

[Brad Garcia]

windows 2003-01-20-08

It takes a few seconds to load, but doesn't hang.
So this appears to be linux- or unix-specific.

Comment 11

[David Bradley]

On Windows XP, I get an assert in JS_GC during the call to PR_Unlock, called by
    GC_CALLBACK(JSGC_MARK_END) macro complaining that we don't own the lock
we're unlocking. lock->owner is 0x0, and lock->iLock.mutex.LockCount is
0xffffffff. So some how js_GC is unlocking the JS runtime lock twice

Comment 12

[David Bradley]

Attachment: Changed GC_CALLBACK to not unlock/lock when GC is running

Looks to me like the GC_CALLBACK macro doesn't take into account where it's
being called. The first two places we don't need to unlock, since we've already
done that above. The last place we do. I'm not sure I really like this patch. I
thought about removing the macro all together, but I think the code is cleaner
with it. In any case, Roger or another JS peer can make the final decision.

Also, it looks like this might affect bug 180727

Comment 13

[David Bradley]

Attachment: Smaller alternative patch

This is simpler. The only issue would be that it ties the concept of "is GC
Running" to mean any status but JSGC_END. If a new status like
JSGC_JUST_BEFORE_END  was added the macro would need to include that as well. 
Thanks to timeless for recognizing this.

Comment 14

[Phil Schwartau]

Using WinNT4.0, 500MHz CPU, 128M RAM
When I tried the testcase in Comment #7:

IE6: seems to hang, but my RAM is not all used up
Moz: seems to hang, and uses up all available RAM

With Moz, I get the Windows warning: "Your system is running low on memory...",
and the box comes close to crashing before I can shut Moz down -

Comment 15

[Phil Schwartau]

Since this may be related to bug 186592, let me reassign this one
to Brendan for consideration of David's patch -

Comment 16

[rogerl (gone)]

Comment on attachment 112891 [details] [diff] [review]
Smaller alternative patch

r=rogerl, but I'm not really  savvy on GC, I'd be happier if one of
shaver/brendan/jband had looked at this

Comment 17

[Mike Shaver (:shaver emeritus)]

Comment on attachment 112891 [details] [diff] [review]
Smaller alternative patch

I bet than Brendan will have a more elegant way to solve this, but this seems
correct and minimally invasive.  I'll stick my neck out a bit. r=shaver.

Comment 18

[David Bradley]

Comment on attachment 112891 [details] [diff] [review]
Smaller alternative patch

Think this should get into 1.3b since it was a recent regression from bug
186592.

Comment 19

[David Bradley]

Fix checked in.

Comment 20

[Brendan Eich [:brendan]]

The fix in attachment 112891 [details] [diff] [review] isn't right -- when js_GC is called with
GC_ALREADY_LOCKED among gcflags, the lock will be held throughout the GC run,
and the callback (if any) must be done only after temporarily releasing the
lock.  The observed hang is due to a goto restart, I believe, which unlocks the
GC whether or not GC_ALREADY_LOCKED, leaving us in a bad state, re-running the
GC without the lock, yet with GC_ALREADY_LOCKED still set (due to the last-ditch
case in js_AllocGCThing being hit) in gcflags.

Patch in a second.

/be

Comment 21

[Brendan Eich [:brendan]]

Wait, I'm wrong -- the GC lock is released always, just 14 lines before the
restart: label.  So dbradley's patch is right, and there's no problem with
racing allocators depleting the just-replenished heap in a last-ditch scenario
(what am I on about?  See bug 162779).

But I prefer to eliminate the macro, as dbradley did, for the same reason.  I'll
patch for 1.4a.

/be

Comment 22

[Brendan Eich [:brendan]]

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

Comment 23

[Brendan Eich [:brendan]]

Attachment: cleanup patch

Comment 24

[Mike Shaver (:shaver emeritus)]

Comment on attachment 113725 [details] [diff] [review]
cleanup patch

Even better.

Comment 25

[Brendan Eich [:brendan]]

The hang was fixed, this is just a cleanup bug now.

/be

Comment 26

[Brendan Eich [:brendan]]

Patch checked into the 1.4a trunk.

/be

Comment 27

[Phil Schwartau]

Checkin verified -

Can contributors confirm that they no longer hang at http://www.psecu.com/? 
I'm no longer having problems with it (although it does load slowly).

But I'd like to hear from other people; thanks -

Comment 28

[Brad Garcia]

linux x86 2003-02-24-08

Yep, it loads now!  Yep, it's pretty slow.

Thanks!