Bug 6751 – For unnamed frames, window.name returns a generated name

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

[Alexey Proskuryakov]

The generated names look like "<!--framePath //<!--frame0-->-->". Other browsers just return the original empty name.

[Alexey Proskuryakov]

Created attachment 5900 [details]
test case

[Daniel Bates]

Created attachment 71356 [details]
Patch with updated test case (Work-in-progress)

Work-in-progress patch.

[Adam Barth]

Comment on attachment 71356 [details]
Patch with updated test case (Work-in-progress)

I don't really understand the difference between name and domName.  What use is a name if not for the DOM?

[Daniel Bates]

Created attachment 71377 [details]
Patch with updated test case (Work-in-progress)

Work-in-progress patch.

Re-purpose FrameTree::name() to return the actual name of the frame as per the HTML5 spec. Added method FrameTree::uniqueName() to return a unique frame name (may be generated).

[Daniel Bates]

Created attachment 71666 [details]
Patch with updated test case

Updated change log.

For the the port-specific API changes in this patch, I chose to preserve the current behavior of returning the unique frame name. We may want to consider changing the API behavior so that it returns the actual name of the frame, which may be the empty string.

One concern I had was regressing the fix for bug 12732, which was landed in changeset 24396 <http://trac.webkit.org/changeset/24396>. The test case landed in this changeset <http://trac.webkit.org/browser/trunk/LayoutTests/fast/frames/iframe-set-inner-html.html> used frame name uniqueness for two unnamed frames to verify the fix. Clearly, this approach conflicts with this patch and the HTML5 spec with regards to frame names.

Further investigation is needed to be able to devise an alternative test for bug 12732. Alternatively, we could expose a DRT method that returns the internal unique name for a frame and modify the test case to use this method. Does anyone have any additional insight/suggestions into bug 12732 and its test case?

[Adam Barth]

Comment on attachment 71666 [details]
Patch with updated test case

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

> WebCore/page/FrameTree.cpp:45
> +    m_name = name;
>      if (!parent()) {
> -        m_name = name;
> +        m_uniqueName = name;

Is m_uniqueName guaranteed to be unique?  If I explicitly set the name of two frames, can I cause a collision?

[Daniel Bates]

(In reply to comment #6)
> (From update of attachment 71666 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71666&action=review
> 
> > WebCore/page/FrameTree.cpp:45
> > +    m_name = name;
> >      if (!parent()) {
> > -        m_name = name;
> > +        m_uniqueName = name;
> 
> Is m_uniqueName guaranteed to be unique?  If I explicitly set the name of two frames, can I cause a collision?

Yes, m_uniqueName is guaranteed to be unique by the if-condition in FrameTree::uniqueName() <http://trac.webkit.org/browser/trunk/WebCore/page/FrameTree.cpp?rev=60287#L104> and FrameTree::child(const AtomicString& name) <http://trac.webkit.org/browser/trunk/WebCore/page/FrameTree.cpp?rev=60287#L159>.

[Adam Barth]

Comment on attachment 71666 [details]
Patch with updated test case

Great.  Thanks for writing this patch.

[Daniel Bates]

Committed r70971: <http://trac.webkit.org/changeset/70971>

[Daniel Bates]

(In reply to comment #9)
> Committed r70971: <http://trac.webkit.org/changeset/70971>

This caused layout test failures. Rolled out patch in changeset 70976 <http://trac.webkit.org/changeset/70976> while I look into this further.

[Mihai Parparita]

Regarding "We should consider exposing a DRT method for obtaining the internal frame name (which
may be generated) and re-implementing this test to use it.", there's already layoutTestController.dumpChildFramesAsText() which exposes internal names.

[Mihai Parparita]

Regarding naming, name() vs. uniqueName() seems confusing, and it also makes it easier to miss switching a callsite from name() to uniqueName() with in your patch. domName() and internalUniqueName() may be better, or perhaps domName() and internalID().

[Adam Barth]

(In reply to comment #12)
> Regarding naming, name() vs. uniqueName() seems confusing, and it also makes it easier to miss switching a callsite from name() to uniqueName() with in your patch. domName() and internalUniqueName() may be better, or perhaps domName() and internalID().

Generally, we try to name things with their DOM names, so we'll want this method to be called name().  However, landing a patch using domName() and then changing it to name() might be a good way of proving that we're correctly changed all the call sites.

[Daniel Bates]

(In reply to comment #13)
> (In reply to comment #12)
> > Regarding naming, name() vs. uniqueName() seems confusing, and it also makes it easier to miss switching a callsite from name() to uniqueName() with in your patch. domName() and internalUniqueName() may be better, or perhaps domName() and internalID().
> 
> Generally, we try to name things with their DOM names, so we'll want this method to be called name().  However, landing a patch using domName() and then changing it to name() might be a good way of proving that we're correctly changed all the call sites.

With the passing of bug #48806, I plan to retry landing this patch in at least two parts based on Adam's suggestion later tonight.

Also, based on Mihai Parparita's feedback I will also land an updated iframe-set-inner-html.html test that uses layoutTestController.dumpChildFramesAsText().

[Daniel Bates]

Split the patch into two parts: 1) substitute FrameTree::uniqueName() for FrameTree::name() and 2) Re-purpose FrameTree::name() for the DOM-specific name and implement support for the empty string as a valid frame name.

Committed (1) in changeset 71216 <trac.webkit.org/changeset/71216> and changeset 71217 <http://trac.webkit.org/changeset/71217>.

Committed (2) in changeset 71219 <http://trac.webkit.org/changeset/71219>.

[WebKit Review Bot]

http://trac.webkit.org/changeset/71219 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
svg/css/getComputedStyle-basic.xhtml

[Daniel Bates]

(In reply to comment #16)
> http://trac.webkit.org/changeset/71219 might have broken GTK Linux 64-bit Debug
> The following tests are not passing:
> svg/css/getComputedStyle-basic.xhtml

This failure is not related to this bug. Using the build bot console view <http://build.webkit.org/console> this test started failing after the patch for bug #48608 <https://bugs.webkit.org/show_bug.cgi?id=48608> was landed in changeset  71218 <http://trac.webkit.org/changeset/71218>.