Bug 48768 - Transferred <iframe>s may not have a unique internal name
Summary: Transferred <iframe>s may not have a unique internal name
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on: 6751
Blocks: 49489
  Show dependency treegraph
 
Reported: 2010-11-01 11:18 PDT by Daniel Bates
Modified: 2010-11-12 22:47 PST (History)
7 users (show)

See Also:


Attachments
Example (1.48 KB, text/html)
2010-11-01 11:49 PDT, Daniel Bates
no flags Details
update internal frame name on reparenting (4.13 KB, patch)
2010-11-02 14:46 PDT, Jenn Braithwaite
dbates: review-
dbates: commit-queue-
Details | Formatted Diff | Diff
[Patch] Work-in-progress (895 bytes, patch)
2010-11-02 14:56 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with layout test (6.19 KB, patch)
2010-11-03 18:59 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with layout test (8.22 KB, patch)
2010-11-03 20:19 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2010-11-01 11:18:16 PDT
<iframe>s that are transferred from document A to document B, where A != B, may not have a unique internal name in B.
Comment 1 Daniel Bates 2010-11-01 11:49:34 PDT
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).
Comment 2 Daniel Bates 2010-11-01 11:52:41 PDT
To run the example, download it locally. Then open it in Safari.
Comment 3 Jenn Braithwaite 2010-11-02 14:46:40 PDT
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.
Comment 4 Daniel Bates 2010-11-02 14:53:47 PDT
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-->.
Comment 5 Daniel Bates 2010-11-02 14:56:57 PDT
Created attachment 72743 [details]
[Patch] Work-in-progress

Work in progress patch.
Comment 6 Daniel Bates 2010-11-03 18:59:14 PDT
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.
Comment 7 Daniel Bates 2010-11-03 20:19:58 PDT
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.
Comment 8 Adam Barth 2010-11-04 15:05:36 PDT
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?
Comment 9 Dmitry Titov 2010-11-04 15:12:56 PDT
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.
Comment 10 Daniel Bates 2010-11-04 15:26:09 PDT
(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.
Comment 11 Daniel Bates 2010-11-04 15:38:52 PDT
(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().
Comment 12 Dmitry Titov 2010-11-04 16:22:50 PDT
> 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...
Comment 13 Daniel Bates 2010-11-04 16:40:39 PDT
(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.
Comment 14 Daniel Bates 2010-11-04 16:52:14 PDT
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)
Comment 15 Dmitry Titov 2010-11-04 17:21:45 PDT
(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+ :-)
Comment 16 Jenn Braithwaite 2010-11-04 20:46:53 PDT
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.
Comment 17 Dmitry Titov 2010-11-12 13:46:18 PST
(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.
Comment 18 Daniel Bates 2010-11-12 21:43:54 PST
(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.
Comment 19 Daniel Bates 2010-11-12 21:46:53 PST
(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.
Comment 20 Daniel Bates 2010-11-12 22:01:54 PST
(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.
Comment 21 Daniel Bates 2010-11-12 22:46:55 PST
Comment on attachment 72901 [details]
Patch with layout test

Clearing flags on attachment: 72901

Committed r71962: <http://trac.webkit.org/changeset/71962>
Comment 22 Daniel Bates 2010-11-12 22:47:04 PST
All reviewed patches have been landed.  Closing bug.