126466 - Endless webshells created until out of memory hang or crash

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Yuying Long]

Build: 02-18 trunk build

By loading the page: http://www.ccidnet.com - this is a very popular Simplified
Chinese web site, when load the page there will be a pop-up window, the machine
CPU will up to 100% and then browser will hang. (See some comments in bug 126264).

IE doesn't has same problem.

Comment 1

[Yuying Long]

Please reassign in case the contact persons are wrong.

Comment 2

[Phil Schwartau]

Setting default owner and QA -

Comment 3

[Phil Schwartau]

This bug evolved out of bug 126264. Here are the findings made there:


------- Additional Comment #10 From Phil Schwartau 2002-02-19 11:27 -------

OK, the problem at that website is pretty bad. In the debug console,
I see hundreds and hundreds of "WEBSHELL"'s being created. If I had
let it continue, it would have gone over 1,000. My CPU does not go 
up to 100%, but the load just keeps going and going and eventually
uses up all my virtual memory -

I will attach a WinNT stack trace of manual interrupts I did
during page load.



------- Additional Comment #11 From Phil Schwartau 2002-02-19 11:29 -------

Created an attachment (id=70349)
WinNT stack trace of manual interrupts during page load



------- Additional Comment #12 From Yuying Long 2002-02-19 11:34 -------

Just for info.: IE will never has the problem that we had with those
kind of pages.



------- Additional Comment #13 From Phil Schwartau 2002-02-19 11:39 -------

We have to decide what to do:

1. Morph this bug into another one for the new site?
2. Close this one as WORKSFORME and open a new one for the new site?

I can't see any Plug-ins at the new site. In IE6 we see a little 
floating box that moves all over the page. I'll bet that's the 
problem. Somehow Mozilla is trying to create a new webshell for each
new position of the floating box? Just a guess. 

The JS file for it is:

  view-source:http://www.ccidnet.com/image/java/ccidnet_floating01.js

document.write("<DIV ID='fly' 
STYLE='position:absolute;width:80;height:80;visibility: visible'>
<iframe width=80 height=80 noresize scrolling=No frameborder=0
marginheight=0 marginwidth=0 
src=http://www.cnnet.com.cn/servlets/ad?Pool=home_floating>
</iframe>
</DIV>
<script SRC='http://www.ccidnet.com/image/java/ccidnet_floating.js'>
</script>");



------- Additional Comment #14 From Phil Schwartau 2002-02-19 11:43 -------

Created an attachment (id=70355)
ccidnet_floating.js



------- Additional Comment #15 From Phil Schwartau 2002-02-19 11:49 -------

Suspect function:

function movechip(chipname)

Suspect line:

   chip.timer1=setTimeout("movechip('"+chip.named+"')",140);


This is neither JS Engine nor Plug-ins. Whether we keep this bug
open, or file a new one, we need to find out: why does Mozilla create
more than a thousand webshells when loading http://www.ccidnet.com ? 

Perhaps Embedding:Docshell component could tell ???



------- Additional Comment #16 From Phil Schwartau 2002-02-19 11:52 -------

I would vote for closing this bug as WORKSFORME and opening a new one.
That way, in case the problem comes back  at the original site (i.e.
the site changes its HTML back to what it was), we can reopen this bug
and try to solve the problem.

What does everybody think?



------- Additional Comment #17 From Yuying Long 2002-02-19 11:56 -------

I agree to open another bug(JavaScript?) for the new web site and mark this one
here for works for me now.



------- Additional Comment #18 From Phil Schwartau 2002-02-19 12:00 -------

OK, note this also:

There's another suspect file at the site:

   view-source:http://www.ccidnet.com/image/java/ccidnet_xl.js

Just look at this time-bomb inside the file:


function MM_reloadPage(init) {  //reloads the window if Nav4 resized
  if (init==true) with (navigator) {
    if ((appName=="Netscape")&&(parseInt(appVersion)==4)) {
      document.MM_pgW=innerWidth;
      document.MM_pgH=innerHeight;
      onresize=MM_reloadPage; 
    }
  }
  else if (innerWidth!=document.MM_pgW || innerHeight!=document.MM_pgH)     
location.reload();
}

MM_reloadPage(true)


Might this be causing an infinite loop of page loads - ? 



------- Additional Comment #19 From Phil Schwartau 2002-02-19 12:03 -------

Yuying: the new bug should not be opened against JavaScript.
There is nothing going wrong in the JavaScript Engine here.

Based on what I have found, I would assign it to Embedding:Docshell
to see if they can find out why so many webshells are being generated. 

Comment 4

[Peter Lubczynski]

Bug 112491 has the same problem, endless webshells till hang or crash:
http://www.bombardier.com/de/1_0/1_1/1_1_0.html

...copying over keywords from other bug

Rephrasing summary from: Loading the page above will cause browser hang

Comment 5

[Peter Lubczynski]

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

Comment 6

[Adam Lock]

This has to be the most evil webpage I've ever seen!

 Can someone tell me if associative arrays like this are meant to work?

  document.images["but"+i].src=butover[i].src;

Or referencing elements by their id, like ccidnet_xl.js does for layer called
"dangdang" it writes into the document:

  document.dangdang.top=pageYOffset+window.innerHeight-imgheight

Comment 7

[Adam Lock]

Argh this site is horrible!! After much searching (battling broken sniffers,
iframes inside iframes inside iframes, document.writes everywhere, zillions of
adverts, timers & codepages), I think it boils down to this URL here:

http://www.ccidnet.com/road.html

It generates a random number between 1 & 4 and stuffs in up to 4 different
iframes adverts depending on the value.

To take one example:

http://www.cnnet.com.cn/servlets/ad?Pool=home_bur

Here is the source for that URL:

<!-- begin dynamic banner insert  -->
<!-- ad management by Ad Juggler 4.0 http://www.aj4.com -->
<script LANGUAGE=JavaScript> 
<!-- 
document.write('<iframe width=0 height=0 noresize scrolling=No frameborder=0
marginheight=0 marginwidth=0 src="http://www.ccidnet.com/road.html"></iframe>'); 
//--> 
</SCRIPT>
<embed src="http://www.ccidnet.com/image/468.swf" quality=high pluginspage="
http://www.macromedia.com/shockwave/download/index.cgi?P1_Prod_Version=
ShockwaveFlash" type="application/x-shockwave-flash" width="468" 
height="60"></embed>
<!-- Copyright 1999-2000 ThruPort Technologies http://www.thruport.com -->
<!-- end dynamic banner insert -->


So the first thing the advert does is run JS to insert another IFRAME pointing
to road.html!

In other words this is an recursive loop of IFRAMES. road.html inserts an IFRAME
containing an advert which inserts an IFRAME containing road.html and so on.
This is very disgusting code and I have no idea why it works in IE. Perhaps IE
has some kind of limiter on how many IFRAMEs it will allow before quitting.
I would be tempted to mark this WONTFIX, but I will wait for peoples opinion.

It would be nice to be able to cap the number of nested IFRAMEs to prevent this
abuse and for security reasons but I don't know how easy it would be to add. If
someone knows of a non-intrusive way to implement this please say so.

Comment 8

[Phil Schwartau]

cc'ing jst, jkeiser, Boris for their opinions on this -

Comment 9

[Johnny Stenback (:jst)]

There's a cap already in mozilla for how deep frames/iframes can nest, IIRC it's 25.

Comment 10

[Adam Lock]

I found the code and bug 8065.

http://lxr.mozilla.org/seamonkey/source/content/base/src/nsFrameLoader.cpp#295

Comment 11

[Adam Lock]

Correction:

http://lxr.mozilla.org/seamonkey/source/layout/html/document/src/nsFrameFrame.cpp#972

The other code is commented out.

I'm working on a patch which lowers the limit. 25 seems pretty high.

Comment 12

[Adam Lock]

Attachment: Patch reduces maximum limit for nested frames

This patch slashes MAX_DEPTH_CONTENT_FRAMES down from 25 to 8 and adds a
warning when the limit is reached.

With the limit in place, http://www.ccidnet.com/ just loads with 490 webshells
and 100Mb memory consumption!

8 still seems a more than reasonable restriction on the number of nested
*content* frames. If people feel it is too high or too low I can adjust it, but
bear in mind that this sample page will not load with any higher number. It may
not even load at 8 on some operating systems with less system resources, such
as Win95/98/Me & Mac OS 9 etc.

Reviews please.

Comment 13

[Johnny Stenback (:jst)]

This code has been moving around a few times lately (due to bug 52334 not
sticking to the tree). When this is checked in, we should make this change
either in nsFrameLoader.cpp or nsFrameFrame.cpp, depending on what code is used
at the time.

sr=jst for this change to either file.

Comment 14

[Adam Lock]

Looking for an r= on the patch please.

Comment 15

[Fabian Guisset]

Comment on attachment 77835 [details] [diff] [review]
Patch reduces maximum limit for nested frames

r=fabian

Comment 16

[Adam Lock]

Nominate for adt1.0.0

Risk summary - low to moderate. Patch is simple and obvious, but there could be
a website out there that needs 8 nested frames which won't work now.

Justification for 1.0 - stops runaway sites earlier. Sites such as
http://www.ccidnet.com that recursively create frames are less likely to take
down Mozilla than with the old limit.

Comment 17

[Johnny Stenback (:jst)]

Actually, the real fix for thsi would be to add proper recursion detection code
to the [i]frame loading code instead of limiting the nesting depth. Doing both
is probably the ideal solution. Do we want to give that a shot for mozilla1.0?
Shouldn't be hard to code up...

Comment 18

[Adam Lock]

How would you see this code working?

Is it ok to modify nsHTMLFrameInnerFrame to count how many parents use the same
URI and halt after some arbitrary limit, e.g. 3? This might stop recursion
sooner. Obviously comparing URIs isn't the ideal solution, but it's the only way
to catch this kind of site where road.html is generating random content each time.

Comment 19

[Johnny Stenback (:jst)]

IMO if a frame ends up including itself (directly or indirectly) we should stop
it right there. I don't know what IE does, but that's the only thing that makes
sense to me. Comparing URI's seems like the obvious way to detect this, or do we
have other better ways?

Comment 20

[Jaime Rodriguez, Jr.]

To recieve ADT approval, pls let us know, if the discussion between adam and jst
needs to be resolved, or is this patch ready to go as is?

Comment 21

[John Keiser (jkeiser)]

Jaime: the problems are orthogonal.  Adam's fix can go in the tree IMO.

JST: comparing URIs will work.  One legit problem is when there is a form POST
that results in a page with iframe or frame, and the URI is the same as one of
the parents.  That is some really strange code though.  However ... if the
reason we want to stop the recursion is malicious intent, any ol' coder can get
around it by making a simple script at http://www.evilcode.com that loads two
frames with http://www.evilcode.com/script?1, that each load two frames with
http://www.evilcode.com/script?2, and so on, and so on ... simple programming
hack.  Adam's code will stop that though.  Adam's code will also stop accidental
recursion so that a webmaster can fix their site.

Adam: I personally think 8 is fine.  Only insane people nest 8 frames.  And we
all know there are no insane people writing web pages.

Comment 22

[Adam Lock]

Jaime, This patch is ready for adt approval. The other issue can be dealt with
seperately.

Johnny, the problem with the sample URL is that pages don't just include
themselves. The pattern is somewhat like this:

road.html
  foo1.html
    road.html
      foo1.html
        .. etc ..
      foo2.html
  foo2.html
    .. etc ..
  foo3.html
    .. etc ...

So references to road.html are sandwiched between references to other URLs. To
catch this kind of behaviour we would have to walk back through the parents,
counting matching URLs. Obviously a URL could be accompanied by postdata,
cookies, js etc. which means matching URLs may have different content. The
safeguard to this is count the matches and only reject after say 3 identical
URLs are found. Redirects might also screw around with this code.

This is no solution to malicious attacks, but it could stop runaway idiot code.

Comment 23

[Johnny Stenback (:jst)]

Fine with me, let's either leave this bug open after landing this fix or file a
new bug on better recursion protection wrt frames.

Comment 24

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin into 1.0. Pls check this one in
as soon as you have a= from drivers.

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 77835 [details] [diff] [review]
Patch reduces maximum limit for nested frames

a=[e-mail] for drivers.  If this misses the branch, please nominate for
checkin to the branch.

Comment 26

[Adam Lock]

Fix checkinto trunk still waiting for 1.0 branch approval.

Comment 27

[Adam Lock]

Fix checked into branch. I will raise a new bug on doing URL comparison.

Comment 28

[Adam Lock]

Raised bug 136580 to examine ways to catch recursive code earlier.

Comment 29

[Katsuhiko Momoi]

It seems that there is an evangelism issue here. 
I happened to have filed Bug 132582 last month. It is this
exact same bug. There are a couple of things that concern me.

1. With IE 5.5, I saw CPU consumption fluctuating between 65-95%.
   While it did not crash, the program became eventually unusable
   after a few minutes.
2. With Mozilla trunk build, I saw the CPU consumption going up to 100%.
   Then crash within 30 seconds or so when you do try to do anything with 
   Mozilla. 
The fix here would avoid a hang but probably would not prevent CPU 
consumption going up, would it? Please explain what this fix would prevent 
and would not prevent. Are we just trying to be on the par with IE, which
would also crash or become unusable in 1-2 minutes?

My sense is that this site needs to be evangelized not to code
in a way that forces browers to become more or less frozen. 
I will keep the other bug open and contact the web site unless I
here otherwise.

Comment 30

[Asa Dotzler [:asa]]

adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it
was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify
the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword
with verified1.0.0.

Comment 31

[av (gone)]

Current trunk debug build shows the number of webshells reaching 100 and layout
giving up. Is it a regression or this is how it should be?

Comment 32

[John Keiser (jkeiser)]

That's how it should be.  Show me a site with more than 100 frames and I'll show
you a site with too much time on its hands :)

Comment 33

[WD]

Mozilla currently hangs on the site in question, though.   IE and Netscape 4.x
seem to handle it in stride.   Should a new bug be opened??

Comment 34

[av (gone)]

And I am talking about the original url. I did not look at it... Does it have
100 frames or this is a recursion problem?

Comment 35

[John Keiser (jkeiser)]

Recursion.