136580 - Catch recursive content much sooner

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P2)

Description

[Adam Lock]

Mozilla should prevent runaway / recursive content as soon as possible. There is
already a nested frame limit but it takes a long time to halt content which is
recursively loading itself. We should insert better detection code to catch it
earlier.

See bug 126466 for a description of a problem and solution. This comment
describes one solution based on counting matching URLs in the frame hierarchy.

http://bugzilla.mozilla.org/show_bug.cgi?id=126466#c22

This URL demonstrates badly behaved content. It recursively loads itself into
iframes, running until it hits the frame limit. It could be caught much sooner.

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

Comment 1

[Adam Lock]

Attachment: Branch Patch

This patch adds code to nsFrameFrame::LoadURL to walk the parents looking for
matching URLs.

Comment 2

[Aleksander Adamowski]

There's also a problem with OBJECT tags, thus Mozilla is vulnerable to issue
discussed on Bugtraq here (though only on Linux, mozilla-win32 trunk handles
this quite gracefully, see dup bug 139108 for details and a screenshot):
http://online.securityfocus.com/archive/1/268776

Try this test page:
https://olo.office.altkom.com.pl/domowa/security/dalej.html

It contains the following code:
<object data="dalej.html" type="text/html"></object>

Comment 3

[Phil Schwartau]

cc'ing Mitch for the Security aspect of this issue -

Comment 4

[Jesse Ruderman]

As I said in bug 89300 comment 4, I think the problem is the crash, not the fact
that Mozilla lets subpages have the same URL.  There are many ways to get around
this same-URL check, while fixing the crash here might fix "real" crash bugs.

Comment 5

[Katsuhiko Momoi]

This affects one of the major sites in China. We need to do something
about this problem before the next major release.
Nominating for nsbeta1.
We need to re-assing this, should we not?

Comment 6

[Jaime Rodriguez, Jr.]

--> Kevin for reassignment, and re-assessing.

Comment 7

[basic]

shouldn't this be in HTMLframes?

Comment 8

[Kevin McCluskey (gone)]

-> jkeiser

Comment 9

[Kevin McCluskey (gone)]

Katsuhiko Momoi: Can you provide a URL to the China site that is crashing?

Comment 10

[Katsuhiko Momoi]

Sorry I did not make this clear. The parent page of the above URL:

http://www.ccidnet.com

is a top 20 site in China. This is what I was referring to. There is a
NS internal Bug-NS 16281 also.

Comment 11

[John Keiser (jkeiser)]

The URL in question appears to have something to do with iframe ads.  It takes
up full CPU and uses increasing stack, which is wrong.  Note that this page does
not render in IE.

I do not believe it has anything to do with this bug, however.  We already have
protection in place that should prevent iframes from recursing indefinitely.

Comment 12

[John Keiser (jkeiser)]

On WinXP trunk 2002061408 it takes up a bunch of memory but does not crash. 
Katsuhiko, do you see anything on these pages in IE?  (I don't.)

I will try it with the patch shortly.

Comment 13

[Adam Lock]

We have a limit on the number of nested frames (limited to 8 deep), but it
should be possible to detect runaway recursion before it hits that limit. In
this site, it still opens hundreds of iframes before hitting the limit.

I have already submitted a patch here that would catch stupid runaway content of
this type a little sooner. The patch puts a special case in to prevent the same
uri from appearing more than 3 times in the nesting hierarchy. 

I also favour site advocacy in this instance. Someone should send an email
(anyone speak Mandarin?) to these people because they might not realise what
their script is doing.

Comment 14

[John Keiser (jkeiser)]

Attachment: Trunk Patch

This is just Adam's patch, adjusted to JST's iframe change.

This fixes the site above, but you still have ~50 frames loading.  But I'm not
sure if this is the way we should go--we're just catching more and more edge
cases.	It seems like we should do what we reall want: have an overall limit to
how many iframes you can have in an iframe tree, total.

Comment 15

[Kevin McCluskey (gone)]

"On WinXP trunk 2002061408 it takes up a bunch of memory but does not crash. 
Katsuhiko, do you see anything on these pages in IE?  (I don't.)"

John, I see the same as you using todays 1.0 branch build On WinXP on 256MB
machine. It does not crash, but consumes a lot of memory.  Using IE6 it doesn't
display anything.

Comment 16

[John Keiser (jkeiser)]

Comment on attachment 88489 [details] [diff] [review]
Trunk Patch

r=jkeiser

This seems OK to me, now that I think.	The really proper way to do things is
not to hang when there are huge numbers of frames, but we'll see about that.

Comment 17

[Adam Lock]

Comment on attachment 88489 [details] [diff] [review]
Trunk Patch

r=adamlock

Comment 18

[Jaime Rodriguez, Jr.]

can someone pls sr= this one, so we can try and get it on the 1.0.1 train?

Comment 19

[John Keiser (jkeiser)]

Comment on attachment 78626 [details] [diff] [review]
Branch Patch

r=jkeiser (on the branch patch)

These two patches are precisely the same, just in different places for trunk
and branch.  Just covering bases with r=.

Comment 20

[Kevin McCluskey (gone)]

I filed a tech evang bug for the china site (//www.ccidnet.com) mentioned in
comment 10. http://bugzilla.mozilla.org/show_bug.cgi?id=156360

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 78626 [details] [diff] [review]
Branch Patch

+  if (parentAsSupports) {
+    nsCOMPtr<nsIDocShellTreeItem>
parentAsItem(do_QueryInterface(parentAsSupports));
+    while (parentAsItem) {
...

The above if (parentAsSupports) check is not needed, do_QueryInterface() is
null safe.

sr=jst with that fixed.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 88489 [details] [diff] [review]
Trunk Patch

sr=jst

Comment 23

[Jaime Rodriguez, Jr.]

pls land this on the trunk asap, so that it can be verified by QA, and evaluated
for taking on the branch. thanks!

Comment 24

[John Keiser (jkeiser)]

Fix checked in.

Comment 25

[Jaime Rodriguez, Jr.]

thanks john.

petersen: chris, john mnetioned you'd be able to verify this one today. it would
be real nice if we got this verified today, as i believe mozilla is gonna cut a
rc1 of 1.0.1 tonight.

Comment 26

[Chris Petersen]

I'm working on this now. Already verified on OS X trunk build and checking on
Windows trunk.

Comment 27

[Chris Petersen]

No longer fails on OS X build (2002-07-10-08 trunk) but still occurs under
Windows trunk (2002-07-10-08). Talked to John K and he is investigating further.

Comment 28

[Chris Petersen]

Updating my earlier comments: This problem is fixed in the today Mozilla Win32.
Page does load completely without the freezing issue I seeing in yesterday's
Mozilla trunk. I think this is good solution to the problem and makes the
browser more stable under these conditions. 

Comment 29

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 30

[scottputterman]

adding adt1.0.1+.  Please check this into the branch today.

Comment 31

[John Keiser (jkeiser)]

Checked in to branch.

Comment 32

[Jaime Rodriguez, Jr.]

petersen: Chris can you verify this on the 1.0 branch?

Comment 33

[Chris Petersen]

Using the 2002-07-16-08 1.0.0 build, I 'm still able to reproduce this problem
using url from comment #10. Page loads with multiple popup windows appearing on
screen. After a few seconds, the throbber stops on browser and the application
appears to hang. Not sure if why this is the case on my Windows ME machine. 

Comment 34

[Terri Preston]

The URL http://www.ccidnet.com uses 99 percent of my CPU until I kill it using
win XP branch build 2002071908 and freezes my whole machine (causing me to
restart) using Win ME 2002071908

Comment 35

[Jaime Rodriguez, Jr.]

petersen: pls verify this as fixed on the branch, then replace "fixed1.0.1" with
"verified1.0.1". thanks!

Comment 36

[John Keiser (jkeiser)]

I believe the point was it's not fixed on all platforms.  The situation *is*
better.  I no longer see it on XP (nor does Chris, though Terri said she saw it)
but Chris and Terri both see it on '98.

Comment 37

[kill this account]

It looks like the problems is occuring for http://www.ccidnet.com, bug 161375 

Comment 38

[John Keiser (jkeiser)]

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