Bug 1931058 - Do not coerce the javascript: return value to a string. r=smaug,devtools-reviewers,nchevobbe

Update the code to follow the spec change:
9997cd93c6

Differential Revision: https://phabricator.services.mozilla.com/D229475

browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js

@@ -12,7 +12,7 @@ add_task(async function test_remote_window_open_js_uri() {
 
   Assert.ok(browser.isRemoteBrowser, "should be a remote browser");
 
-  BrowserTestUtils.startLoadingURIString(browser, `javascript:1;`);
+  BrowserTestUtils.startLoadingURIString(browser, `javascript:"1";`);
 
   await BrowserTestUtils.browserLoaded(browser);
 

devtools/shared/commands/resource/tests/browser_resources_sources.js

@@ -193,7 +193,7 @@ async function getExpectedResources(ignoreUnresurrectedSources = false) {
       },
       sourceContent: {
         contentType: "text/javascript",
-        source: "666",
+        source: "'666'",
       },
     },
     {

devtools/shared/commands/resource/tests/sources.html

@@ -46,7 +46,7 @@
     <!-- introductionType=srcScript mapped to scriptElement -->
     <script src="sources.js"></script>
     <!-- introductionType=javascriptURL -->
-    <iframe src="javascript:666"></iframe>
+    <iframe src="javascript:'666'"></iframe>
     <!-- srcdoc attribute on iframes -->
     <iframe srcdoc="<script>console.log('srcdoc')</script> <script>console.log('srcdoc 2')</script>"></iframe>
   </body>

dom/jsurl/nsJSProtocolHandler.cpp

@@ -179,61 +179,44 @@ static bool AllowedByCSP(nsIContentSecurityPolicy* aCSP,
   return (NS_SUCCEEDED(rv) && allowsInlineScript);
 }
 
-static bool IsPromiseValue(JSContext* aCx, JS::Handle<JS::Value> aValue) {
+// https://html.spec.whatwg.org/#evaluate-a-javascript:-url
-  if (!aValue.isObject()) {
+// Steps 7-10.
-    return false;
-  }
-
-  // We only care about Promise here, so CheckedUnwrapStatic is fine.
-  JS::Rooted<JSObject*> obj(aCx, js::CheckedUnwrapStatic(&aValue.toObject()));
-  if (!obj) {
-    return false;
-  }
-
-  return JS::IsPromiseObject(obj);
-}
-
-// Execute the compiled script a get the return value, coerced to a string.
 //
-// Copy the returned value into the mutable handle argument. In case of a
+// If the execution result is a string, |aRv| is set to success, and
-// evaluation failure either during the execution or the conversion of the
+// |aRetValue| is set to the string.
-// result to a string, the nsresult is be set to the corresponding result
-// code and the mutable handle argument remains unchanged.
 //
-// The value returned in the mutable handle argument is part of |aCx|'s
+// If the execution result is not a string, |aRv| is set to success, and
-// compartment. If the caller is in a different compartment, then the out-param
+// |aRetValue| is set to undefined.
-// value should be wrapped by calling |JS_WrapValue|.
 //
-static void ExecScriptAndCoerceToString(JSContext* aCx,
+// In case of a evaluation failure during the execution, |aRv| is set to the
+// corresponding result code and |aRetValue| remains unchanged.
+static void ExecScriptAndGetString(JSContext* aCx,
                                    JS::Handle<JSScript*> aScript,
                                    JS::MutableHandle<JS::Value> aRetValue,
                                    mozilla::ErrorResult& aRv) {
   MOZ_ASSERT(aScript);
 
+  // Step 7. Let evaluationStatus be the result of running the classic script
+  //         script.
   if (!JS_ExecuteScript(aCx, aScript, aRetValue)) {
     aRv.NoteJSContextException(aCx);
     return;
   }
 
-  if (IsPromiseValue(aCx, aRetValue)) {
+  // Step 8. Let result be null.
-    // We're a javascript: url and we should treat Promise return values as
+  // Step 9. If evaluationStatus is a normal completion, and
-    // undefined.
+  //         evaluationStatus.[[Value]] is a String, then set result to
-    //
+  //         evaluationStatus.[[Value]].
-    // Once bug 1477821 is fixed this code might be able to go away, or will
+  if (aRetValue.isString()) {
-    // become enshrined in the spec, depending.
-    aRetValue.setUndefined();
-  }
-
-  if (!aRetValue.isUndefined()) {
-    JSString* str = JS::ToString(aCx, aRetValue);
-    if (!str) {
-      // ToString can be a function call, so an exception can be raised while
-      // executing the function.
-      aRv.NoteJSContextException(aCx);
     return;
   }
-    aRetValue.set(JS::StringValue(str));
+
-  }
+  // Step 10. Otherwise, return null.
+  //
+  // NOTE: The `null` here is the return value of the entire algorithm.
+  //       This function returns `undefined` for all cases and let the caller
+  //       handle it.
+  aRetValue.setUndefined();
 }
 
 nsresult JSURLInputStream::EvaluateScript(
@@ -416,15 +399,16 @@ nsresult JSURLInputStream::EvaluateScript(
 
       if (!erv.Failed()) {
         MOZ_ASSERT(!options.noScriptRval);
-        ExecScriptAndCoerceToString(cx, compiledScript, &v, erv);
+        ExecScriptAndGetString(cx, compiledScript, &v, erv);
       }
     }
     rv = mozilla::dom::EvaluationExceptionToNSResult(erv);
   }
 
   js::AssertSameCompartment(cx, v);
+  MOZ_ASSERT(v.isString() || v.isUndefined());
 
-  if (NS_FAILED(rv) || !(v.isString() || v.isUndefined())) {
+  if (NS_FAILED(rv)) {
     return NS_ERROR_MALFORMED_URI;
   }
   if (v.isUndefined()) {

testing/web-platform/meta/url/javascript-urls.window.js.ini

@@ -1,2 +1,5 @@
 [javascript-urls.window.html]
-  expected: ERROR
+  [javascript: URL that fails to parse due to invalid host and has a U+0009 in scheme]
+    expected: FAIL
+  [javascript: URL without an opaque path]
+    expected: FAIL