Bug 9959

Summary:

REGRESSION: iframes stop rendering after 200th one on successive reloads

Product:

WebKit

Reporter:

David Kilzer (:ddkilzer) <ddkilzer>

Component:

Frames

Assignee:

Anders Carlsson <andersca>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, ap, darin, hyatt, mjs

Priority:

Keywords:

HasReduction, Regression

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

Attachments:

Description	Flags
Content for test case iframes	none
Test case (reload 30 times)	none
Local test case (much faster; reload 30 times)	none
Torture test (reload 3 times)	none
reduced test case	none
Patch	darin: review+

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+	Details Formatted Diff Diff
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.