Bug 23900 - Arbitrary frame names should be supported, even duplicate or empty
Summary: Arbitrary frame names should be supported, even duplicate or empty
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 23899 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-02-11 12:24 PST by Sverrir Á. Berg
Modified: 2010-06-10 17:09 PDT (History)
4 users (show)

See Also:


Attachments
Prevent frames from naming themselves _top (1.35 KB, patch)
2009-02-11 12:28 PST, Sverrir Á. Berg
ap: review-
Details | Formatted Diff | Diff
New patch (4.15 KB, patch)
2009-02-18 07:35 PST, Sverrir Á. Berg
ap: review-
Details | Formatted Diff | Diff
test _top (121 bytes, text/html)
2009-02-19 02:03 PST, Alexey Proskuryakov
no flags Details
The original crash reduction (82 bytes, text/html)
2009-03-01 13:11 PST, Dimitri Glazkov (Google)
no flags Details
The right way to fix this (19.15 KB, patch)
2009-03-20 10:11 PDT, Sverrir Á. Berg
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sverrir Á. Berg 2009-02-11 12:24:44 PST
Fixes a crash.  See:
https://bugs.webkit.org/show_bug.cgi?id=23899
Comment 1 Sverrir Á. Berg 2009-02-11 12:28:16 PST
Created attachment 27568 [details]
Prevent frames from naming themselves _top

This was the only logical place I thought of fixing this.  Otherwise FrameLoader manages to call itself recursively and ends up loading the new frame ontop of the first one.
Playing around with other browsers it seems that _top is not allowed as a frame name so this should not break any webpages.
Comment 2 Alexey Proskuryakov 2009-02-17 02:51:29 PST
*** Bug 23899 has been marked as a duplicate of this bug. ***
Comment 3 Alexey Proskuryakov 2009-02-17 02:58:10 PST
Comment on attachment 27568 [details]
Prevent frames from naming themselves _top

+        WARNING: NO TEST CASES ADDED OR CHANGED

The fix looks reasonable, but it definitely needs an automated test case, r- because of that. Note that I couldn't easily make Safari crash with the code provided in bug 23899.

+	Make sure a frame does not name itself "_top".  This prevents a crash.

Please fix ChangeLog to use spaces, not tabs.

A better place for this line is in the generated part of ChangeLog:

+        * page/FrameTree.cpp:
+        (WebCore::FrameTree::uniqueChildName):
Comment 4 Eric Seidel (no email) 2009-02-17 11:54:39 PST
Comment on attachment 27568 [details]
Prevent frames from naming themselves _top

It seems we need a test case for _top, _parent, _blank, _self (and any other special names).

If I were writing this code, I might even consider breaking this if out into a static inline function.

if (isAllowedChildName(this, requestedName))
    return requestedName;

static isAllowedChildName(FrameTree* tree, const AtomicString& name)
{
if (name.isEmpty() || child(name))
    return false;

if (name == "_blank" || name == "_top" || name == "_parent" || name == "_self")
  return false;

return true;
}
Comment 5 Sverrir Á. Berg 2009-02-18 07:35:58 PST
Created attachment 27749 [details]
New patch

I've abstracted the check to a member function and widened the check to all invalid frame names (all starting with _).  I've also added a test.
Note that the test works fine on webkit trunk on Mac but crashes Win Nightly.

Please take another look
Comment 6 Alexey Proskuryakov 2009-02-19 01:58:34 PST
Comment on attachment 27749 [details]
New patch

What do other browsers do with e.g. "_foobar"? Please check, because being more restrictive than others is dangerous. Have you checked that the name "_top" is ignored by other browsers?

The test doesn't need to dump pixels, but it needs to have some text explaining what's going on (the title alone isn't enough). This may be easier to achieve with IFrame. E.g.

<p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=23900">bug 23900</a>: A frame named _top crashes the browser.</p><p>PASS if no crash.</p>
<script>
if (window.layoutTestController)
     layoutTestController.dumpAsText();
</script>
<iframe name="_top" src="about:blank"></iframe>

The function allowedChildName() should be a static in cpp file - it doesn't use data members, so there is no reason to make it a private member. We start such function names with "is":

static bool isAllowedChildName(const AtomicString& name)
{...}
Comment 7 Alexey Proskuryakov 2009-02-19 02:03:03 PST
Created attachment 27782 [details]
test _top

Actually, I've just checked, and in Firefox, the name _top seems to be honored, see the attached test.
Comment 8 Dimitri Glazkov (Google) 2009-02-28 20:17:06 PST
This is has been fixed by http://trac.webkit.org/changeset/41213.
Comment 9 Alexey Proskuryakov 2009-03-01 00:33:51 PST
Re-titling to make it clear what was fixed (we didn't disallow _top as a name, and I'm still confused on whether that's needed to match other browsers).
Comment 10 Sverrir Á. Berg 2009-03-01 09:33:04 PST
I was not able to verify on latest Windows nightly is built on 41199 (and my Windows box has never been able to build webkit).
The patch http://trac.webkit.org/changeset/41213 has been integrated into chromium and testing the patch there seems to indicate that this patch does not fix the problem.  I reopen the bug for now.

I will try to explain what my understanding up to now.  There are multiple ways to refer to frames and some of them have special meaning.  Those that I know of are: by name in the top namespace (javascript: myframe), by iterating through the frameset (javascript: frames[i]) and in hyperlinks (<a target="myframe"...).  As far as I can tell the hyperlink-target approach is the only one that should have this special meaning.  Currently webkit is not allowing frames to call themselves _blank but both IE and FF allow that.  Other limitation in webkit is the enforcing of unique names - both IE and FF allow multiple frames at the same level with the same name.  Referring to them in javascript consistently just finds the first frame with that name.  The third thing I've found is it is inconsistent within webkit if you can find frames with special names or not - using iteration you get the mangled names but direct reference by id or name finds the frame fine.

My change was only a quick workaround to fix the bug and I suggest it is checked in for now until a more involved approach is found.  The correct approach IMO is to allow frames to pick their own names and when finding frames for hyperlink targets the special names are checked at that point and then a fallback in looking up the framename from the frametree.  This will give a more consistent behavior to FF/IE.
Comment 11 Dimitri Glazkov (Google) 2009-03-01 13:11:02 PST
Created attachment 28145 [details]
The original crash reduction

I went ahead and uploaded the crash reduction as you described. It does not cause a crash on WebKit r41279 or Chromium r10680. BTW, you don't have to build WebKit or Chromium: you can download nightlies from http://nightly.webkit.org/ or http://build.chromium.org/buildbot/continuous/.

I think the issue still exists, because naming inner frame "_top" causes overwrite of the outer frame. But there's no crash. Right?
Comment 12 Sverrir Á. Berg 2009-03-20 10:11:40 PDT
Created attachment 28786 [details]
The right way to fix this

This is the right way to fix this.  Now frames can basically have any name they want and even same names as other frames.  This brings WebKit in line with how other browsers work.
Note that this breaks approx 50 tests that rely on the current name mangling (<!--framePath //<!--frame0-->-->) of frame names.  If this patch gets a positive review I will upload another patch that fixes these tests.
Comment 13 Darin Adler 2009-03-20 10:28:27 PDT
Comment on attachment 28786 [details]
The right way to fix this

Thanks for tackling this.

I'm excited that you think we can make frames without names work properly. Are you sure there are no new problems caused by removing the names? I believe we use those frame names for back/forward housekeeping on Mac OS X at least. I'm not sure it's safe to just throw that away.

What's your rationale for removing the disconnected frame feature? The fact that _setIsDisconnected is in WebFramePrivate.h means that it’s SPI on Mac OS X. So removing it would break Mac OS X applications that are using that API. So you can’t just remove it. Did you remove it because you didn’t understand it or because of some specific problem with it?

There’s no ChangeLog entry for WebCore. You need one.

The patch needs to include changes to test results for the tests that depend on the old frame naming system, not just the new test.

There are tabs and a conflict marker in the LayoutTests/ChangeLog entry.
Comment 14 Alexey Proskuryakov 2009-05-12 08:14:38 PDT
Re-titling again, as the crash no longer occurs. Also, downgrading from P1 for the same reason.