Bug 23900 – Arbitrary frame names should be supported, even duplicate or empty

• Component: Frames
• Status: NEW

[Sverrir Á. Berg]

Fixes a crash.  See:
https://bugs.webkit.org/show_bug.cgi?id=23899

[Sverrir Á. Berg]

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.

[Alexey Proskuryakov]

*** Bug 23899 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

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):

[Eric Seidel (no email)]

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;
}

[Sverrir Á. Berg]

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

[Alexey Proskuryakov]

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)
{...}

[Alexey Proskuryakov]

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.

[Dimitri Glazkov (Google)]

This is has been fixed by http://trac.webkit.org/changeset/41213.

[Alexey Proskuryakov]

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).

[Sverrir Á. Berg]

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.

[Dimitri Glazkov (Google)]

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?

[Sverrir Á. Berg]

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.

[Darin Adler]

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.

[Alexey Proskuryakov]

Re-titling again, as the crash no longer occurs. Also, downgrading from P1 for the same reason.