Tweak window.open features argument tokenizer to match HTML standard …

…and Edge https://bugs.webkit.org/show_bug.cgi?id=170548 Reviewed by Geoffrey Garen. LayoutTests/imported/w3c: * web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener-expected.txt: Rebaseline test now that more checks are passing. The remaining failures are because the test currently expects "noopener=0" / "noopener=false" to activate the 'noopener' feature. The test matches the specification which currently says that if the 'noopener' key is present, then the 'noopener' feature should be activated, no matter its value. However, I am intentionally not making this change yet because: - This behavior would be inconsistent with other Window features - There is upstream discussion on this (whatwg/html#2600) and the current feedback is that the specification should likely change to treat 'noopener' more consistently with other features. I will follow-up once the specification / test settles. * web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener.html: Re-sync test from upstream after web-platform-tests/wpt#5715. Source/WebCore: Update window.open() features argument tokenizer to match HTML standard: - https://html.spec.whatwg.org/#concept-window-open-features-tokenize Also update window.open() to return null instead of the window when the 'noopener' feature is activated, as per: - https://html.spec.whatwg.org/#dom-open (Step 10) No new tests, rebaselined existing test. * page/DOMWindow.cpp: (WebCore::DOMWindow::createWindow): Update window.open() to return null instead of the window when the 'noopener' feature is activated, as per: - https://html.spec.whatwg.org/#dom-open (Step 10) * page/WindowFeatures.cpp: (WebCore::isSeparator): Treat all ASCII spaces as feature separators, as per: - https://html.spec.whatwg.org/#feature-separator This has the effect of adding U+000C (FormFeed) as a separator. (WebCore::processFeaturesString): Align tokenizing code with the specification: - https://html.spec.whatwg.org/#concept-window-open-features-tokenize In particular, the following changes were made: - After the key, skip to first '=', but don't skip past a ',' or a non-separator. The "or a non-separator" part is new in the spec (step 3.6.1) and is now implemented. - After looking for the '=', only treat what follows as a value if the current character is a separator. This is as per step 7 in the spec. These changes now cause us to parse 'foo noopener=1' as ('foo', ''), ('noopener', '1'). git-svn-id: http://svn.webkit.org/repository/webkit/trunk@215945 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Apr 28, 2017 · 08907ab · 08907ab
1 parent a8b5193
commit 08907ab
Show file tree

Hide file tree

Showing 9 changed files with 102 additions and 49 deletions.
diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
@@ -1,3 +1,22 @@
+2017-04-28  Chris Dumez  <cdumez@apple.com>
+
+        Tweak window.open features argument tokenizer to match HTML standard and Edge
+        https://bugs.webkit.org/show_bug.cgi?id=170548
+
+        Reviewed by Geoffrey Garen.
+
+        * web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener-expected.txt:
+        Rebaseline test now that more checks are passing. The remaining failures are because the test currently expects "noopener=0" / "noopener=false" to activate
+        the 'noopener' feature. The test matches the specification which currently says that if the 'noopener' key is present, then the 'noopener' feature should be
+        activated, no matter its value. However, I am intentionally not making this change yet because:
+        - This behavior would be inconsistent with other Window features
+        - There is upstream discussion on this (https://github.com/whatwg/html/issues/2600) and the current feedback is that the specification should likely
+          change to treat 'noopener' more consistently with other features.
+        I will follow-up once the specification / test settles.
+
+        * web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener.html:
+        Re-sync test from upstream after https://github.com/w3c/web-platform-tests/pull/5715.
+
 2017-04-28  Chris Dumez  <cdumez@apple.com>
 
         Update DOMTokenList.replace() to match the latest DOM specification

diff --git a/...and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener-expected.txt b/...and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener-expected.txt
@@ -1,12 +1,9 @@
 
-FAIL tokenization should skip window features separators before `name` assert_equals: " noopener" should activate feature "noopener" expected null but got [stringifying object threw SecurityError (DOM Exception 18): Blocked a frame with origin "http://localhost:8800" from accessing a frame with origin "null".  The frame requesting access has a protocol of "http", the frame being accessed has a protocol of "". Protocols must match.
- with type object]
-FAIL feature `name` should be converted to ASCII lowercase assert_equals: "NOOPENER" should activate feature "noopener" expected null but got [stringifying object threw SecurityError (DOM Exception 18): Blocked a frame with origin "http://localhost:8800" from accessing a frame with origin "null".  The frame requesting access has a protocol of "http", the frame being accessed has a protocol of "". Protocols must match.
- with type object]
-FAIL after `name`, tokenization should skip window features separators that are not "=" or "," assert_equals: "noopener" should activate feature "noopener" expected null but got [stringifying object threw SecurityError (DOM Exception 18): Blocked a frame with origin "http://localhost:8800" from accessing a frame with origin "null".  The frame requesting access has a protocol of "http", the frame being accessed has a protocol of "". Protocols must match.
- with type object]
-FAIL Tokenizing should ignore window feature separators except "," after initial "=" and before value assert_equals: "noopener=  yes" should activate feature "noopener" expected null but got [stringifying object threw SecurityError (DOM Exception 18): Blocked a frame with origin "http://localhost:8800" from accessing a frame with origin "null".  The frame requesting access has a protocol of "http", the frame being accessed has a protocol of "". Protocols must match.
- with type object]
+PASS tokenization should skip window features separators before `name` 
+FAIL feature `name` should be converted to ASCII lowercase assert_equals: "noopener=NOOPENER" should activate feature "noopener" expected null but got object "[object Window]"
+PASS after `name`, tokenization should skip window features separators that are not "=" or "," 
+FAIL Tokenizing should ignore window feature separators except "," after initial "=" and before value assert_equals: "noopener
+=\r noopener," should activate feature "noopener" expected null but got object "[object Window]"
 FAIL Tokenizing should read characters until first window feature separator as `value` assert_equals: "noopener=noopener" should set "noopener" expected null but got object "[object Window]"
 FAIL "noopener" should be based on name (key), not value assert_equals: "noopener=false" should activate feature "noopener" expected null but got object "[object Window]"
 PASS invalid feature names should not tokenize as "noopener" 

diff --git a/...reating-and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener.html b/...reating-and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener.html
@@ -64,7 +64,7 @@
     'noopener,=',
     'noopener foo', // => ('noopener', ''), ('foo', '')
     'foo noopener=1', // => ('foo', ''), ('noopener', '1')
-    'foo=\u000Cnoopener' // => ('foo', ''), ('noopener', '')
+    'foo=\u000Cbar\u000Cnoopener' // => ('foo', 'bar'), ('noopener', '')
   ];
   featureVariants.forEach(feature => {
     var win = window.open(windowURL, '', feature);
@@ -139,7 +139,8 @@
     'no\nopener', //    => ('no', ''), ('opener', '')
     'no,opener', //     => ('no', ''), ('opener', '')
     '\0noopener', //    => ('\0noopener', '')
-    'noopener\u0000=yes' // => ('noopener\0', 'yes')
+    'noopener\u0000=yes', // => ('noopener\0', 'yes')
+    'foo=\u000Cnoopener' // => ('foo', 'noopener')
   ];
   invalidFeatureVariants.forEach(feature => {
     var win = window.open(windowURL, '', feature);

diff --git a/...and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener-expected.txt b/...and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener-expected.txt
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,41 @@
+2017-04-28  Chris Dumez  <cdumez@apple.com>
+
+        Tweak window.open features argument tokenizer to match HTML standard and Edge
+        https://bugs.webkit.org/show_bug.cgi?id=170548
+
+        Reviewed by Geoffrey Garen.
+
+        Update window.open() features argument tokenizer to match HTML standard:
+        - https://html.spec.whatwg.org/#concept-window-open-features-tokenize
+
+        Also update window.open() to return null instead of the window when
+        the 'noopener' feature is activated, as per:
+        - https://html.spec.whatwg.org/#dom-open (Step 10)
+
+        No new tests, rebaselined existing test.
+
+        * page/DOMWindow.cpp:
+        (WebCore::DOMWindow::createWindow):
+        Update window.open() to return null instead of the window when
+        the 'noopener' feature is activated, as per:
+        - https://html.spec.whatwg.org/#dom-open (Step 10)
+
+        * page/WindowFeatures.cpp:
+        (WebCore::isSeparator):
+        Treat all ASCII spaces as feature separators, as per:
+        - https://html.spec.whatwg.org/#feature-separator
+        This has the effect of adding U+000C (FormFeed) as a separator.
+
+        (WebCore::processFeaturesString):
+        Align tokenizing code with the specification:
+        - https://html.spec.whatwg.org/#concept-window-open-features-tokenize
+        In particular, the following changes were made:
+        - After the key, skip to first '=', but don't skip past a ',' or a non-separator.
+          The "or a non-separator" part is new in the spec (step 3.6.1) and is now implemented.
+        - After looking for the '=', only treat what follows as a value if the current character
+          is a separator. This is as per step 7 in the spec.
+        These changes now cause us to parse 'foo noopener=1' as ('foo', ''), ('noopener', '1').
+
 2017-04-28  Eric Carlson  <eric.carlson@apple.com>
 
         Implement ondevicechange

diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
@@ -3175,7 +3175,7 @@ void Document::processViewport(const String& features, ViewportArguments::Type o
 
     m_viewportArguments = ViewportArguments(origin);
 
-    processFeaturesString(features, [this](StringView key, StringView value) {
+    processFeaturesString(features, FeatureMode::Viewport, [this](StringView key, StringView value) {
         setViewportFeature(m_viewportArguments, *this, key, value);
     });
 
@@ -3198,7 +3198,7 @@ void Document::updateViewportArguments()
 void Document::processFormatDetection(const String& features)
 {
     // FIXME: Find a better place for this function.
-    processFeaturesString(features, [this](StringView key, StringView value) {
+    processFeaturesString(features, FeatureMode::Viewport, [this](StringView key, StringView value) {
         if (equalLettersIgnoringASCIICase(key, "telephone") && equalLettersIgnoringASCIICase(value, "no"))
             setIsTelephoneNumberParsingAllowed(false);
     });

diff --git a/Source/WebCore/page/DOMWindow.cpp b/Source/WebCore/page/DOMWindow.cpp
@@ -2206,7 +2206,7 @@ RefPtr<Frame> DOMWindow::createWindow(const String& urlString, const AtomicStrin
     newFrame->page()->setOpenedByDOM();
 
     if (newFrame->document()->domWindow()->isInsecureScriptAccess(activeWindow, completedURL))
-        return newFrame;
+        return windowFeatures.noopener ? nullptr : newFrame;
 
     if (prepareDialogFunction)
         prepareDialogFunction(*newFrame->document()->domWindow());
@@ -2224,7 +2224,7 @@ RefPtr<Frame> DOMWindow::createWindow(const String& urlString, const AtomicStrin
     if (!newFrame->page())
         return nullptr;
 
-    return newFrame;
+    return windowFeatures.noopener ? nullptr : newFrame;
 }
 
 RefPtr<DOMWindow> DOMWindow::open(const String& urlString, const AtomicString& frameName, const String& windowFeaturesString, DOMWindow& activeWindow, DOMWindow& firstWindow)

diff --git a/Source/WebCore/page/WindowFeatures.cpp b/Source/WebCore/page/WindowFeatures.cpp
@@ -24,6 +24,7 @@
 #include "WindowFeatures.h"
 
 #include "FloatRect.h"
+#include <wtf/ASCIICType.h>
 #include <wtf/Assertions.h>
 #include <wtf/HashMap.h>
 #include <wtf/MathExtras.h>
@@ -39,9 +40,13 @@ static DialogFeaturesMap parseDialogFeaturesMap(const String&);
 static std::optional<bool> boolFeature(const DialogFeaturesMap&, const char* key);
 static std::optional<float> floatFeature(const DialogFeaturesMap&, const char* key, float min, float max);
 
-static bool isSeparator(UChar character)
+// https://html.spec.whatwg.org/#feature-separator
+static bool isSeparator(UChar character, FeatureMode mode)
 {
-    return character == ' ' || character == '\t' || character == '\n' || character == '\r' || character == '=' || character == ',';
+    if (mode == FeatureMode::Viewport)
+        return character == ' ' || character == '\t' || character == '\n' || character == '\r' || character == '=' || character == ',';
+
+    return isASCIISpace(character) || character == '=' || character == ',';
 }
 
 WindowFeatures parseWindowFeatures(StringView featuresString)
@@ -65,42 +70,47 @@ WindowFeatures parseWindowFeatures(StringView featuresString)
     features.scrollbarsVisible = false;
     features.noopener = false;
 
-    processFeaturesString(featuresString, [&features](StringView key, StringView value) {
+    processFeaturesString(featuresString, FeatureMode::Window, [&features](StringView key, StringView value) {
         setWindowFeature(features, key, value);
     });
 
     return features;
 }
 
-void processFeaturesString(StringView features, std::function<void(StringView type, StringView value)> callback)
+// Window: https://html.spec.whatwg.org/#concept-window-open-features-tokenize
+// Viewport: https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/MetaTags.html#//apple_ref/doc/uid/TP40008193-SW6
+// FIXME: We should considering aligning Viewport feature parsing with Window features parsing.
+void processFeaturesString(StringView features, FeatureMode mode, std::function<void(StringView type, StringView value)> callback)
 {
     unsigned length = features.length();
     for (unsigned i = 0; i < length; ) {
-        // skip to first non-separator
-        while (i < length && isSeparator(features[i]))
+        // Skip to first non-separator.
+        while (i < length && isSeparator(features[i], mode))
             ++i;
         unsigned keyBegin = i;
 
-        // skip to first separator
-        while (i < length && !isSeparator(features[i]))
+        // Skip to first separator.
+        while (i < length && !isSeparator(features[i], mode))
             i++;
         unsigned keyEnd = i;
 
-        // skip to first '=', but don't skip past a ','
-        while (i < length && features[i] != '=' && features[i] != ',')
-            ++i;
-
-        // skip to first non-separator, but don't skip past a ','
-        while (i < length && isSeparator(features[i]) && features[i] != ',')
-            ++i;
-        unsigned valueBegin = i;
-
-        // skip to first separator
-        while (i < length && !isSeparator(features[i]))
+        // Skip to first '=', but don't skip past a ',' or a non-separator.
+        while (i < length && features[i] != '=' && features[i] != ',' && (mode == FeatureMode::Viewport || isSeparator(features[i], mode)))
             ++i;
-        unsigned valueEnd = i;
 
-        callback(features.substring(keyBegin, keyEnd - keyBegin), features.substring(valueBegin, valueEnd - valueBegin));
+        // Skip to first non-separator, but don't skip past a ','.
+        if (mode == FeatureMode::Viewport || (i < length && isSeparator(features[i], mode))) {
+            while (i < length && isSeparator(features[i], mode) && features[i] != ',')
+                ++i;
+            unsigned valueBegin = i;
+
+            // Skip to first separator.
+            while (i < length && !isSeparator(features[i], mode))
+                ++i;
+            unsigned valueEnd = i;
+            callback(features.substring(keyBegin, keyEnd - keyBegin), features.substring(valueBegin, valueEnd - valueBegin));
+        } else
+            callback(features.substring(keyBegin, keyEnd - keyBegin), StringView());
     }
 }
 

diff --git a/Source/WebCore/page/WindowFeatures.h b/Source/WebCore/page/WindowFeatures.h
@@ -60,6 +60,7 @@ struct WindowFeatures {
 WindowFeatures parseWindowFeatures(StringView windowFeaturesString);
 WindowFeatures parseDialogFeatures(const String& dialogFeaturesString, const FloatRect& screenAvailableRect);
 
-void processFeaturesString(StringView features, std::function<void(StringView type, StringView value)> callback);
+enum class FeatureMode { Window, Viewport };
+void processFeaturesString(StringView features, FeatureMode, std::function<void(StringView type, StringView value)> callback);
 
 } // namespace WebCore