Bug 39427 – Increase limit on number of (i)frames from 200 to 1000.

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Hans Wennborg]

Increase limit on number of (i)frames from 200 to 1000.

[Hans Wennborg]

Created attachment 56596 [details]
Patch

[Hans Wennborg]

Note: The attached layout test tends to time out on Chromium debug builds, so it may have to be marked as slow. I am not sure how important it is to include a layout test for this at all.

(This discussion started at http://crbug.com/16184)

[Darin Fisher (:fishd, Google)]

what's the justification for this change?

[Hans Wennborg]

Justification is in the patch to WebCore/ChangeLog. Should I also have put it in as a bug comment? (I'm new here.)


In my testing, both Chrome and Safari handles this well. I tried to find a limit on number of frames in IE and Firefox, but could not find one. User experience gets pretty bad with huge amounts of iframes though (and FF crashed trying to render 12k iframes), so having some limit seems reasonable.

The limit of 200 seemed harsh though. For example, the Vim color scheme test page (http://vimcolorschemetest.googlecode.com/svn/html/index-c.html) is broken in WebKit browsers.

In an IRC chat (excerpt on http://crbug.com/16184) between jorlow and mjs, it was said that the O(N^2) algorithms referred to in the original patch (http://trac.webkit.org/changeset/3707) as the reason for this limit are not used anymore.

So since the underlying reason for this limit is said to be gone, some websites would be un-broken by increasing it, and testing shows that WebKit handles it well, I think we should consider bumping the limit to 1000.

[Alexey Proskuryakov]

It would be nice to make this a duplicate of bug 32766, and discuss the change there.

Is there code that assumes that frame number is no longer than 3 decimal digits? Some code and comments in FrameTree::uniqueChildName() come alarmingly close, give or take an off-by-one error somewhere.

[Jeremy Orlow]

Oops.  I guess we should have searched the bug archive better before filing this.  Given that there's already a patch here and this bug is about the specific problem, I'm going to dup that to this.

Hans, can you trace through the code AP mentioned to see if it's an actual issue?  Even if we think it's OK, it might make sense to make it 990 or something to ensure we don't hit any weird limits.  And if there is a real issue, it'd be great if we could add an ASSERT and/or layout test to ensure no one in the future makes that mistake without realizing it.

[Jeremy Orlow]

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

[Alexey Proskuryakov]

> this bug is about the specific problem, I'm going to dup that to this.

Actually, it's bug 32766 that was about a specific problem on a specific site, which is why I liked it and its title more. But that's not a big thing.

[Darin Fisher (:fishd, Google)]

Comment on attachment 56596 [details]
Patch

WebCore/html/HTMLFrameElementBase.cpp:73
 +      // worse in the number of frames, we'll keep it at 200 for now.
nit: update the comment from 200 to 1000

[Hans Wennborg]

Looking at FrameTree::uniqueChildName(), it seems there is head room for 20+ digit frame numbers (FrameTree.cpp:144). The comment just says three digits is the highest it gets in practice, but any unsigned 64-bit number will be fine. If we change the limit to 1000 or higher, we should update that comment, though.

I have been investigating other users of this number:
WebKit/mac/WebView/WebFrame.mm:1562
WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1094
WebCore/page/DOMWindow.cpp:1034
WebCore/loader/archive/cf/LegacyWebArchive.cpp:444
WebCore/loader/HistoryController.cpp:607
WebCore/bindings/js/JSDOMWindowCustom.cpp:{208,354}

None of these make any dangerous assumptions about the size of this number that I can see. The m_childCount member variable is not exposed other than through childCount(), and not used in any alarming way internally.

Then there is Page::frameCount(). Again, I have not been able to find uses that make dangerous assumptions about the size of that number.

RenderEmbeddedObject::isURLAllowed() looks to see if Page::frameCount() is < 200. It seems to me that it should use the same limit as HTMLFrameElementBase. Please correct me if I am mistaken here. And as the same number is used in these two places, it would make sense to introduce a constant for the limit somewhere (would Page be a good place?).


One idea would be to raise the limit to 1111 frames, with the purpose of exposing any potential problems I've missed and hoping the build bots or nightly build users will trigger it.

[Alexey Proskuryakov]

Comment on attachment 56596 [details]
Patch

> RenderEmbeddedObject::isURLAllowed() looks to see if 
> Page::frameCount() is < 200. It seems to me that it should use the
> same limit as HTMLFrameElementBase.

I don't know what this check is about, but it definitely seems so! Now I wonder what was broken due to not having it fixed, too. Ideally, we would have a test case that fails with the current version of your patch, but not with a final one.

> One idea would be to raise the limit to 1111 frames

Having a round number like 999, 1000, or 2000 is better in that it's easier to understand what's going on when we hit the limit.

Marking r- to get this out of review queue, since some modifications are clearly needed, as well as some investigation.

[Jeremy Orlow]

(In reply to comment #11)
> (From update of attachment 56596 [details])
> > RenderEmbeddedObject::isURLAllowed() looks to see if 
> > Page::frameCount() is < 200. It seems to me that it should use the
> > same limit as HTMLFrameElementBase.
> 
> I don't know what this check is about, but it definitely seems so! Now I wonder what was broken due to not having it fixed, too. Ideally, we would have a test case that fails with the current version of your patch, but not with a final one.

Agreed.  Hans, you should probably first look at the ChangeLog associated with this limit being added in.  Hopefully that'll make it clear why.  If not, maybe you can do some investigating including contacting the author and/or ccing him/her.

[Hans Wennborg]

The check in RenderEmbeddedObject::isURLAllowed() was introduced in http://trac.webkit.org/changeset/11162. (That code has since migrated through revisions r11966, r12155, r14334 and r52947.)

The way I read that ChangeLog is that depending on the circumstances, there were code paths where the other check for number of frames (the one introduced in r3707) was circumvented, and this was a fix for that. As I understand it, it does the same check and has the same purpose as in HTMLFrameElementBase::isURLAllowed().

Eric and Maciej wrote and reviewed that patch. Perhaps they can shed some light on it?

What would have been "broken" by increasing the limit in HTMLFrameElementBase::isURLAllowed() and not in RenderEmbeddedObject::isURLAllowed() is that a page would be able to have 200+ iframes, but not 200+ embedded objects.

Uploading new patch.

[Hans Wennborg]

Created attachment 56899 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 56899 [details]
Patch

WebCore/page/Page.h:240
 +          static const int m_maxNbrOfFrames = 1000;
style nit: spell out words and reserve m_ for private members:
m_maxNbrOfFrames -> maxNumberOfFrames

LayoutTests/compositing/iframes/too-many-objects.html:9
 +    var created_objects = 0;
nit: use camelCase for variables

LayoutTests/compositing/iframes/too-many-objects.html:26
 +    {
nit: be consistent with braces

LayoutTests/compositing/iframes/too-many-objects.html:25
 +    function createIFrames(nbr)
nit: avoid abbreviations like "nbr"

nit: it seems like you could combine lots-of-iframes and too-many-iframes.
lots-of-iframes is testing that you can create 1000 frames, and too-many-iframes
is testing that once you have 1000 frames, you can't create one more.  they seem
like they could just be part of a single test that asserts that 1000 is the limit
for the number of tests you can create.  same goes for the object tag.

[Hans Wennborg]

Thanks for the input.

(In reply to comment #15)
> (From update of attachment 56899 [details])
> WebCore/page/Page.h:240
>  +          static const int m_maxNbrOfFrames = 1000;
> style nit: spell out words and reserve m_ for private members:
> m_maxNbrOfFrames -> maxNumberOfFrames
Done.
 
> LayoutTests/compositing/iframes/too-many-objects.html:9
>  +    var created_objects = 0;
> nit: use camelCase for variables
Done.

> LayoutTests/compositing/iframes/too-many-objects.html:26
>  +    {
> nit: be consistent with braces
Done.
 
> LayoutTests/compositing/iframes/too-many-objects.html:25
>  +    function createIFrames(nbr)
> nit: avoid abbreviations like "nbr"
Done.

> nit: it seems like you could combine lots-of-iframes and too-many-iframes.
> lots-of-iframes is testing that you can create 1000 frames, and too-many-iframes
> is testing that once you have 1000 frames, you can't create one more.  they seem
> like they could just be part of a single test that asserts that 1000 is the limit
> for the number of tests you can create.  same goes for the object tag.
Done.

Uploading new patch.

[Hans Wennborg]

Created attachment 57002 [details]
Patch

[Jeremy Orlow]

Any remaining reservations with this?

[Alexey Proskuryakov]

Comment on attachment 57002 [details]
Patch

Looks fine, r=me.

[WebKit Commit Bot]

Comment on attachment 57002 [details]
Patch

Clearing flags on attachment: 57002

Committed r60287: <http://trac.webkit.org/changeset/60287>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

The tests were added in the wrong place. They have nothing to do with compositing.

[Jeremy Orlow]

Sorry, I should have caught that.  Hans, can you please try to find a better place and open a new bug to fix that?

[Alexey Proskuryakov]

Moved the tests to fast/frames in <http://trac.webkit.org/changeset/61199>.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61199 might have broken Qt Linux Release

[Alexey Proskuryakov]

And even better in <http://trac.webkit.org/changeset/61205> :-(

[Hans Wennborg]

Thanks Alexey.