From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.3+) Gecko/20010903 BuildID: 2001090303 While using configurator an said URL: The whole computer slows down. After a while it crashes. On the task manager, memory usage go up and up... (win2000) I tried on Linux (Mozilla 0.9.1 on debian woody), and the whole system became unresponsive and after a few minutes I did a hard reset. It's probably the same problem. Reproducible: Always Steps to Reproduce: 1. Load the start URL (http://www.arcplan.com/dynasel/demo.htm) 2. Click on the START button 3. Click on the small arrow right to "Please click to enter" 4. On the combo box right to "Frame size", select 62 (or any value) 5. Watch mozilla hog all the memory (or freeze the whole system :-( ) Actual Results: Mozilla seemed to update the page, but grew in memory, until it crashed (or I stopped it from task manager) Expected Results: visibly nothing (only update the value on the combo box and the server) Appears on Mozilla 0.9.3 and nightly build 2001090303 on win2000 Does not appear on Netscape 6.0 but on Netscape 6.1 Mozilla 0.8 won't even load the page completely. Appears on linux with Mozilla 0.9.1

getting off unconfirmed list

I have a feeling that this is caused by something going wrong and causing the js/dom stuff to go recursive infinitely. When selecting something from the list it causes a submit and a page is loaded somewhere (not sure where but probably in a hidden frame) with an onload="". I'll try to get a very much simplified testcase.

Alexandre: Are you the mentainer of the site? Could you try to create a simpler testcase? I've tried but there's just too many frames and js involved.

this might be another case of bug 92425

I'm not maintainer of the site, but developping the server that generates the html. I think it's possible to make a test with only one frame. I'll try that.

-> Dom0

I made a simplified example at: http://dynasel.arcplan.com/moztest.html it still has 3 frames... click on the START button chose 'red' on the combo box. that's it. It is expected that the white rectangle below 'Color' turns red.

When loading the testcase in mozilla I hit the assert NS_ASSERTION(aNextSibling != this, "attempt to create circular frame list"); in nsFrame, and from there on it's all downhill. Here's the stack for the assertion: nsFrame::SetNextSibling() nsFrameItems::AddChild() nsCSSFrameConstructor::ConstructFrameByDisplayType() nsCSSFrameConstructor::ConstructFrameInternal() nsCSSFrameConstructor::ConstructFrame() nsCSSFrameConstructor::ContentAppended() StyleSetImpl::ContentAppended() PresShell::ContentAppended() nsDocument::ContentAppended() nsHTMLDocument::ContentAppended() nsGenericElement::doInsertBefore() nsGenericHTMLContainerElement::AppendChild() nsHTMLTableElement::AppendChild() nsGenericHTMLElement::SetInnerHTML() nsGenericHTMLElementTearoff::SetInnerHTML() XPTC_InvokeByIndex() Reassigning to Layout.

circular frame list - cool. accepting, P1 crasher.

Bulk-reassign of some crashers to Waterson - thanks Chris.

Simpler testcase online again (http://dynasel.arcplan.com/moztest.html) If it's offline again and you need it, feel free to mail me.

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 (you can query for this string to delete spam or retrieve the list of bugs I've moved)

Crashed today using w2k 2002021403. Talkback ID: (couldn't connect to server) Comments from talkback: Tried to reproduce bug 98158. Loaded attachment 54196 [details] in new tab. Mozilla went into a sort of infinite loop (cf. java script console, same message repeated over and over). Close tab led to crash (didn't crash without me clicking close).

alexandre, can you show your testcase's html code? I can not see it in IE and mozilla. using mozilla with debug rc1 version, error message show on my console looply, such as webshell+= 690 webshell+= 691 ... JavaScript error: http://bugzilla.mozilla.org/common/selscript.js line 1: syntax error.

Attachment 54196 [details], zipped, because it is very difficult to get it from bugzilla (should we file a bug for this against bugzilla ? ;-) ) open the task manager (to kill mozilla yourself before your machine is hogged), unzip the file, open it in Mozilla with Open File..., and watch the cpu/memory usage. It probably isn't valid html, but mozilla shouldn't die so horribly on it, should it?

Something is wrong with me or bugzilla, or perhaps I only have a bad day... Anyway, when you download my last attachment, rename the file as attachment.zip, because I doubt your system will understand what is a .cgi file. Sorry if it is unnecessarily complicated, but it didn't work as I expected Alexandre

I hava a very simple testcase; --------------------------- <html> <frameset rows=105,*> <frame src=#http://www.sina.com.cn name=sel scrolling=NO> </frameset> </html> --------------------------- If you cut the "#", the page will present normally, but if you add the "#", the presentation is error. I trace the source code, the # caused a event to make program to read the tag buffer cyclely, ... --> nsParser::OnStartRequest --> nsParser::OnDatatAvailable --> ResumeParser -->..... One time, two time ..... so too many token was produced. Raw tag --> token --> content --> frame who can tell me why the "#" caused the program produce the OnStartRequest event?

Reproducing this with current BRANCH_1_0 (X11; U; Linux i686; en-US; rv:1.0.0) Gecko/20020608 on Mandrake 8.2 Linux. The testcase makes Mozilla unusable, eating more and more memory while chewing CPU forcing me to kill its processes by hand.

During the reflow, in the nsFrameLoader::LoadFrame, the program will load a URL according the html <frame src=<url> ....>. If the '#' and '?' located in the beginning of <url>, the program cause a indefinite cycle. when the program process the cgi <url> like query, it need process '#' and '?' in the nsStandURL::Resolve. when it process the '#' and '?' before scheme of url, it will cause confused. Since, the # and ? in the beginning of url is useless, so, the simple method to fix this bug is that cutting the # and ? before the scheme of url. I give a patch that avoid mlk and crash when access the three testcases. If you test it according to the comment 16, It will work well.

I think there's a standard necko call you can make to strip out the part after the delimiter (`?', `#', etc.) CC darin and bbaetz for code review.

Comment on attachment 88110 [details] [diff] [review] proposed patch for review No, this isn't a necko bug. An href of "#foo" points to a fragment from the current page. Look below in this function, where we check for a relative url, and handle things beginning with # and ? The frame endless-loop detection code should be ignoring things with the same url, excluding the fragment. However, even if it didn't, the top level frame ('foo.html') would load 'foo.html#bar', which would then load 'foo.html#bar', and so the loop detection stuff should kick in then. hwaara: There are ways to do that, but not while we're still in the process of parsing it.

Hi Bradley: Could you tell me how to detect the frame endless-loop? what other ways can do that? if not while we're still in the process of parsing it.

I don't know who does that, sorry. I assume we do have code to handle that, somewhere, though.

Oh, My obsolete patch will cuase some errors when you access the <URL> like http://bugzilla.mozilla.org/show_bug.cgi?id=98158#c9. I will create a new patch for this bug.

Bradley is right, meeting with An href of "#foo" points to a fragment from the current page. If we cut #, it will cause some error. I want to analyze the reson lead to endless-loop. Firstly, the process for "#"(ref) or "?"(query). for example http://bugzilla.mozilla.org/show_bug.cgi?id=98158#c9, the program have two steps to process this url, the first is for when http://bugzilla.mozilla.org/show_bug.cgi?id=98158, the second is for #c9, when processing #c9, #c9 is ref not a real url, it needn't load a new uri. Secondly, when the program process a url1 include "<frame src=<url2> ... >" when processing src, the url change to <url1+url2> to load a uri, if url2 begin with #, the url will be <url1+#url2>,for example, http://buzilla.mozilla.org#http://google.com.so, the url2 will be looked as a ref to process. It will be cut to two urls to process. when process url2<google.com>, the program will restore the whole url<url1+url2> to load a new uri. Now the progam run back to the initial state. I think it will lead to the endless-loop. I think the simplest mothod to fix this bug is that cutting the "#" or "?" at the beginnig of <url> located <frame src=<url> ... >. I can cut it in the nsFrmaeLoader::LoadFrame(nsnsFrameLoader.cpp), It will not affect other parts and avoid endless-loop. Bradley & others, Do you agree with me? If not, give some suggestions!

I modified the code according to the comment27, and test it using the testcases, all work normally. Please test it & give me some suggestions

Comment on attachment 88417 [details] [diff] [review] Patch V1.0 for review Just trimming seems like the wrong solution. First, it will cause an attempt to load a URL that wasn't in the document, leading to a 404 error. Second, Trim will trim from the end of the string rather than the beginning, which you don't want. Also, if there's already real recursion-detection somewhere, can't it be fixed to handle this?

If we do concatenate urls with +, then how do we handle urls with a + character in them? (eg an escaped space for a uri? Whats the lxr url to the that code?

Hi David Firstly, I think the "#" or "?" at the beginning of the url is useless. so, cutting it has no problem. Secondly, src.Trim() will cut the special charactar from string at the beginning and the end. because the # & ? at the beginning or end is useless. Trim can not cut the charactar among the url like src=http://www.sina.com#cn. the # cannot be cut. so, I believe it will not cause an attempt to load a URL that wasn't in the document. I hope you can download the testcase to local computer and replace the html file in the testcase, and then use my patch to test it. Don't use my method, you must modify the mechanism for processing html frame tag or processing #ref and ?query. it will take large effort.

Hi Bradley: I think I have a mistake in comment 27. I want to say when the src=#file://foo/boo.html/, the program can not parse the scheme, so, it will call nsStandardURL::Init to solve the baseURI. when set a breakpoint at baseURI->Resolve(spec,buf) and watch buf, the buf changed to url1+url2(like file://export/home/testcase/test1.html/#./test2.html, here ,url1 is file://export/home/testcase/test1.html/ url2 is #./test2.html). you can see the file ../mozilla/netwerk/base/src/nsStandardURL.cpp and see the function nsStandardURL::Resolve. There are some code to process # and ? in this function.

Oh, I see. I misunderstood teh + stuff, sorry. My point is that that is correct - we are being told to load a frame at that url, and scroll to that anchor if it exists. Trimming it off anywhere would break that (which I presume we support) If the problem for this bug is that we're ending up loading frames recursively for ever, something is broken in the frame loader. Isn't this a dupe of bug 89300 (that test case is down now, though), or, more likely, bug 136580? In fact, the patch in bug 136580 is exactly waht you need here, isn't it? (Plus some error checking that patch seems to leave out...)

The patch for 136580 can avoid the endless-loop, by breaking the loop accoring a limit condition. I think it is a good method. But,I think the best method is that can avoid endless-loop, not force to break loop.Now, I don't know the real reason for bug 136580. My patch is only for the testcases of this bug, not for testcase of bug 136580. I think you should accept my patch.

What if I really did want to load "?foo" ? That could run a CGI script which gives me a totally different page. I haven't played with frame loading, so I'll let jkeiser (who wrote the other patch) comment on it some more.

The fact that this particular syntax confuses the URL parser and causes it to load the same document over is a question of what the parser should do when an invalid URL shows up to it. I don't see why we should hack and fix a couple of bad cases just to fix this recursive problem. The recursion problem is going away soon. The thing about a bad URL trying to reload the frame is definitely bad--can you think of a way to fix that directly, without rewriting the URL? What should we do when the parser gets confused? Maybe give up and not load anything or load about:blank? That's the real problem and the real solution here.

But this isn't an invalid URL. I expect a <frame src="foo.html#bar"> to load foo.html, and scroll to the anchor with a name of 'bar'. Whats invalid is that its recursive. Whats wrong with the fix for bug 136580?

The fix in that bug is fine, and will fix this. My vague understanding was that the parser did not interpret <frame src="#whatever"> as "currentpage#whatever". If that's wrong then you're right, this bug can just be put to rest.

Oh! a <frame src="foo.html#bar"> will load foo.html#bar not the foo.html, because the patch only cut the # at the beginning of the src url. like <frame src="#foo.html"> will load foo.html.

Jack: if the current page is "currentpage.html" and you do <frame src="#foo.html">, it will try to load "currentpage.html#foo.html", right? If it does, it's not a bug, it's doing the right thing.

Yes, without my patch, you do <frame src="#foo.html">, it will try to load "currentpage.html#foo.html", but when it load currentpage.html#foo.html, It will load currentpage.html firstly, and load #foo.html secondly. when it load #foo.html, it continue load "currentpage.html#foo.html", Now a endless-loop take place. With my patch, the do <frame src="#foo.html">, it will load foo.html.

The patch for bug136580 seem useless to this new testcase. I think the patch for 136580 is very similar to the part for bug 8065 in nsFrameLoader::EnsureDocShell(). If I change the MAX_DEPTH_CONTENT_FRAMES from 8 to 1. the new testcase can work well.

The patch for 136580 check recursive loop after NewURI, I think the checking is too late. so the patch can not work on the new testcase(this testcase is same as the first one). I notice that the EnsureDocShell() include a check for recursive loop, I modify the depth limit from 8 to 2. so, it can work well one the new testcase. I think that the modification can not affect the other parts.

Can anyone take a look at Jack's patch and give some suggestions? This seems to be a serious issue.

Ack, I'm sorry it took me so long to comment, I thought I had already. Limiting things to 2 nested frames will fix the problem but breaks content that goes deeper than 2 frames (which I can almost guarantee happens somewhere). It's not a viable solution. A better fix might be to limit the *total* number of webshells that we can create, regardless of depth. The real problem is that we run out of memory and crash. We could also fix it from that angle.

Is it also possible that our two existing protections aren't working? Those protections are: 1. limit frames to 8 in depth 2. limit frames to 3 in depth when they have the same name (to avoid recursion)

1. limit frames to 8 in depth 2. limit frames to 3 in depth when they have the same name (to avoid recursion) These two existing protections aren't working, If we limit frames from 8 to 3 or more little. the number of webShells will be fewer than before and not crash.

The question was more whether we are in fact stopping after 8 or 3 webshells still. No matter how many webshells we stop at, it will always be possible to crash the browser (unless perhaps we limit the total number of webshells).

I agree with John Keiser. I give a patch for review according to his idea.

Comment on attachment 93071 [details] [diff] [review] A PATCH for review The global webshell count limits the total number of applications and the total number of webshells that *all* apps can create--all browsers, all mail/news windows, IRC, etc. I think we need to limit the number of web shells in each page. Limiting the total number of webshells for the app is too constrictive. The problem we're trying to solve is recursive frames.

What you could do is store mTotalNumberOfChildren in the webshell and go up to the top webshell in the page (target=_top) and update that every time you add or subtract a webshell. If that count ever goes above 100, then you stop.

Jkeiser: Could you review the new patch?

Comment on attachment 93268 [details] [diff] [review] A new Patch for review. Yes, this is where we want to go! Comments: 1. The increment and decrement stuff belongs better in nsDocShell::SetParent() (or you could override that and do nsWebShell::SetParent()). You decrement from the previous parent / set of parents, and when it's null you decrement, when it's not null you increment (but see point #2) 2. There is a logic error--parents need to increase by the number of new child DocShell's number of children as well as the DocShell itself (1+child->GetNumOfWebShells()). Just incrementing might get you the wrong number. Same with removal--if you remove a child docshell that had 10 children, you need to decrease your number of children by 11. 3. While you're in nsFrameLoader.cpp, could you get rid of the extra MAX_SAME_URL_CONTENT_FRAMES definition near the top? You can see there are two. This is my fault :)

I meet some questions. In nsDocShell::SetParent(), I can get the top docshell in ease page. so I can increment the number of docshells. but, when removing a child node. Not all node can find the top docshell, so I can't decrement the numbber of docshell in the correct top docshell. I feel some node's mParent was modified to null,so I can not get he correct top docshell. + // bug 98158: Limit the totoal number of docshells in each page. + // regardless of the depth. + // Go to the top docshell and count the number of child docshells + // If the count goes above MAX_NUMBER_DOCSHELLS, then return a + // failure tag. + + // If adding a childnode, the @Para aParent not be null, through + // the aParent, you can get point of top docshell. and increment + // the number of total number of child docshell in one page. + // If remove a childnode, the @Para aParent be null, we must use + // "this" to get the top docshell. At mean while, decrement the + // number of total number of child docshell in one page. + + nsCOMPtr<nsIDocShellTreeItem> parentItem; + if (!aParent) { + parentItem = NS_STATIC_CAST(nsIDocShellTreeItem *, this); + } else { + parentItem = aParent; + } + + while (parentItem) + { + if (aParent) + { + parentItem->SetNumDocShells(1); + + PRInt32 DocShellCnt; + parentItem->GetNumDocShells(&DocShellCnt); + if (DocShellCnt > MAX_NUMBER_DOCSHELLS) + { + NS_WARNING("Too many webshells in on page, so giving up"); + return NS_ERROR_FAILURE; // Too many, give up! (silently?) + } + + } else { + parentItem->SetNumDocShells(-1); + } + + PRInt32 parentType; + parentItem->GetItemType(&parentType); + + if (nsIDocShellTreeItem::typeContent == parentType) + { + nsIDocShellTreeItem* temp = parentItem; + temp->GetParent(getter_AddRefs(parentItem)); + } else { + break; + } + } + : John keiser: give me some suggestion.

nsDocShell::SetParent(...aParent) { mParent = aParent; ... ... nsCOMPtr<nsIDocShellTreeItem> root; /* Get the root docshell. */ GetSameTypeRootTreeItem(getter_AddRefs(root)); NS_ENSURE_TRUE(root, NS_ERROR_FAILURE); if (aParent) { root->SetNumDocShells(1); // increment the number of docshells PRInt32 DocShellCnt; root->GetNumDocShells(&DocShellCnt); // get the number if (DocShellCnt > MAX_NUMBER_DOCSHELLS) { NS_WARNING("Too many webshells in on page, so giving up"); bp1---> return NS_ERROR_FAILURE; // Too many, give up! (silently?) } } else { bp2---> root->SetNumDocShells(-1); // decrement the number } return NS_OK } I set two break points above and watch the root, I expect that when increment the root keep one point. and when decrement the root is the same point as increment. The watch result: when increment, root was only one value.--->correct when decrement, root modified frequently. ----> error? why? john keiser: Could you see the code and give me suggestion?

sorry, I have mistake. mParent=aParent caused error.

Because not all the destroying or a docshell call by nsDocShell::SetParent, so, I can't decrement the total number of top docshell correctly in this function. so, i change the decrement the number to the nsDocShell's deconstructor. John keiser, could you review this patch? and give some suggestion.

Comment on attachment 93705 [details] [diff] [review] A new patch for review. The basic problem you are running into is because you need to decrement the *old* docshell *before* you set mParent = aParent, and increment the *new* one *after* you set mParent = aParent. What you want to do is something like this: // Helper to add a certain amount to all parent DocShells void AdjustAncestorDocShells(PRInt32 aAmount) { nsCOMPtr<nsIDocShellTreeItem> parent; nsCOMPtr<nsIDocShellTreeItem> tmpParent; // Get the first ancestor this->GetSameTypeParent(parent); while (parent) { // Actually adjust the docshell count PRInt32 numDocShells = 0; parent->GetNumDocShells(&numDocShells); parent->SetNumDocShells(numDocShells + mNumDocShells); // Get the next ancestor tmpParent = parent; tmpParent->GetSameTypeParent(parent); } } SetParent(aParent) { // Remove our docshell count amount from old ancestors AdjustAncestorDocShells(-mNumDocShells); mParent = aParent; // Add our docshell count to new ancestors AdjustAncestorDocShells(mNumDocShells); } This patch has several other problems: 1. You have to add or remove mNumOfDocShells from the root docshell when you set your docshell's parent, not 1. Scenario: (a) docshell B contains C, its mNumDocShells = 2. (b) docshell A has no children, its mNumDocShells = 1. (c) we set B's parent to be A. (d) docshell A needs to have mNumDocShells = 3, but your code only increments it by 1, so it will be 2. 2. SetNumDocShells should not increment or decrement mNumDocShells. It's a setter. A setter does not add to a number, it sets the number. You could create a method, addToNumDocShells(), that does what you want. 3. Your code does not null out mParent when it's done. 4. You are returning failure when it cannot find the root, but this will actually happen in normal cases! Specifically when you add the main content docshell into the chrome. 5. The comment on numDocShells should probably read something like: "The total number of descendant docshells of the same type, plus the number of docshells on the current shell. This number is used to determine whether there are too many docshells created in content. This is not a full review because the patch isn't ready yet :)

Do we really need to store the number of docshells contained within a docshell, can't we just always calculate it? Or would that be O(n^2) where n is a significant number in cases we care about?

It depends on whether the case we care about is when we add a frame or iframe. It would be O(n^2) where n is the number of docshells in the document. In ordinary cases it's something we could easily live with. In this case it would get purty big. It is a good point, that would work fine. However, the amount of bloat in cases we care about is *also* minimal. So either way, the cases we care about don't matter much. And this algorithm really isn't that bad.

jkeiser:a new patch for review :) mNumDocShell is 1(self) + number of children node.

Comment on attachment 93832 [details] [diff] [review] a new patch Looking good :) Just a few things, mostly small--this patch is algorithmically just about right. Index: nsDocShell.cpp =================================================================== +// Bug 98158: Limit to the number of total docShells in one page. Drop the "to", English grammar + // bug 98158: Limit the totoal number of docshells in each page. s/totoal/total, change the period (.) at the end to comma (,). + if (!aParent) + { + PRInt32 numDocShells = 0; + AdjustAncestorDocShells(-mNumDocShells,&numDocShells); mParent = aParent; + } + else { + mParent = aParent; + PRInt32 numDocShells = 0; + AdjustAncestorDocShells(mNumDocShells,&numDocShells); + if (numDocShells >= MAX_NUMBER_DOCSHELLS) + { + NS_WARNING("Too many webshells in on page, so giving up"); + return NS_ERROR_FAILURE; // Too many, give up! (silently?) + } + } This should work in sane cases, but on the safe side, you should account for when a DocShell is moved from one tree to another (or one part of the tree to another). It's cleaner this way too: if (mParent) { <put the stuff in if (!aParent) clause here> } mParent = aParent; if (mParent) { <put the stuff in the else clause here> } ADDITIONALLY, in the failure case (too many webshells) you need to readjust the ancestors (AdjustAncestorDocShells(-mNumDocShells) and set mParent = nsnull so that everything is consistent. + if (!parent) + { + PRInt32 numDocShells = 0; + tmpParent->GetNumDocShells(&numDocShells); + + *aNumDocShells = numDocShells; + } + } Just put a little comment in front of this to explain what's going on ... it's not immediately obvious. Index: nsDocShell.h =================================================================== + NS_IMETHOD AdjustAncestorDocShells(PRInt32 aAmount,PRInt32* aNumDocShells); First, this doesn't need to be virtual. Just have it return nsresult. (Change the NS_IMETHODIMP to nsresult in the implementation too). Second, need some documentation on this, especially making it clear what aNumDocShells is for.) Index: nsIDocShellTreeItem.idl =================================================================== + + /* + Record the total number of child docshells in each page. If the number goes above + the limit, stop create the new docshells. + 1.) Get the number, + 2.) Set the number according to the number of child node. + */ + attribute long numDocShells; This description does not describe numDocShells. You might say something like "The total number of child docshells, # of descendants + 1 (for this docshell). This is used to create a limit to the total number of frames on a page (bug 98158). Index: nsFrameLoader.cpp =================================================================== + nsresult rv = parentAsNode->AddChild(docShellAsItem); + NS_ENSURE_SUCCESS(rv,NS_ERROR_FAILURE); This should set mDocShell = nsnull in the failure case.

Comment on attachment 93981 [details] [diff] [review] A PATCH for review Woohoo, this is it except a couple of comments and two forgotten changes from the review. + // bug 98158: Limit the totoal number of docshells in each page, + // regardless of the depth. s/totoal/total + + if (mParent) + { + PRInt32 numDocShells = 0; + AdjustAncestorDocShells(-mNumDocShells,&numDocShells); + } + mParent = aParent; + if (mParent) + { Put this (or somethihng like it) before the first mParent: // Remove the docshell count from ancestor docshells since we are removing this docshell from its parent And put this (or something like it) before the second if (mParent): // Add the docshell count to ancestor docshells since we have been added to the parent docshell--and make sure that we haven't gone over the limit (bug 98158) + // Get the root docshell point here, + // Get the total number of docshells in one page, + // and return it through @Para aNumDocShell. This is hard to read. How about: // We are at the root docshell. numDocShells is the total number of shells in the page; return it. Also, you forgot to change AdjustAncestorDocShells() ... just make the method return void, you're not returning any kind of failure anyway. I just noticed something else in AdjustAncestorDocShells: you need to set *aNumDocShells = mNumDocShells in the case where there is no parent. Always set return values. Just change that stuff and r=jkeiser.

According to jkeiser's suggestion, I add some comments. jkeiser gave r=. who can give sr= ?

Comment on attachment 93987 [details] [diff] [review] a better patch for sr= r=jkeiser But you missed the word "totoal" again, which should be "total".

The same patch with above. Sorry for my spelling mistake.

The more I think about this the more I think this code does *not* belong in the docshell. A docshell hierarchy is not necessarily large (a docshell hierarchy can in theory even exist w/o a single document in it), and IMO should not be capped of in size (or count, rather). The layout of a large docshell hierarchy is however large (potentially, at least), and that's where we need IMO the cap. If we absolutely have to, we can possibly add some code to the docshell to help with the counting of number of docshells in a hierarchy, but the cap, and the bulk (ideally all) of the code for this should live in layout (or possibly content) land.

According to the testcase of this bug. If we only limit the depth of the docshell hierarchy like the patch of bug 136580, in addition, the frames in a page is too many, the testpage will make too many many docshells. that can couse short of memory or crash. so, I think it was not a good method. comparing with that,I agree with jkeiser. I think limit the total docshell in each is a better method that limit the depth of docshell hierarchy. jst's idea seems reasonable, but I think limit of a total number of docshell is more simple and safe than limit of number of one hierarchy and depth.

I'm not suggesting you change what you limit here, I'm just saying that I think you put the limiting code in the wrong place. I.e. the code you have should be useable, but it needs to move from the docshell into some place in layout so that it's not tied to the docshell code.

According to jst's suggestion, I want to add some code to limit the totoal number of frame(the html tag not the reflow frame). frame(tag) ---> htmlparser ---> content ---> New a docshell ---> NewURI (according to the src= .. in <frame src=..>) ---> network(Necko)---> a new page. The frame tags merged with other html tags, I was hard to limit the number of frame tag independently.so, I was inclined to the current method. jst: some suggestions :)

I just talked to JST and what we came up with was checking for this in nsFrameLoader::EnsureDocShell() along with the MAX_DEPTH_CONTENT_FRAMES stuff. This means you'll have to walk the entire content docshell tree to find out how many DocShells there are, but given that we're generally talking about a low number of frames that should be OK. The reason for keeping this out of DocShell is, DocShell is already doing too much stuff and the guys that own it don't want more :) On another note, once we add this check we won't need MAX_SAME_URL_CONTENT_FRAMES anymore (three checks is too many). If you like, you can remove that while you're at it. Not a requirement for this patch though, if you don't want to. Thanks very much for your patience :)

jkeiser&jst: I had the similar idea like you. but i found that the patches for bug8065 and bug136580 could not avoid the recursive loop for some complicated frames like the first testcase of this bug. I want to know whether we can delete the patches(8065 & 136580) from the code and add the patch for this bug.

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

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

jst and jkeiser: Could you accept the new patch? I move the code to the nsFrameLoader.cpp.

Comment on attachment 95086 [details] [diff] [review] A new patch v1.0 Could you declare childCount and i on a different line than retval? They serve different purposes. Also, the check for # of docshells should go above that code, next to the place where we check for depth. That way we don't even waste time creating a docshell.

A New Patch.

Comment on attachment 95189 [details] [diff] [review] patch v1.01 That whole thing needs to be wrapped in the if (parentAsSupports) -- otherwise you could crash. r=jkeiser with that

Comment on attachment 95205 [details] [diff] [review] patch v1.02 r=jkeiser

Comment on attachment 95205 [details] [diff] [review] patch v1.02 Rename CountDocShellChildren() to GetDocshellChildCount() and use 2-spaces for indentation here, not 4-spaces. sr=jst with those changes.

Thanks jst & jkeiser.

In trunk. Marking 'Solved'.

I said rename CoundDocShellChildren() to GetDocShellChildCount(), not GetDocShellChildren(), that name doesn't match what the method does. I corrected this on the trunk already.

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

Hi Jim, you can read this patch.

jkeiser & jst: I don't know whether the patch need r= & sr= before it be checked in OEM_branch.

In OEM branch

Note for reference: bug 193011 will be backing out this patch, and fixing this testcase a different way.