Closed Bug 136580 Opened 22 years ago Closed 22 years ago

Catch recursive content much sooner

Categories

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

Product:
Core
Component:
Layout: Images, Video, and HTML Frames
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Future

People

(Reporter: adamlock, Assigned: john)

References

(
URL
)

Details

(4 keywords, Whiteboard: [adt2 RTM] [ETA 07/11])

Attachments

(2 files)

Branch Patch
3.09 KB, patch
john
: review+
jst
: superreview+
jud
: approval+
Details | Diff | Splinter Review
Trunk Patch
2.56 KB, patch
john
: review+
jst
: superreview+
Details | Diff | Splinter Review 
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
Attached patch Branch Patch — — Splinter Review 
This patch adds code to nsFrameFrame::LoadURL to walk the parents looking for
matching URLs.
Keywords: patch
Priority: -- → P2
Target Milestone: --- → Future
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>
cc'ing Mitch for the Security aspect of this issue -
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.
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?
Keywords: nsbeta1
Keywords: perf
--> Kevin for reassignment, and re-assessing.
Keywords: topembed
Whiteboard: [adt2 RTM]
Blocks: 143047
shouldn't this be in HTMLframes?
Keywords: mozilla1.1
-> jkeiser
Assignee: attinasi → jkeiser
Component: Layout → HTMLFrames
Blocks: 89300
Blocks: 122856
Status: NEW → ASSIGNED
Katsuhiko Momoi: Can you provide a URL to the China site that is crashing?
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.
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.
Keywords: testcase
Whiteboard: [adt2 RTM] → [adt2 RTM] [ETA needed]
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.
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.
Attached patch Trunk Patch — — Splinter Review 
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.
"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 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.
Attachment #88489 - Flags: review+
Comment on attachment 88489 [details] [diff] [review]
Trunk Patch

r=adamlock
Blocks: 98158
Blocks: 154896
Whiteboard: [adt2 RTM] [ETA needed] → [adt2 RTM] [ETA 07/09]
No longer blocks: 154896
Blocks: 154896
can someone pls sr= this one, so we can try and get it on the 1.0.1 train?
Keywords: nsbeta1 → mozilla1.0.1, nsbeta1+
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=.
Attachment #78626 - Attachment description: Patch → Branch Patch
Attachment #78626 - Flags: review+
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 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.
Attachment #78626 - Flags: superreview+
Comment on attachment 88489 [details] [diff] [review]
Trunk Patch

sr=jst
Attachment #88489 - Flags: superreview+
Keywords: adt1.0.1
Keywords: crash
pls land this on the trunk asap, so that it can be verified by QA, and evaluated
for taking on the branch. thanks!
Keywords: approval
Whiteboard: [adt2 RTM] [ETA 07/09] → [adt2 RTM] [ETA 07/10]
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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.
Whiteboard: [adt2 RTM] [ETA 07/10] → [adt2 RTM] [ETA 07/11]
I'm working on this now. Already verified on OS X trunk build and checking on
Windows trunk.
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.
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.
Attachment #78626 - Flags: approval+
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
adding adt1.0.1+.  Please check this into the branch today.
Keywords: adt1.0.1 → adt1.0.1+
Checked in to branch.
Keywords: fixed1.0.0
petersen: Chris can you verify this on the 1.0 branch?
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. 
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
petersen: pls verify this as fixed on the branch, then replace "fixed1.0.1" with
"verified1.0.1". thanks!
Keywords: mozilla1.0.1+, patch
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.
No longer blocks: 122856
It looks like the problems is occuring for http://www.ccidnet.com, bug 161375 
*** Bug 89300 has been marked as a duplicate of this bug. ***
Product: Core → Core Graveyard
Component: Layout: HTML Frames → Layout: Images
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: