<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[247924] trunk</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/247924">247924</a></dd>
<dt>Author</dt> <dd>rniwa@webkit.org</dd>
<dt>Date</dt> <dd>2019-07-29 14:57:39 -0700 (Mon, 29 Jul 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre>The maximum subframe count check should not be skipped for empty URLs.
https://bugs.webkit.org/show_bug.cgi?id=200032

Patch by Sergei Glazunov <glazunov@google.com> on 2019-07-29
Reviewed by Ryosuke Niwa.

Source/WebCore:

Move the check closer to the actual frame creation code in `loadSubframe`.

Test: fast/dom/connected-subframe-counter-overflow.html

* dom/Document.cpp:
(WebCore::Document::prepareForDestruction): Assert that all child frames have been detached.
* html/HTMLFrameElementBase.cpp:
(WebCore::HTMLFrameElementBase::canLoad const):
(WebCore::HTMLFrameElementBase::canLoadURL const):
* html/HTMLFrameOwnerElement.cpp:
(WebCore::HTMLFrameOwnerElement::canAddSubframe const): Deleted.
* html/HTMLFrameOwnerElement.h:
* html/HTMLPlugInImageElement.cpp:
(WebCore::HTMLPlugInImageElement::canLoadURL const):
* loader/SubframeLoader.cpp:
(WebCore::SubframeLoader::loadSubframe):

LayoutTests:

* fast/dom/connected-subframe-counter-overflow-expected.txt: Added.
* fast/dom/connected-subframe-counter-overflow.html: Added.
* fast/frames/lots-of-iframes-expected.txt:</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkLayoutTestsfastframeslotsofiframesexpectedtxt">trunk/LayoutTests/fast/frames/lots-of-iframes-expected.txt</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoredomDocumentcpp">trunk/Source/WebCore/dom/Document.cpp</a></li>
<li><a href="#trunkSourceWebCorehtmlHTMLFrameElementBasecpp">trunk/Source/WebCore/html/HTMLFrameElementBase.cpp</a></li>
<li><a href="#trunkSourceWebCorehtmlHTMLFrameOwnerElementcpp">trunk/Source/WebCore/html/HTMLFrameOwnerElement.cpp</a></li>
<li><a href="#trunkSourceWebCorehtmlHTMLFrameOwnerElementh">trunk/Source/WebCore/html/HTMLFrameOwnerElement.h</a></li>
<li><a href="#trunkSourceWebCorehtmlHTMLPlugInImageElementcpp">trunk/Source/WebCore/html/HTMLPlugInImageElement.cpp</a></li>
<li><a href="#trunkSourceWebCoreloaderSubframeLoadercpp">trunk/Source/WebCore/loader/SubframeLoader.cpp</a></li>
</ul>

<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsfastdomconnectedsubframecounteroverflowexpectedtxt">trunk/LayoutTests/fast/dom/connected-subframe-counter-overflow-expected.txt</a></li>
<li><a href="#trunkLayoutTestsfastdomconnectedsubframecounteroverflowhtml">trunk/LayoutTests/fast/dom/connected-subframe-counter-overflow.html</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (247923 => 247924)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog      2019-07-29 21:43:42 UTC (rev 247923)
+++ trunk/LayoutTests/ChangeLog 2019-07-29 21:57:39 UTC (rev 247924)
</span><span class="lines">@@ -1,3 +1,14 @@
</span><ins>+2019-07-29  Sergei Glazunov  <glazunov@google.com>
+
+        The maximum subframe count check should not be skipped for empty URLs.
+        https://bugs.webkit.org/show_bug.cgi?id=200032
+
+        Reviewed by Ryosuke Niwa.
+
+        * fast/dom/connected-subframe-counter-overflow-expected.txt: Added.
+        * fast/dom/connected-subframe-counter-overflow.html: Added.
+        * fast/frames/lots-of-iframes-expected.txt:
+
</ins><span class="cx"> 2019-07-29  Youenn Fablet  <youenn@apple.com>
</span><span class="cx"> 
</span><span class="cx">         REGRESSION: WebSockets no longer work in Service Workers
</span></span></pre></div>
<a id="trunkLayoutTestsfastdomconnectedsubframecounteroverflowexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/dom/connected-subframe-counter-overflow-expected.txt (0 => 247924)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/fast/dom/connected-subframe-counter-overflow-expected.txt                              (rev 0)
+++ trunk/LayoutTests/fast/dom/connected-subframe-counter-overflow-expected.txt 2019-07-29 21:57:39 UTC (rev 247924)
</span><span class="lines">@@ -0,0 +1,10 @@
</span><ins>+The connected subframe counter should not overflow and make `disconnectSubframes` leave active subframes in a detached DOM node.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS container.firstChild.contentWindow is null
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
</ins></span></pre></div>
<a id="trunkLayoutTestsfastdomconnectedsubframecounteroverflowhtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/dom/connected-subframe-counter-overflow.html (0 => 247924)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/fast/dom/connected-subframe-counter-overflow.html                              (rev 0)
+++ trunk/LayoutTests/fast/dom/connected-subframe-counter-overflow.html 2019-07-29 21:57:39 UTC (rev 247924)
</span><span class="lines">@@ -0,0 +1,23 @@
</span><ins>+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script>
+description('The connected subframe counter should not overflow and make `disconnectSubframes` leave active subframes in a detached DOM node.');
+
+const FRAME_COUNT = 1024;
+
+let container = document.body.appendChild(document.createElement('div'));
+for (let i = 0; i < FRAME_COUNT; ++i) {
+  let frame = container.appendChild(document.createElement('iframe'));
+  frame.style.display = 'none';
+}
+container.remove();
+
+shouldBeNull('container.firstChild.contentWindow');
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
</ins></span></pre></div>
<a id="trunkLayoutTestsfastframeslotsofiframesexpectedtxt"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/fast/frames/lots-of-iframes-expected.txt (247923 => 247924)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/fast/frames/lots-of-iframes-expected.txt       2019-07-29 21:43:42 UTC (rev 247923)
+++ trunk/LayoutTests/fast/frames/lots-of-iframes-expected.txt  2019-07-29 21:57:39 UTC (rev 247924)
</span><span class="lines">@@ -1,2 +1,3 @@
</span><span class="cx"> Sucessfully created 1000 frames.
</span><span class="cx"> Successfully blocked creation of frame number 1001.
</span><ins>+
</ins></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (247923 => 247924)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog   2019-07-29 21:43:42 UTC (rev 247923)
+++ trunk/Source/WebCore/ChangeLog      2019-07-29 21:57:39 UTC (rev 247924)
</span><span class="lines">@@ -1,3 +1,27 @@
</span><ins>+2019-07-29  Sergei Glazunov  <glazunov@google.com>
+
+        The maximum subframe count check should not be skipped for empty URLs.
+        https://bugs.webkit.org/show_bug.cgi?id=200032
+
+        Reviewed by Ryosuke Niwa.
+
+        Move the check closer to the actual frame creation code in `loadSubframe`.
+
+        Test: fast/dom/connected-subframe-counter-overflow.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::prepareForDestruction): Assert that all child frames have been detached.
+        * html/HTMLFrameElementBase.cpp:
+        (WebCore::HTMLFrameElementBase::canLoad const):
+        (WebCore::HTMLFrameElementBase::canLoadURL const):
+        * html/HTMLFrameOwnerElement.cpp:
+        (WebCore::HTMLFrameOwnerElement::canAddSubframe const): Deleted.
+        * html/HTMLFrameOwnerElement.h:
+        * html/HTMLPlugInImageElement.cpp:
+        (WebCore::HTMLPlugInImageElement::canLoadURL const):
+        * loader/SubframeLoader.cpp:
+        (WebCore::SubframeLoader::loadSubframe):
+
</ins><span class="cx"> 2019-07-29  Zalan Bujtas  <zalan@apple.com>
</span><span class="cx"> 
</span><span class="cx">         [ContentChangeObserver] ChromeClient::observedContentChange() name is misleading
</span></span></pre></div>
<a id="trunkSourceWebCoredomDocumentcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/dom/Document.cpp (247923 => 247924)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/dom/Document.cpp    2019-07-29 21:43:42 UTC (rev 247923)
+++ trunk/Source/WebCore/dom/Document.cpp       2019-07-29 21:57:39 UTC (rev 247924)
</span><span class="lines">@@ -2487,6 +2487,7 @@
</span><span class="cx">         NavigationDisabler navigationDisabler(m_frame);
</span><span class="cx">         disconnectDescendantFrames();
</span><span class="cx">     }
</span><ins>+    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!m_frame || !m_frame->tree().childCount());
</ins><span class="cx"> 
</span><span class="cx">     if (m_domWindow && m_frame)
</span><span class="cx">         m_domWindow->willDetachDocumentFromFrame();
</span></span></pre></div>
<a id="trunkSourceWebCorehtmlHTMLFrameElementBasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/html/HTMLFrameElementBase.cpp (247923 => 247924)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/html/HTMLFrameElementBase.cpp       2019-07-29 21:43:42 UTC (rev 247923)
+++ trunk/Source/WebCore/html/HTMLFrameElementBase.cpp  2019-07-29 21:57:39 UTC (rev 247924)
</span><span class="lines">@@ -61,7 +61,6 @@
</span><span class="cx"> {
</span><span class="cx">     // FIXME: Why is it valuable to return true when m_URL is empty?
</span><span class="cx">     // FIXME: After openURL replaces an empty URL with the blank URL, this may no longer necessarily return true.
</span><del>-    // FIXME: It does not seem correct to skip the maximum subframe count check when m_URL is empty.
</del><span class="cx">     return m_URL.isEmpty() || canLoadURL(m_URL);
</span><span class="cx"> }
</span><span class="cx"> 
</span><span class="lines">@@ -73,10 +72,6 @@
</span><span class="cx"> // Note that unlike HTMLPlugInImageElement::canLoadURL this uses ScriptController::canAccessFromCurrentOrigin.
</span><span class="cx"> bool HTMLFrameElementBase::canLoadURL(const URL& completeURL) const
</span><span class="cx"> {
</span><del>-    // FIXME: This assumes we are adding a new subframe; incorrectly prevents modifying an existing one once we are at the limit.
-    if (!canAddSubframe())
-        return false;
-
</del><span class="cx">     if (WTF::protocolIsJavaScript(completeURL)) {
</span><span class="cx">         RefPtr<Document> contentDocument = this->contentDocument();
</span><span class="cx">         if (contentDocument && !ScriptController::canAccessFromCurrentOrigin(contentDocument->frame(), document()))
</span></span></pre></div>
<a id="trunkSourceWebCorehtmlHTMLFrameOwnerElementcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/html/HTMLFrameOwnerElement.cpp (247923 => 247924)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/html/HTMLFrameOwnerElement.cpp      2019-07-29 21:43:42 UTC (rev 247923)
+++ trunk/Source/WebCore/html/HTMLFrameOwnerElement.cpp 2019-07-29 21:57:39 UTC (rev 247924)
</span><span class="lines">@@ -128,13 +128,6 @@
</span><span class="cx">         invalidateStyleAndLayerComposition();
</span><span class="cx"> }
</span><span class="cx"> 
</span><del>-bool HTMLFrameOwnerElement::canAddSubframe() const
-{
-    // FIXME: Might be safer to return false when page is null, but need to test in case we rely on returning true.
-    auto* page = document().page();
-    return !page || page->subframeCount() < Page::maxNumberOfFrames;
-}
-
</del><span class="cx"> bool HTMLFrameOwnerElement::isProhibitedSelfReference(const URL& completeURL) const
</span><span class="cx"> {
</span><span class="cx">     // We allow one level of self-reference because some websites depend on that, but we don't allow more than one.
</span></span></pre></div>
<a id="trunkSourceWebCorehtmlHTMLFrameOwnerElementh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/html/HTMLFrameOwnerElement.h (247923 => 247924)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/html/HTMLFrameOwnerElement.h        2019-07-29 21:43:42 UTC (rev 247923)
+++ trunk/Source/WebCore/html/HTMLFrameOwnerElement.h   2019-07-29 21:57:39 UTC (rev 247924)
</span><span class="lines">@@ -65,7 +65,6 @@
</span><span class="cx"> protected:
</span><span class="cx">     HTMLFrameOwnerElement(const QualifiedName& tagName, Document&);
</span><span class="cx">     void setSandboxFlags(SandboxFlags);
</span><del>-    bool canAddSubframe() const;
</del><span class="cx">     bool isProhibitedSelfReference(const URL&) const;
</span><span class="cx"> 
</span><span class="cx"> private:
</span></span></pre></div>
<a id="trunkSourceWebCorehtmlHTMLPlugInImageElementcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/html/HTMLPlugInImageElement.cpp (247923 => 247924)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/html/HTMLPlugInImageElement.cpp     2019-07-29 21:43:42 UTC (rev 247923)
+++ trunk/Source/WebCore/html/HTMLPlugInImageElement.cpp        2019-07-29 21:57:39 UTC (rev 247924)
</span><span class="lines">@@ -159,13 +159,9 @@
</span><span class="cx">     return canLoadURL(document().completeURL(relativeURL));
</span><span class="cx"> }
</span><span class="cx"> 
</span><del>-// Note that unlike HTMLFrameElementBase::canLoadURL this uses ScriptController::canAccessFromCurrentOrigin.
</del><ins>+// Note that unlike HTMLFrameElementBase::canLoadURL this uses SecurityOrigin::canAccess.
</ins><span class="cx"> bool HTMLPlugInImageElement::canLoadURL(const URL& completeURL) const
</span><span class="cx"> {
</span><del>-    // FIXME: This assumes we are adding a new subframe; incorrectly prevents modifying an existing one once we are at the limit.
-    if (!canAddSubframe())
-        return false;
-
</del><span class="cx">     if (WTF::protocolIsJavaScript(completeURL)) {
</span><span class="cx">         RefPtr<Document> contentDocument = this->contentDocument();
</span><span class="cx">         if (contentDocument && !document().securityOrigin().canAccess(contentDocument->securityOrigin()))
</span></span></pre></div>
<a id="trunkSourceWebCoreloaderSubframeLoadercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/loader/SubframeLoader.cpp (247923 => 247924)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/loader/SubframeLoader.cpp   2019-07-29 21:43:42 UTC (rev 247923)
+++ trunk/Source/WebCore/loader/SubframeLoader.cpp      2019-07-29 21:57:39 UTC (rev 247924)
</span><span class="lines">@@ -329,6 +329,9 @@
</span><span class="cx">     if (!SubframeLoadingDisabler::canLoadFrame(ownerElement))
</span><span class="cx">         return nullptr;
</span><span class="cx"> 
</span><ins>+    if (!m_frame.page() || m_frame.page()->subframeCount() >= Page::maxNumberOfFrames)
+        return nullptr;
+
</ins><span class="cx">     ReferrerPolicy policy = ownerElement.referrerPolicy();
</span><span class="cx">     if (policy == ReferrerPolicy::EmptyString)
</span><span class="cx">         policy = document->referrerPolicy();
</span></span></pre>
</div>
</div>

</body>
</html>