RESOLVED FIXED 9959
REGRESSION: iframes stop rendering after 200th one on successive reloads 
https://bugs.webkit.org/show_bug.cgi?id=9959
Summary REGRESSION: iframes stop rendering after 200th one on successive reloads
David Kilzer (:ddkilzer)
Reported 2006-07-16 10:49:26 PDT
I was working on layout tests for Bug 9947, when I suddenly noticed that iframe content was disappearing after I reloaded the layout test too many times. Steps to reproduce: 1. Load test page.  Set "count" to 1. 2. Click reload browser button 29 more times (until "count" is 30).  Pause between each reload until it is complete. Expected results: The same seven iframes should appear every time the page is reloaded. Actual resutls: On the 29th reload, the bottom three iframes disappear leaving only four iframes.  On the 30th reload (and all subsequent reloads), all seven iframes disappear. Note that I haven't tried reloading much beyond 35 to see if they come back. Regression: Works as expected on production Safari 2.0.4 (419.3) on Mac OS X 10.4.7 (8J135/PowerPC). Notes: Tested on locally-built WebKit r15466 (debug build) with the above versions of Safari and Mac OS X.
Attachments
Content for test case iframes (514 bytes, text/html)
2006-07-16 10:50 PDT, David Kilzer (:ddkilzer) 		no flags
Details
Test case (reload 30 times) (2.29 KB, text/html)
2006-07-16 10:52 PDT, David Kilzer (:ddkilzer) 		no flags
Details
Local test case (much faster; reload 30 times) (1.23 KB, application/zip)
2006-07-16 10:57 PDT, David Kilzer (:ddkilzer) 		no flags
Details
Torture test (reload 3 times) (1.49 KB, application/zip)
2006-07-16 14:22 PDT, David Kilzer (:ddkilzer) 		no flags
Details
reduced test case (378 bytes, text/html)
2006-07-17 09:59 PDT, Alexey Proskuryakov 		no flags
Details
Patch (6.09 KB, patch)
2006-07-17 16:10 PDT, Anders Carlsson 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
David Kilzer (:ddkilzer)
Comment 1 2006-07-16 10:50:44 PDT
Created attachment 9497 [details] Content for test case iframes
David Kilzer (:ddkilzer)
Comment 2 2006-07-16 10:52:17 PDT
Created attachment 9498 [details] Test case (reload 30 times)
David Kilzer (:ddkilzer)
Comment 3 2006-07-16 10:57:02 PDT
Created attachment 9499 [details] Local test case (much faster; reload 30 times) It's much faster to reproduce this locally.
David Kilzer (:ddkilzer)
Comment 4 2006-07-16 10:57:54 PDT
(In reply to comment #0) > Actual resutls: > > On the 29th reload, the bottom three iframes disappear leaving only four > iframes.  On the 30th reload (and all subsequent reloads), all seven iframes > disappear. Actually, the bottom four iframes disappear leaving only the top three on the 29th reload.
David Kilzer (:ddkilzer)
Comment 5 2006-07-16 10:59:28 PDT
(In reply to comment #0) > Steps to reproduce: > > 1. Load test page.  Set "count" to 1. > 2. Click reload browser button 29 more times (until "count" is 30).  Pause > between each reload until it is complete. Where "count" is an internal counter in your brain.
David Kilzer (:ddkilzer)
Comment 6 2006-07-16 11:05:24 PDT
Additional testing note:  This problem seems to be on a per-browser-window (or per-tab) basis since opening a new tab or a new browser window will let you continue loading the same page another 30 times until all of the content disappears.
David Kilzer (:ddkilzer)
Comment 7 2006-07-16 11:10:15 PDT
Tested WebKit nightly r15467 (a release build), but it doesn't exhibit this behavior.  May only be reproducible on debug builds.
John Sullivan
Comment 8 2006-07-16 11:43:13 PDT
Maybe some recursion limit gone wrong?
David Kilzer (:ddkilzer)
Comment 9 2006-07-16 13:52:39 PDT
Note that my history limit is currently maxed out, although I'm not sure if that would make a difference or not when reproducing this bug.  I see the effects of this history limit bug when I start the production Safari after surfing with a ToT WebKit (that fix was committed in r14127) : <rdar://problem/3126419> history load enforces history limit, but deletes the newest instead of oldest items
David Kilzer (:ddkilzer)
Comment 10 2006-07-16 14:08:44 PDT
If you look closely, the test cases I posted start with a <thtml> element instead of an <html> element.  Fixing this does not resolve the problem, though.
David Kilzer (:ddkilzer)
Comment 11 2006-07-16 14:22:46 PDT
Created attachment 9505 [details] Torture test (reload 3 times) This test only requires 3 reloads before iframes just stop appearing (on a debug build).  I simply added a lot more iframes than the original test to make the effect happen faster. Note that Safari beach-balls while this test page loads as well. I also fixed the <thtml> tag in this upload.
Alexey Proskuryakov
Comment 12 2006-07-17 09:03:02 PDT
Apparently, the frame count isn't reset when reloading. HTMLIFrameElement.cpp, line 91: // FIXME: This limit could be higher, but WebKit has some // algorithms that happen while loading which appear to be N^2 or // worse in the number of frames if (w->frame()->page()->frameCount() >= 200) return false;
Alexey Proskuryakov
Comment 13 2006-07-17 09:59:05 PDT
Created attachment 9523 [details] reduced test case HTMLIFrameElement::willRemove() is the only place calling decrementFrameCount(), and it isn't invoked during reload. I have no idea about who is supposed to call it.
Darin Adler
Comment 14 2006-07-17 11:21:56 PDT
Anders has been working in this area recently. He might be someone good to fix this.
Anders Carlsson
Comment 15 2006-07-17 16:10:44 PDT
Created attachment 9534 [details] Patch
Darin Adler
Comment 16 2006-07-17 16:52:58 PDT
Comment on attachment 9534 [details] Patch The only thing I don't like about this is that the name disconnectOwnerElement becomes less appropriate if the function does anything else other than clearing the owner element field. r=me
Anders Carlsson
Comment 17 2006-07-18 13:16:36 PDT
Committed revision 15506.
Darin Adler
Comment 18 2006-07-24 11:09:54 PDT
I know this sounds crazy, but the patch for this bug is causing a measurable performance regression on the iBench HTML benchmark, somewhere in the 5-15% range.
Geoffrey Garen
Comment 19 2006-07-24 11:49:58 PDT
I'm investigating the performance regression.
Anders Carlsson
Comment 20 2006-07-24 12:12:39 PDT
The only significant (well, less non-significant) change that I can see is that we always set the ownerElement to 0 in the destructor with the patch. Might be worth to investigate rolling that part out (replacing the call to disconnectOwnerElement with one that just decrements the frame count)
David Kilzer (:ddkilzer)
Comment 21 2007-06-24 12:04:44 PDT
Mass removal of NeedsRadar keyword from my bugs that have already been RESOLVED.
Note You need to log in before you can comment on or make changes to this bug.