Bug 48768 – Transferred <iframe>s may not have a unique internal name

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

<iframe>s that are transferred from document A to document B, where A != B, may not have a unique internal name in B.

[Daniel Bates]

Created attachment 72534 [details]
Example

This example uses a known side effect of bug #6751. That is, we can get the internal WebKit frame name of a frame via its contentWindow.name property.

Assuming that each frame in a document has a unique internal name.

Using this example we can demonstrate that two <iframe>s in the same document can have the same internal name.

Let A be the frame in document D_0 that contains a frame F_1 whose name is "Frame1" and whose content is "Frame1". Notice, F_1 is in the document of A, call it D_A.
When you run the example, a frame F_2 in document D_0  whose name is "Frame1" and whose content is "Frame2" is adopted by A such that A has two <iframe>s, F_1 and F_2; => F_1 and F_2 are in D_A. But the name of F_2 == "Frame1" ==  name of F_1 (contradicts names uniqueness).

[Daniel Bates]

To run the example, download it locally. Then open it in Safari.

[Jenn Braithwaite]

Created attachment 72739 [details]
update internal frame name on reparenting

In Frame.cpp, the line above my change that calls m_ownerElement->setName() will no longer be needed once bug 6751 is fixed as that frame name will not need to be updated for uniqueness upon reparenting.

Example test case added as layout test in this patch.

[Daniel Bates]

Comment on attachment 72739 [details]
update internal frame name on reparenting

View in context: https://bugs.webkit.org/attachment.cgi?id=72739&action=review

I have a work in progress patch for this issue and was planned to address it after I land bug #6751.

> WebCore/page/Frame.cpp:738
> +            tree()->setName(tree()->name());

Because of <https://bugs.webkit.org/show_bug.cgi?id=48806#c0> we can't call setName() after appendChild() since the child count will be off. Disregarding the frame prefix, notice in your results the unique name has <!--frame2-->, but we count from zero, so this should be <!--frame1-->.

[Daniel Bates]

Created attachment 72743 [details]
[Patch] Work-in-progress

Work in progress patch.

[Daniel Bates]

Created attachment 72896 [details]
Patch with layout test

I thought to move the child transfer code from Frame::transferChildFrameToNewDocument() to FrameTree::transferChild() so that we can set ensure that the name of the transferred frame has a unique name with respect to its new parent. I am open to suggestions on the name of this function and its return type as well as the approach of this patch.

[Daniel Bates]

Created attachment 72901 [details]
Patch with layout test

Updated patch that removes the methods HTMLFrameElementBase::setName() and HTMLFrameOwnerElement::setName(). Also, re-worded description in layout test to imply that this test only works in DRT.

[Adam Barth]

Comment on attachment 72901 [details]
Patch with layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=72901&action=review

> WebCore/page/FrameTree.cpp:75
> +    ASSERT(child->page() == m_thisFrame->page());

Why is this true?  You can't transfer frames between pages?

[Dmitry Titov]

It probably would be good to split FrameTree::setName() into setName() and updateUniqueName(), since it is a mechanical combination of two and apparently those two are not always to be called in a lockstep.

For example, various 'createChildFrame' methods in WebKit layer typically have a new frame name, so they call setName() before appendChild(). With this patch, we'll call setName() again inside appendChild, passing the same name to itself. For example, WebKit/mac/WebView/WebFrame.mm, _createFrameWithPage.

Instead, we could call setName() in createFrameWithPage and friends, and simply updateUniqueName() from appendChild. It might make code clearer.

[Daniel Bates]

(In reply to comment #8)
> (From update of attachment 72901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72901&action=review
> 
> > WebCore/page/FrameTree.cpp:75
> > +    ASSERT(child->page() == m_thisFrame->page());
> 
> Why is this true?  You can't transfer frames between pages?

I will look into this further and respond shortly.

[Daniel Bates]

(In reply to comment #9)
> It probably would be good to split FrameTree::setName() into setName() and updateUniqueName(), since it is a mechanical combination of two and apparently those two are not always to be called in a lockstep.
> 
> For example, various 'createChildFrame' methods in WebKit layer typically have a new frame name, so they call setName() before appendChild(). With this patch, we'll call setName() again inside appendChild, passing the same name to itself. For example, WebKit/mac/WebView/WebFrame.mm, _createFrameWithPage.
> 

Notice, neither FrameTree::appendChild() nor FrameTree::actuallyAppendChild() (added in this patch) calls FrameTree::setName(). And Frame::transferChildFrameToNewDocument() is the only call site that needs to have the child's unique name updated (since we are transferring it to a new parent, which may already contain a frame with an identical name).

> Instead, we could call setName() in createFrameWithPage and friends, and simply updateUniqueName() from appendChild. It might make code clearer.

As aforementioned, appendChild() does not update the unique name. We could factor our the unique name updating from FrameTree::setName(), but we don't always have to update the unique frame name so I'm unclear of the benefit given the current call sites of FrameTree::setName().

[Dmitry Titov]

> As aforementioned, appendChild() does not update the unique name. We could factor our the unique name updating from FrameTree::setName(), but we don't always have to update the unique frame name so I'm unclear of the benefit given the current call sites of FrameTree::setName().

That's right, sorry I was not clear.. What I should have said was we could split those, perhaps call it generateUniqueName() rather, and only call it inside appendChild(), before incrementing the child counter, since it seems this is the only place that effectively establishes the computed unique name (other then case which where there is no parent).

Then there is no need for actuallyAppendChild(), and transferChild(), can simply call appendChild() to set parent, update name and do the rest of appending, if I don't miss something...

[Daniel Bates]

(In reply to comment #12)
> > As aforementioned, appendChild() does not update the unique name. We could factor our the unique name updating from FrameTree::setName(), but we don't always have to update the unique frame name so I'm unclear of the benefit given the current call sites of FrameTree::setName().
> 
> That's right, sorry I was not clear.. What I should have said was we could split those, perhaps call it generateUniqueName() rather, and only call it inside appendChild(), before incrementing the child counter, since it seems this is the only place that effectively establishes the computed unique name (other then case which where there is no parent).
> 
> Then there is no need for actuallyAppendChild(), and transferChild(), can simply call appendChild() to set parent, update name and do the rest of appending, if I don't miss something...

Notice, FrameTree::uniqueChildName() calls FrameTree::child() <http://trac.webkit.org/browser/trunk/WebCore/page/FrameTree.cpp?rev=71219#L107>, which enforces frame name uniqueness by traversing the frame tree for a frame with the requested name.

I decided against having FrameTree::appendChild() update the name of the child because Frame::transferChildFrameToNewDocument() is the only code path where we may have to generate a new unique name for a frame (*), and I was unclear whether the performance implications of such a change would be negligible. I could try measuring the performance of this change.

[Daniel Bates]

I forgot to add my remark (*):

(*) Currently, the code assumes a convention of setting the name of a frame before appending it to the tree. This conventions ensures that both the name of the frame is unique and that the generated name reflects its placement in the frame tree hierarchy with respect to its parent (notice, generated frame names are of the form <!--frame0-->/<!--frame1-->, where <!--frame0--> is the parent of <!--frame1-->).

Ideally, we should enforce this convention; maybe through some kind of ASSERT(s); introduce a function that both sets the name and appends it to the tree, say FrameTree::setNameAndAppendChild(), or reduce the call sites that call both FrameTree::setName() and FrameTree::appendChild() (for example, these functions come up in the platform-specific FrameLoaderClient code for every port; we could try to factor out this code into some common WebCore function as a means to both reduce duplicate code and prevent a violation of the convention)

[Dmitry Titov]

(In reply to comment #14)

Thanks for explanation! Indeed, in some cases there can be perf impact since having name generated in appendChild may be unnecessary in some cases. I've coded what I had in mind, and didn't find it much clearer as well.

I second Adam's r+ :-)

[Jenn Braithwaite]

Comment on attachment 72901 [details]
Patch with layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=72901&action=review

> WebCore/page/Frame.cpp:730
> +    if (newParent) {

The old code removed the frame from oldParent even if newParent is NULL.  The new code does not.

>>> WebCore/page/FrameTree.cpp:75
>>> +    ASSERT(child->page() == m_thisFrame->page());
>> 
>> Why is this true?  You can't transfer frames between pages?
> 
> I will look into this further and respond shortly.

The only code path that calls this method has already updated the page before updating the FrameTree.

[Dmitry Titov]

(In reply to comment #16)
> (From update of attachment 72901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72901&action=review
> 
> > WebCore/page/Frame.cpp:730
> > +    if (newParent) {
> 
> The old code removed the frame from oldParent even if newParent is NULL.  The new code does not.

Indeed, the iframe can be transferred into a document that does not have a frame. I think a document created by XHR is one example.

[Daniel Bates]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 72901 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=72901&action=review
> > 
> > > WebCore/page/Frame.cpp:730
> > > +    if (newParent) {
> > 
> > The old code removed the frame from oldParent even if newParent is NULL.  The new code does not.
> 
> Indeed, the iframe can be transferred into a document that does not have a frame. I think a document created by XHR is one example.

It is true that an <iframe> can be transferred into a document that does not have a frame.

But it is not possible for newParent to be null in Frame::transferChildFrameToNewDocument() based on the the call order of HTMLFrameElementBase::setRemainsAliveOnRemovalFromTree() and Frame::transferChildFrameToNewDocument(). See the description of bug #49489 <https://bugs.webkit.org/show_bug.cgi?id=49489#c0> for more details.

[Daniel Bates]

(In reply to comment #16)
> (From update of attachment 72901 [details])
> >>> WebCore/page/FrameTree.cpp:75
> >>> +    ASSERT(child->page() == m_thisFrame->page());
> >> 
> >> Why is this true?  You can't transfer frames between pages?
> > 
> > I will look into this further and respond shortly.
> 
> The only code path that calls this method has already updated the page before updating the FrameTree.

From talking with Adam Barth on IRC yesterday (11/12), he is satisfied with this answer.

[Daniel Bates]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (From update of attachment 72901 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=72901&action=review
> > > 
> > > > WebCore/page/Frame.cpp:730
> > > > +    if (newParent) {
> > > 
> > > The old code removed the frame from oldParent even if newParent is NULL.  The new code does not.
> > 
> > Indeed, the iframe can be transferred into a document that does not have a frame. I think a document created by XHR is one example.
> 
> It is true that an <iframe> can be transferred into a document that does not have a frame.

I mean't to say, that an <iframe> can be adopted by another document D' and appended to D'. That is, I didn't mean to imply "transferred" as in Frame::transferChildFrameToNewDocument() is called.

[Daniel Bates]

Comment on attachment 72901 [details]
Patch with layout test

Clearing flags on attachment: 72901

Committed r71962: <http://trac.webkit.org/changeset/71962>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.