Bug 6751 - For unnamed frames, window.name returns a generated name
Summary: For unnamed frames, window.name returns a generated name
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: HasReduction
Depends on: 48806
Blocks: 48768
  Show dependency treegraph
 
Reported: 2006-01-23 21:49 PST by Alexey Proskuryakov
Modified: 2010-11-03 02:22 PDT (History)
11 users (show)

See Also:


Attachments
test case (4.01 KB, application/zip)
2006-01-23 21:50 PST, Alexey Proskuryakov
no flags Details
Patch with updated test case (Work-in-progress) (9.24 KB, patch)
2010-10-20 16:32 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with updated test case (Work-in-progress) (27.42 KB, patch)
2010-10-20 18:14 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with updated test case (27.61 KB, patch)
2010-10-23 21:00 PDT, Daniel Bates
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2006-01-23 21:49:47 PST
The generated names look like "<!--framePath //<!--frame0-->-->". Other browsers just return the original empty name.
Comment 1 Alexey Proskuryakov 2006-01-23 21:50:17 PST
Created attachment 5900 [details]
test case
Comment 2 Daniel Bates 2010-10-20 16:32:45 PDT
Created attachment 71356 [details]
Patch with updated test case (Work-in-progress)

Work-in-progress patch.
Comment 3 Adam Barth 2010-10-20 16:34:21 PDT
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?
Comment 4 Daniel Bates 2010-10-20 18:14:38 PDT
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).
Comment 5 Daniel Bates 2010-10-23 21:00:54 PDT
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?
Comment 6 Adam Barth 2010-10-27 13:14:15 PDT
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?
Comment 7 Daniel Bates 2010-10-27 15:14:52 PDT
(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>.
Comment 8 Adam Barth 2010-10-27 15:28:57 PDT
Comment on attachment 71666 [details]
Patch with updated test case

Great.  Thanks for writing this patch.
Comment 9 Daniel Bates 2010-10-29 20:49:36 PDT
Committed r70971: <http://trac.webkit.org/changeset/70971>
Comment 10 Daniel Bates 2010-10-29 23:15:18 PDT
(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.
Comment 11 Mihai Parparita 2010-10-30 09:07:18 PDT
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.
Comment 12 Mihai Parparita 2010-10-30 09:15:08 PDT
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().
Comment 13 Adam Barth 2010-10-30 12:48:21 PDT
(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.
Comment 14 Daniel Bates 2010-11-02 10:29:34 PDT
(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().
Comment 15 Daniel Bates 2010-11-03 02:14:21 PDT
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>.
Comment 16 WebKit Review Bot 2010-11-03 02:15:28 PDT
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
Comment 17 Daniel Bates 2010-11-03 02:22:12 PDT
(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>.