- Revision
- 190752
- Author
- [email protected]
- Date
- 2015-10-08 15:43:17 -0700 (Thu, 08 Oct 2015)
Log Message
Generated frame tree names should be kept reasonably long.
<https://webkit.org/b/149874>
Reviewed by Darin Adler.
Source/WebCore:
Some clumsy advertising script is going around assigning _javascript_ source code
to the "name" attribute of iframes. This is causing WebKit to generate way too huge
names for anonymous descendants of such iframes.
Previously, the generated name of an anonymous subframe would be its slash-separated
path from the root frame, with the "name" attribute of each ancestor between the
slashes, or "<!--frame${index in parent}-->" for anonymous ancestors.
These ad scripts are often over 100kB in size, with multiple subframes, so we'd end
up with frame names looking like this:
"<!--framePath //<MONSTER BLOB OF _javascript_ FROM HELL>/<!--frame0--><!--frame0-->-->"
While this is worth fixing for the memory usage alone, we've been making it way
worse by also using these paths when recording the back/forward history parts of
WebKit session state.
This patch makes generated paths always use index-in-parent as the "directory name"
for ancestors of anonymous subframes. The above example path will now instead be:
"<!--framePath //<!--frame0-->/<!--frame0-->/<!--frame0-->-->"
Test: fast/frames/long-names-in-nested-subframes.html
* page/FrameTree.cpp:
(WebCore::FrameTree::indexInParent):
(WebCore::FrameTree::uniqueChildName):
* page/FrameTree.h:
LayoutTests:
Added a test to document our name generation behavior for subframes with long-named ancestors.
Also rebaselined some tests that exposed the old behavior.
* fast/forms/form-and-frame-interaction-retains-values-expected.txt:
* fast/frames/long-names-in-nested-subframes-expected.txt: Added.
* fast/frames/long-names-in-nested-subframes.html: Added.
* http/tests/navigation/image-load-in-subframe-unload-handler-expected.txt:
* http/tests/security/dataURL/xss-DENIED-from-data-url-sub-frame-2-level-expected.txt:
* http/tests/security/dataURL/xss-DENIED-to-data-url-sub-frame-2-level-expected.txt:
* http/tests/security/_javascript_URL/xss-ALLOWED-from-_javascript_-url-sub-frame-2-level-expected.txt:
* http/tests/security/_javascript_URL/xss-ALLOWED-from-_javascript_-url-to-javscript-url-expected.txt:
* http/tests/security/_javascript_URL/xss-ALLOWED-to-_javascript_-url-from-javscript-url-expected.txt:
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (190751 => 190752)
--- trunk/LayoutTests/ChangeLog 2015-10-08 22:25:49 UTC (rev 190751)
+++ trunk/LayoutTests/ChangeLog 2015-10-08 22:43:17 UTC (rev 190752)
@@ -1,3 +1,23 @@
+2015-10-08 Andreas Kling <[email protected]>
+
+ Generated frame tree names should be kept reasonably long.
+ <https://webkit.org/b/149874>
+
+ Reviewed by Darin Adler.
+
+ Added a test to document our name generation behavior for subframes with long-named ancestors.
+ Also rebaselined some tests that exposed the old behavior.
+
+ * fast/forms/form-and-frame-interaction-retains-values-expected.txt:
+ * fast/frames/long-names-in-nested-subframes-expected.txt: Added.
+ * fast/frames/long-names-in-nested-subframes.html: Added.
+ * http/tests/navigation/image-load-in-subframe-unload-handler-expected.txt:
+ * http/tests/security/dataURL/xss-DENIED-from-data-url-sub-frame-2-level-expected.txt:
+ * http/tests/security/dataURL/xss-DENIED-to-data-url-sub-frame-2-level-expected.txt:
+ * http/tests/security/_javascript_URL/xss-ALLOWED-from-_javascript_-url-sub-frame-2-level-expected.txt:
+ * http/tests/security/_javascript_URL/xss-ALLOWED-from-_javascript_-url-to-javscript-url-expected.txt:
+ * http/tests/security/_javascript_URL/xss-ALLOWED-to-_javascript_-url-from-javscript-url-expected.txt:
+
2015-10-08 Saam barati <[email protected]>
We should be able to inline getter/setter calls inside an inline cache even when the SpillRegistersMode is NeedsToSpill
Modified: trunk/LayoutTests/fast/forms/form-and-frame-interaction-retains-values-expected.txt (190751 => 190752)
--- trunk/LayoutTests/fast/forms/form-and-frame-interaction-retains-values-expected.txt 2015-10-08 22:25:49 UTC (rev 190751)
+++ trunk/LayoutTests/fast/forms/form-and-frame-interaction-retains-values-expected.txt 2015-10-08 22:43:17 UTC (rev 190752)
@@ -13,6 +13,6 @@
--------
-Frame: '<!--framePath //submitted/<!--frame0-->-->'
+Frame: '<!--framePath //<!--frame0-->/<!--frame0-->-->'
--------
Added: trunk/LayoutTests/fast/frames/long-names-in-nested-subframes-expected.txt (0 => 190752)
--- trunk/LayoutTests/fast/frames/long-names-in-nested-subframes-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/frames/long-names-in-nested-subframes-expected.txt 2015-10-08 22:43:17 UTC (rev 190752)
@@ -0,0 +1,31 @@
+
+
+--------
+Frame: 'since_this_name_is_very_long_it_would_not_be_great_to_repeat_it_in_every_frame_path'
+--------
+
+
+--------
+Frame: 'and_this_name_is_long_too_so_we_would_get_pretty_long_names'
+--------
+
+
+--------
+Frame: '<!--framePath //<!--frame0-->/<!--frame0-->/<!--frame0-->-->'
+--------
+
+
+--------
+Frame: '<!--framePath //<!--frame0-->/<!--frame0-->/<!--frame1-->-->'
+--------
+
+
+--------
+Frame: '<!--framePath //<!--frame0-->/<!--frame0-->/<!--frame2-->-->'
+--------
+
+
+--------
+Frame: '<!--framePath //<!--frame0-->/<!--frame0-->/<!--frame3-->-->'
+--------
+
Added: trunk/LayoutTests/fast/frames/long-names-in-nested-subframes.html (0 => 190752)
--- trunk/LayoutTests/fast/frames/long-names-in-nested-subframes.html (rev 0)
+++ trunk/LayoutTests/fast/frames/long-names-in-nested-subframes.html 2015-10-08 22:43:17 UTC (rev 190752)
@@ -0,0 +1,23 @@
+<html>
+<head>
+<script>
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.dumpChildFramesAsText();
+}
+</script>
+</head>
+<body>
+<iframe
+ name="since_this_name_is_very_long_it_would_not_be_great_to_repeat_it_in_every_frame_path"
+ src=""
+ <iframe name=and_this_name_is_long_too_so_we_would_get_pretty_long_names src=''></iframe>">
+</iframe>
+</body>
+</html>
Modified: trunk/LayoutTests/http/tests/navigation/image-load-in-subframe-unload-handler-expected.txt (190751 => 190752)
--- trunk/LayoutTests/http/tests/navigation/image-load-in-subframe-unload-handler-expected.txt 2015-10-08 22:25:49 UTC (rev 190751)
+++ trunk/LayoutTests/http/tests/navigation/image-load-in-subframe-unload-handler-expected.txt 2015-10-08 22:43:17 UTC (rev 190752)
@@ -1,2 +1,2 @@
-frame "<!--framePath //target/<!--frame0-->-->" - has 1 onunload handler(s)
+frame "<!--framePath //<!--frame0-->/<!--frame0-->-->" - has 1 onunload handler(s)
This test triggers an unload handler that starts an image load in a different frame (and deletes both frames), but ensures the main frame is not destroyed. We pass if we don't crash.
Modified: trunk/LayoutTests/http/tests/security/dataURL/xss-DENIED-from-data-url-sub-frame-2-level-expected.txt (190751 => 190752)
--- trunk/LayoutTests/http/tests/security/dataURL/xss-DENIED-from-data-url-sub-frame-2-level-expected.txt 2015-10-08 22:25:49 UTC (rev 190751)
+++ trunk/LayoutTests/http/tests/security/dataURL/xss-DENIED-from-data-url-sub-frame-2-level-expected.txt 2015-10-08 22:43:17 UTC (rev 190752)
@@ -13,6 +13,6 @@
--------
-Frame: '<!--framePath //aFrame/<!--frame0-->-->'
+Frame: '<!--framePath //<!--frame0-->/<!--frame0-->-->'
--------
Inner-inner iframe.
Modified: trunk/LayoutTests/http/tests/security/dataURL/xss-DENIED-to-data-url-sub-frame-2-level-expected.txt (190751 => 190752)
--- trunk/LayoutTests/http/tests/security/dataURL/xss-DENIED-to-data-url-sub-frame-2-level-expected.txt 2015-10-08 22:25:49 UTC (rev 190751)
+++ trunk/LayoutTests/http/tests/security/dataURL/xss-DENIED-to-data-url-sub-frame-2-level-expected.txt 2015-10-08 22:43:17 UTC (rev 190752)
@@ -14,7 +14,7 @@
--------
-Frame: '<!--framePath //aFrame/<!--frame0-->-->'
+Frame: '<!--framePath //<!--frame0-->/<!--frame0-->-->'
--------
PASS: Cross frame access to a data: URL 2 levels deep was denied.
Modified: trunk/LayoutTests/http/tests/security/_javascript_URL/xss-ALLOWED-from-_javascript_-url-sub-frame-2-level-expected.txt (190751 => 190752)
--- trunk/LayoutTests/http/tests/security/_javascript_URL/xss-ALLOWED-from-_javascript_-url-sub-frame-2-level-expected.txt 2015-10-08 22:25:49 UTC (rev 190751)
+++ trunk/LayoutTests/http/tests/security/_javascript_URL/xss-ALLOWED-from-_javascript_-url-sub-frame-2-level-expected.txt 2015-10-08 22:43:17 UTC (rev 190752)
@@ -10,6 +10,6 @@
Inner iframe.
--------
-Frame: '<!--framePath //aFrame/<!--frame0-->-->'
+Frame: '<!--framePath //<!--frame0-->/<!--frame0-->-->'
--------
Inner-inner iframe.
Modified: trunk/LayoutTests/http/tests/security/_javascript_URL/xss-ALLOWED-from-_javascript_-url-to-javscript-url-expected.txt (190751 => 190752)
--- trunk/LayoutTests/http/tests/security/_javascript_URL/xss-ALLOWED-from-_javascript_-url-to-javscript-url-expected.txt 2015-10-08 22:25:49 UTC (rev 190751)
+++ trunk/LayoutTests/http/tests/security/_javascript_URL/xss-ALLOWED-from-_javascript_-url-to-javscript-url-expected.txt 2015-10-08 22:43:17 UTC (rev 190752)
@@ -9,7 +9,7 @@
Inner iframe.
--------
-Frame: '<!--framePath //aFrame/<!--frame0-->-->'
+Frame: '<!--framePath //<!--frame0-->/<!--frame0-->-->'
--------
PASS: Cross frame access from a _javascript_: URL was allowed!
Modified: trunk/LayoutTests/http/tests/security/_javascript_URL/xss-ALLOWED-to-_javascript_-url-from-javscript-url-expected.txt (190751 => 190752)
--- trunk/LayoutTests/http/tests/security/_javascript_URL/xss-ALLOWED-to-_javascript_-url-from-javscript-url-expected.txt 2015-10-08 22:25:49 UTC (rev 190751)
+++ trunk/LayoutTests/http/tests/security/_javascript_URL/xss-ALLOWED-to-_javascript_-url-from-javscript-url-expected.txt 2015-10-08 22:43:17 UTC (rev 190752)
@@ -11,6 +11,6 @@
Inner iframe.
--------
-Frame: '<!--framePath //aFrame/<!--frame0-->-->'
+Frame: '<!--framePath //<!--frame0-->/<!--frame0-->-->'
--------
Inner-inner iframe.
Modified: trunk/LayoutTests/http/tests/security/_javascript_URL/xss-ALLOWED-to-_javascript_-url-sub-frame-2-level-expected.txt (190751 => 190752)
--- trunk/LayoutTests/http/tests/security/_javascript_URL/xss-ALLOWED-to-_javascript_-url-sub-frame-2-level-expected.txt 2015-10-08 22:25:49 UTC (rev 190751)
+++ trunk/LayoutTests/http/tests/security/_javascript_URL/xss-ALLOWED-to-_javascript_-url-sub-frame-2-level-expected.txt 2015-10-08 22:43:17 UTC (rev 190752)
@@ -11,7 +11,7 @@
Inner iframe.
--------
-Frame: '<!--framePath //aFrame/<!--frame0-->-->'
+Frame: '<!--framePath //<!--frame0-->/<!--frame0-->-->'
--------
PASS: Cross frame access to a _javascript_: URL 2 levels deep was allowed!
Modified: trunk/Source/WebCore/ChangeLog (190751 => 190752)
--- trunk/Source/WebCore/ChangeLog 2015-10-08 22:25:49 UTC (rev 190751)
+++ trunk/Source/WebCore/ChangeLog 2015-10-08 22:43:17 UTC (rev 190752)
@@ -1,3 +1,39 @@
+2015-10-08 Andreas Kling <[email protected]>
+
+ Generated frame tree names should be kept reasonably long.
+ <https://webkit.org/b/149874>
+
+ Reviewed by Darin Adler.
+
+ Some clumsy advertising script is going around assigning _javascript_ source code
+ to the "name" attribute of iframes. This is causing WebKit to generate way too huge
+ names for anonymous descendants of such iframes.
+
+ Previously, the generated name of an anonymous subframe would be its slash-separated
+ path from the root frame, with the "name" attribute of each ancestor between the
+ slashes, or "<!--frame${index in parent}-->" for anonymous ancestors.
+
+ These ad scripts are often over 100kB in size, with multiple subframes, so we'd end
+ up with frame names looking like this:
+
+ "<!--framePath //<MONSTER BLOB OF _javascript_ FROM HELL>/<!--frame0--><!--frame0-->-->"
+
+ While this is worth fixing for the memory usage alone, we've been making it way
+ worse by also using these paths when recording the back/forward history parts of
+ WebKit session state.
+
+ This patch makes generated paths always use index-in-parent as the "directory name"
+ for ancestors of anonymous subframes. The above example path will now instead be:
+
+ "<!--framePath //<!--frame0-->/<!--frame0-->/<!--frame0-->-->"
+
+ Test: fast/frames/long-names-in-nested-subframes.html
+
+ * page/FrameTree.cpp:
+ (WebCore::FrameTree::indexInParent):
+ (WebCore::FrameTree::uniqueChildName):
+ * page/FrameTree.h:
+
2015-10-08 Commit Queue <[email protected]>
Unreviewed, rolling out r190701.
Modified: trunk/Source/WebCore/page/FrameTree.cpp (190751 => 190752)
--- trunk/Source/WebCore/page/FrameTree.cpp 2015-10-08 22:25:49 UTC (rev 190751)
+++ trunk/Source/WebCore/page/FrameTree.cpp 2015-10-08 22:43:17 UTC (rev 190752)
@@ -82,6 +82,19 @@
return true;
}
+unsigned FrameTree::indexInParent() const
+{
+ if (!m_parent)
+ return 0;
+ unsigned index = 0;
+ for (Frame* frame = m_parent->tree().firstChild(); frame; frame = frame->tree().nextSibling()) {
+ if (&frame->tree() == this)
+ return index;
+ ++index;
+ }
+ RELEASE_ASSERT_NOT_REACHED();
+}
+
void FrameTree::appendChild(PassRefPtr<Frame> child)
{
ASSERT(child->page() == m_thisFrame.page());
@@ -128,16 +141,17 @@
AtomicString FrameTree::uniqueChildName(const AtomicString& requestedName) const
{
+ // If the requested name (the frame's "name" attribute) is unique, just use that.
if (!requestedName.isEmpty() && !child(requestedName) && requestedName != "_blank")
return requestedName;
- // Create a repeatable name for a child about to be added to us. The name must be
- // unique within the frame tree. The string we generate includes a "path" of names
- // from the root frame down to us. For this path to be unique, each set of siblings must
- // contribute a unique name to the path, which can't collide with any HTML-assigned names.
- // We generate this path component by index in the child list along with an unlikely
- // frame name that can't be set in HTML because it collides with comment syntax.
+ // The "name" attribute was not unique or absent. Generate a name based on the
+ // new frame's location in the frame tree. The name uses HTML comment syntax to
+ // avoid collisions with author names.
+ // An example path for the third child of the second child of the root frame:
+ // <!--framePath //<!--frame1-->/<!--frame2-->-->
+
const char framePathPrefix[] = "<!--framePath ";
const int framePathPrefixLength = 14;
const int framePathSuffixLength = 3;
@@ -159,7 +173,11 @@
for (int i = chain.size() - 1; i >= 0; --i) {
frame = chain[i];
name.append('/');
- name.append(frame->tree().uniqueName());
+ if (frame->tree().parent()) {
+ name.appendLiteral("<!--frame");
+ name.appendNumber(frame->tree().indexInParent());
+ name.appendLiteral("-->");
+ }
}
name.appendLiteral("/<!--frame");
Modified: trunk/Source/WebCore/page/FrameTree.h (190751 => 190752)
--- trunk/Source/WebCore/page/FrameTree.h 2015-10-08 22:25:49 UTC (rev 190751)
+++ trunk/Source/WebCore/page/FrameTree.h 2015-10-08 22:43:17 UTC (rev 190752)
@@ -84,6 +84,8 @@
Frame* scopedChild(const AtomicString& name) const;
unsigned scopedChildCount() const;
+ unsigned indexInParent() const;
+
private:
Frame* deepLastChild() const;
void actuallyAppendChild(PassRefPtr<Frame>);
