The default bug view has changed. See this FAQ.

Catch recursive content much sooner

RESOLVED FIXED in Future

Status

()

Core
Layout: HTML Frames
P2
normal
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: Adam Lock, Assigned: John Keiser (jkeiser))

Tracking

(4 keywords)

Trunk
Future
crash, perf, testcase, topembed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments)

3.09 KB, patch
John Keiser (jkeiser)
: review+
Judson Valeski
: approval+
Details | Diff | Splinter Review
2.56 KB, patch
John Keiser (jkeiser)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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
(Reporter)

Comment 1

15 years ago
Created attachment 78626 [details] [diff] [review]
Branch Patch

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

Updated

15 years ago
Keywords: patch
Priority: -- → P2
Target Milestone: --- → Future

Comment 2

15 years ago
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

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

Comment 4

15 years ago
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

15 years ago
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

Updated

15 years ago
Keywords: perf

Comment 6

15 years ago
--> Kevin for reassignment, and re-assessing.
Keywords: topembed
Whiteboard: [adt2 RTM]

Updated

15 years ago
Blocks: 143047

Comment 7

15 years ago
shouldn't this be in HTMLframes?
Keywords: mozilla1.1
-> jkeiser
Assignee: attinasi → jkeiser
Component: Layout → HTMLFrames
(Assignee)

Updated

15 years ago
Blocks: 89300
(Assignee)

Updated

15 years ago
Blocks: 122856
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
Katsuhiko Momoi: Can you provide a URL to the China site that is crashing?

Comment 10

15 years ago
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.
(Assignee)

Comment 11

15 years ago
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.

Updated

15 years ago
Keywords: testcase

Updated

15 years ago
Whiteboard: [adt2 RTM] → [adt2 RTM] [ETA needed]
(Assignee)

Comment 12

15 years ago
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.
(Reporter)

Comment 13

15 years ago
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.
(Assignee)

Comment 14

15 years ago
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.
"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.
(Assignee)

Comment 16

15 years ago
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+
(Reporter)

Comment 17

15 years ago
Comment on attachment 88489 [details] [diff] [review]
Trunk Patch

r=adamlock
(Assignee)

Updated

15 years ago
Blocks: 98158

Updated

15 years ago
Blocks: 154896
Whiteboard: [adt2 RTM] [ETA needed] → [adt2 RTM] [ETA 07/09]

Updated

15 years ago
No longer blocks: 154896

Updated

15 years ago
Blocks: 154896

Comment 18

15 years ago
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+
(Assignee)

Comment 19

15 years ago
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+

Updated

15 years ago
Keywords: adt1.0.1

Updated

15 years ago
Keywords: crash

Comment 23

15 years ago
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

Updated

15 years ago
Whiteboard: [adt2 RTM] [ETA 07/09] → [adt2 RTM] [ETA 07/10]
(Assignee)

Comment 24

15 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 25

15 years ago
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]

Comment 26

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

Comment 27

15 years ago
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

15 years ago
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. 

Updated

15 years ago
Attachment #78626 - Flags: approval+

Comment 29

15 years ago
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+

Comment 30

15 years ago
adding adt1.0.1+.  Please check this into the branch today.
Keywords: adt1.0.1 → adt1.0.1+
(Assignee)

Comment 31

15 years ago
Checked in to branch.
Keywords: fixed1.0.0

Comment 32

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

Comment 33

15 years ago
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

15 years ago
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

15 years ago
petersen: pls verify this as fixed on the branch, then replace "fixed1.0.1" with
"verified1.0.1". thanks!
Keywords: mozilla1.0.1+, patch
(Assignee)

Comment 36

15 years ago
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.

Updated

15 years ago
No longer blocks: 122856

Comment 37

15 years ago
It looks like the problems is occuring for http://www.ccidnet.com, bug 161375 
(Assignee)

Comment 38

14 years ago
*** Bug 89300 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.