Last Comment Bug 136580 - Catch recursive content much sooner
: Catch recursive content much sooner
Status: RESOLVED FIXED
[adt2 RTM] [ETA 07/11]
: crash, perf, testcase, topembed
Product: Core
Classification: Components
Component: Layout: HTML Frames (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Future
Assigned To: John Keiser (jkeiser)
: Chris Petersen
: Jet Villegas (:jet)
Mentors:
http://www.ccidnet.com/road.html
: 89300 (view as bug list)
Depends on:
Blocks: 89300 98158 143047 154896
  Show dependency treegraph
 
Reported: 2002-04-10 04:48 PDT by Adam Lock
Modified: 2003-02-04 10:17 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Branch Patch (3.09 KB, patch)
2002-04-10 15:33 PDT, Adam Lock
john: review+
jst: superreview+
jud: approval+
Details | Diff | Splinter Review
Trunk Patch (2.56 KB, patch)
2002-06-20 11:06 PDT, John Keiser (jkeiser)
john: review+
jst: superreview+
Details | Diff | Splinter Review

Description Adam Lock 2002-04-10 04:48:29 PDT
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 2002-04-10 15:33:33 PDT
Created attachment 78626 [details] [diff] [review]
Branch Patch

This patch adds code to nsFrameFrame::LoadURL to walk the parents looking for
matching URLs.
Comment 2 Aleksander Adamowski 2002-04-22 02:21:48 PDT
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 2002-04-22 11:57:55 PDT
cc'ing Mitch for the Security aspect of this issue -
Comment 4 Jesse Ruderman 2002-05-07 04:00:28 PDT
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 2002-05-31 12:57:42 PDT
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. 2002-06-05 09:14:34 PDT
--> Kevin for reassignment, and re-assessing.
Comment 7 basic 2002-06-05 10:56:43 PDT
shouldn't this be in HTMLframes?
Comment 8 Kevin McCluskey (gone) 2002-06-05 13:18:49 PDT
-> jkeiser
Comment 9 Kevin McCluskey (gone) 2002-06-14 16:10:42 PDT
Katsuhiko Momoi: Can you provide a URL to the China site that is crashing?
Comment 10 Katsuhiko Momoi 2002-06-18 13:35:10 PDT
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) 2002-06-18 14:38:16 PDT
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) 2002-06-19 16:00:44 PDT
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 2002-06-20 08:11:56 PDT
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) 2002-06-20 11:06:44 PDT
Created attachment 88489 [details] [diff] [review]
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) 2002-06-21 15:59:52 PDT
"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) 2002-06-26 11:36:59 PDT
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 2002-06-26 11:57:39 PDT
Comment on attachment 88489 [details] [diff] [review]
Trunk Patch

r=adamlock
Comment 18 Jaime Rodriguez, Jr. 2002-07-08 15:51:56 PDT
can someone pls sr= this one, so we can try and get it on the 1.0.1 train?
Comment 19 John Keiser (jkeiser) 2002-07-08 16:10:40 PDT
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) 2002-07-08 16:15:05 PDT
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, jst@mozilla.com) 2002-07-08 16:20:18 PDT
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, jst@mozilla.com) 2002-07-08 16:22:01 PDT
Comment on attachment 88489 [details] [diff] [review]
Trunk Patch

sr=jst
Comment 23 Jaime Rodriguez, Jr. 2002-07-09 08:29:06 PDT
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) 2002-07-09 23:56:43 PDT
Fix checked in.
Comment 25 Jaime Rodriguez, Jr. 2002-07-10 08:53:58 PDT
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 2002-07-10 13:14:13 PDT
I'm working on this now. Already verified on OS X trunk build and checking on
Windows trunk.
Comment 27 Chris Petersen 2002-07-10 13:46:45 PDT
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 2002-07-10 14:56:52 PDT
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 2002-07-11 09:45:11 PDT
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 2002-07-11 09:49:43 PDT
adding adt1.0.1+.  Please check this into the branch today.
Comment 31 John Keiser (jkeiser) 2002-07-11 20:39:29 PDT
Checked in to branch.
Comment 32 Jaime Rodriguez, Jr. 2002-07-19 12:56:41 PDT
petersen: Chris can you verify this on the 1.0 branch?
Comment 33 Chris Petersen 2002-07-19 13:14:32 PDT
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 2002-07-19 14:05:38 PDT
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. 2002-07-22 20:20:21 PDT
petersen: pls verify this as fixed on the branch, then replace "fixed1.0.1" with
"verified1.0.1". thanks!
Comment 36 John Keiser (jkeiser) 2002-07-22 20:53:26 PDT
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 2002-08-12 12:45:49 PDT
It looks like the problems is occuring for http://www.ccidnet.com, bug 161375 
Comment 38 John Keiser (jkeiser) 2003-01-30 17:05:09 PST
*** Bug 89300 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.