Auto merge of #26410 - utsavoza:ugo/issue-26287/04-05-2020, r=ferjm

Update window.open() to return fallible result

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes partially fix #26287
- [x] There are tests for these changes

6 files changed, 14 insertions, 135 deletions

diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs
index 3606ffc7c9d..eacd6a11745 100644
--- a/components/script/dom/document.rs
+++ b/components/script/dom/document.rs
@@ -4685,14 +4685,10 @@ impl DocumentMethods for Document {
         url: USVString,
         target: DOMString,
         features: DOMString,
-    ) -> Fallible<DomRoot<WindowProxy>> {
-        // WhatWG spec states this should always return a WindowProxy, but the spec for WindowProxy.open states
-        // it optionally returns a WindowProxy. Assume an error if window.open returns none.
-        // See https://github.com/whatwg/html/issues/4091
-        let context = self.browsing_context().ok_or(Error::InvalidAccess)?;
-        context
+    ) -> Fallible<Option<DomRoot<WindowProxy>>> {
+        self.browsing_context()
+            .ok_or(Error::InvalidAccess)?
             .open(url, target, features)
-            .ok_or(Error::InvalidAccess)
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-document-write
diff --git a/components/script/dom/webidls/Document.webidl b/components/script/dom/webidls/Document.webidl
index a9fe7fc982b..8ca4bbe50a1 100644
--- a/components/script/dom/webidls/Document.webidl
+++ b/components/script/dom/webidls/Document.webidl
@@ -124,7 +124,7 @@ partial /*sealed*/ interface Document {
   [CEReactions, Throws]
   Document open(optional DOMString unused1, optional DOMString unused2);
   [CEReactions, Throws]
-  WindowProxy open(USVString url, DOMString name, DOMString features);
+  WindowProxy? open(USVString url, DOMString name, DOMString features);
   [CEReactions, Throws]
   void close();
   [CEReactions, Throws]
diff --git a/components/script/dom/webidls/Window.webidl b/components/script/dom/webidls/Window.webidl
index 0c0b5e80860..f0b5b47d7af 100644
--- a/components/script/dom/webidls/Window.webidl
+++ b/components/script/dom/webidls/Window.webidl
@@ -40,8 +40,8 @@
   // https://github.com/whatwg/html/issues/2115
   [Replaceable] readonly attribute WindowProxy? parent;
   readonly attribute Element? frameElement;
-  WindowProxy? open(optional USVString url = "", optional DOMString target = "_blank",
-                    optional DOMString features = "");
+  [Throws] WindowProxy? open(optional USVString url = "", optional DOMString target = "_blank",
+                             optional DOMString features = "");
   //getter WindowProxy (unsigned long index);
 
   // https://github.com/servo/servo/issues/14453
diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs
index ccb4b8dfb48..93019cf913d 100644
--- a/components/script/dom/window.rs
+++ b/components/script/dom/window.rs
@@ -661,7 +661,7 @@ impl WindowMethods for Window {
         url: USVString,
         target: DOMString,
         features: DOMString,
-    ) -> Option<DomRoot<WindowProxy>> {
+    ) -> Fallible<Option<DomRoot<WindowProxy>>> {
         self.window_proxy().open(url, target, features)
     }
 
diff --git a/components/script/dom/windowproxy.rs b/components/script/dom/windowproxy.rs
index 4155d3626a2..14f9f80e20a 100644
--- a/components/script/dom/windowproxy.rs
+++ b/components/script/dom/windowproxy.rs
@@ -4,7 +4,7 @@
 
 use crate::dom::bindings::cell::DomRefCell;
 use crate::dom::bindings::conversions::{root_from_handleobject, ToJSValConvertible};
-use crate::dom::bindings::error::{throw_dom_exception, Error};
+use crate::dom::bindings::error::{throw_dom_exception, Error, Fallible};
 use crate::dom::bindings::inheritance::Castable;
 use crate::dom::bindings::proxyhandler::fill_property_descriptor;
 use crate::dom::bindings::reflector::{DomObject, Reflector};
@@ -415,7 +415,7 @@ impl WindowProxy {
         url: USVString,
         target: DOMString,
         features: DOMString,
-    ) -> Option<DomRoot<WindowProxy>> {
+    ) -> Fallible<Option<DomRoot<WindowProxy>>> {
         // Step 4.
         let non_empty_target = match target.as_ref() {
             "" => DOMString::from("_blank"),
@@ -433,12 +433,12 @@ impl WindowProxy {
         // Step 10, 11
         let (chosen, new) = match self.choose_browsing_context(non_empty_target, noopener) {
             (Some(chosen), new) => (chosen, new),
-            (None, _) => return None,
+            (None, _) => return Ok(None),
         };
         // TODO Step 12, set up browsing context features.
         let target_document = match chosen.document() {
             Some(target_document) => target_document,
-            None => return None,
+            None => return Ok(None),
         };
         let target_window = target_document.window();
         // Step 13, and 14.4, will have happened elsewhere,
@@ -452,7 +452,7 @@ impl WindowProxy {
             // Step 14.1
             let url = match existing_document.url().join(&url) {
                 Ok(url) => url,
-                Err(_) => return None, // TODO: throw a  "SyntaxError" DOMException.
+                Err(_) => return Err(Error::Syntax),
             };
             // Step 14.3
             let referrer = if noreferrer {
@@ -479,10 +479,10 @@ impl WindowProxy {
         }
         if noopener {
             // Step 15 (Dis-owning has been done in create_auxiliary_browsing_context).
-            return None;
+            return Ok(None);
         }
         // Step 17.
-        return target_document.browsing_context();
+        return Ok(target_document.browsing_context());
     }
 
     // https://html.spec.whatwg.org/multipage/#the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name
diff --git a/tests/wpt/metadata/url/failure.html.ini b/tests/wpt/metadata/url/failure.html.ini
index 56834eb76b6..b54dca88127 100644
--- a/tests/wpt/metadata/url/failure.html.ini
+++ b/tests/wpt/metadata/url/failure.html.ini
@@ -3,33 +3,12 @@
   [Test URL parser failure consistency]
     expected: FAIL
 
-  [window.open(): file://example:1/ should throw]
-    expected: FAIL
-
-  [window.open(): file://example:test/ should throw]
-    expected: FAIL
-
-  [window.open(): file://example%/ should throw]
-    expected: FAIL
-
-  [window.open(): file://[example\]/ should throw]
-    expected: FAIL
-
-  [window.open(): http://user:pass@/ should throw]
-    expected: FAIL
-
-  [window.open(): http://foo:-80/ should throw]
-    expected: FAIL
-
   [XHR: http:/:@/www.example.com should throw]
     expected: FAIL
 
   [window.open(): http:/:@/www.example.com should throw]
     expected: FAIL
 
-  [window.open(): http://user@/www.example.com should throw]
-    expected: FAIL
-
   [XHR: http:@/www.example.com should throw]
     expected: FAIL
 
@@ -42,12 +21,6 @@
   [window.open(): http:/@/www.example.com should throw]
     expected: FAIL
 
-  [window.open(): http://@/www.example.com should throw]
-    expected: FAIL
-
-  [window.open(): https:@/www.example.com should throw]
-    expected: FAIL
-
   [XHR: http:a:b@/www.example.com should throw]
     expected: FAIL
 
@@ -60,9 +33,6 @@
   [window.open(): http:/a:b@/www.example.com should throw]
     expected: FAIL
 
-  [window.open(): http://a:b@/www.example.com should throw]
-    expected: FAIL
-
   [XHR: http::@/www.example.com should throw]
     expected: FAIL
 
@@ -81,27 +51,6 @@
   [window.open(): http:/@:www.example.com should throw]
     expected: FAIL
 
-  [window.open(): http://@:www.example.com should throw]
-    expected: FAIL
-
-  [window.open(): https://� should throw]
-    expected: FAIL
-
-  [window.open(): https://%EF%BF%BD should throw]
-    expected: FAIL
-
-  [window.open(): https://x x:12 should throw]
-    expected: FAIL
-
-  [window.open(): http://[www.google.com\]/ should throw]
-    expected: FAIL
-
-  [window.open(): sc:/// should throw]
-    expected: FAIL
-
-  [window.open(): sc:// / should throw]
-    expected: FAIL
-
   [URL's constructor's base argument: sc://@/ should throw]
     expected: FAIL
 
@@ -195,69 +144,6 @@
   [window.open(): sc://:12/ should throw]
     expected: FAIL
 
-  [window.open(): sc://[/ should throw]
-    expected: FAIL
-
-  [window.open(): sc://\\/ should throw]
-    expected: FAIL
-
-  [window.open(): sc://\]/ should throw]
-    expected: FAIL
-
-  [window.open(): sc://\x00/ should throw]
-    expected: FAIL
-
-  [window.open(): ftp://example.com%80/ should throw]
-    expected: FAIL
-
-  [window.open(): ftp://example.com%A0/ should throw]
-    expected: FAIL
-
-  [window.open(): https://example.com%80/ should throw]
-    expected: FAIL
-
-  [window.open(): https://example.com%A0/ should throw]
-    expected: FAIL
-
-  [window.open(): https://0x100000000/test should throw]
-    expected: FAIL
-
-  [window.open(): https://256.0.0.1/test should throw]
-    expected: FAIL
-
-  [window.open(): https://[0::0::0\] should throw]
-    expected: FAIL
-
-  [window.open(): https://[0:.0\] should throw]
-    expected: FAIL
-
-  [window.open(): https://[0:0:\] should throw]
-    expected: FAIL
-
-  [window.open(): https://[0:1:2:3:4:5:6:7.0.0.0.1\] should throw]
-    expected: FAIL
-
-  [window.open(): https://[0:1.00.0.0.0\] should throw]
-    expected: FAIL
-
-  [window.open(): https://[0:1.290.0.0.0\] should throw]
-    expected: FAIL
-
-  [window.open(): https://[0:1.23.23\] should throw]
-    expected: FAIL
-
-  [window.open(): http://? should throw]
-    expected: FAIL
-
-  [window.open(): http://# should throw]
-    expected: FAIL
-
-  [window.open(): non-special://[:80/ should throw]
-    expected: FAIL
-
-  [window.open(): http://[::127.0.0.0.1\] should throw]
-    expected: FAIL
-
   [XHR: a should throw]
     expected: FAIL
 
@@ -276,6 +162,3 @@
   [window.open(): a// should throw]
     expected: FAIL
 
-  [window.open(): https://� should throw]
-    expected: FAIL
-