Auto merge of #24634 - paulrouget:url, r=jdm

Do not assume UWP and Servo agree on URL validity

This should address #24589.

Note: to check that a URL is valid, we try to parse the url and if an exception is raised, we assume the url is not valid. Even though we catch the exception, Visual Studio might "force" the crash + break here: https://github.com/servo/servo/compare/master...paulrouget:url?expand=1#diff-4e059561062fe024a35522f60f7f14c1R116 - I'm not sure why. In VS, in the Exception Setting, we need to uncheck the Break When Thrown option to avoid that crash. It only happens for few urls and I'm unclear why.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24589 (GitHub issue number if applicable)

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

ports/libsimpleservo/api/src/lib.rs

@@ -152,6 +152,12 @@ pub fn servo_version() -> String {
     servo::config::servo_version()
 }
 
+/// Test if a url is valid.
+pub fn is_uri_valid(url: &str) -> bool {
+    info!("load_uri: {}", url);
+    ServoUrl::parse(url).is_ok()
+}
+
 /// Initialize Servo. At that point, we need a valid GL context.
 /// In the future, this will be done in multiple steps.
 pub fn init(

ports/libsimpleservo/capi/src/lib.rs

@@ -186,16 +186,17 @@ fn do_redirect_stdout_stderr() -> Result<(), ()> {
     Ok(())
 }
 
-fn call<F>(f: F)
+fn call<T, F>(f: F) -> T
 where
-    F: Fn(&mut ServoGlue) -> Result<(), &'static str>,
+    F: Fn(&mut ServoGlue) -> Result<T, &'static str>,
 {
-    if let Err(e) = SERVO.with(|s| match s.borrow_mut().as_mut() {
+    match SERVO.with(|s| match s.borrow_mut().as_mut() {
         Some(ref mut s) => (f)(s),
         None => Err("Servo not available in this thread"),
     }) {
-        panic!(e);
+        Err(e) => panic!(e),
-    };
+        Ok(r) => r,
+    }
 }
 
 /// Callback used by Servo internals
@@ -430,13 +431,23 @@ pub extern "C" fn perform_updates() {
 }
 
 #[no_mangle]
-pub extern "C" fn load_uri(url: *const c_char) {
+pub extern "C" fn is_uri_valid(url: *const c_char) -> bool {
+    catch_any_panic(|| {
+        debug!("is_uri_valid");
+        let url = unsafe { CStr::from_ptr(url) };
+        let url = url.to_str().expect("Can't read string");
+        simpleservo::is_uri_valid(url)
+    })
+}
+
+#[no_mangle]
+pub extern "C" fn load_uri(url: *const c_char) -> bool {
     catch_any_panic(|| {
         debug!("load_url");
         let url = unsafe { CStr::from_ptr(url) };
         let url = url.to_str().expect("Can't read string");
-        call(|s| s.load_uri(url));
+        call(|s| Ok(s.load_uri(url).is_ok()))
-    });
+    })
 }
 
 #[no_mangle]

support/hololens/ServoApp/BrowserPage.cpp

@@ -116,7 +116,7 @@ void BrowserPage::OnURLEdited(IInspectable const &,
     servoControl().Focus(FocusState::Programmatic);
     auto input = urlTextbox().Text();
     auto uri = servoControl().LoadURIOrSearch(input);
-    urlTextbox().Text(uri.ToString());
+    urlTextbox().Text(uri);
   }
 }
 

support/hololens/ServoApp/ServoControl/Servo.h

@@ -66,7 +66,10 @@ public:
 
   void Reload() { capi::reload(); }
   void Stop() { capi::stop(); }
-  void LoadUri(hstring uri) { capi::load_uri(*hstring2char(uri)); }
+  bool LoadUri(hstring uri) { return capi::load_uri(*hstring2char(uri)); }
+  bool IsUriValid(hstring uri) {
+    return capi::is_uri_valid(*hstring2char(uri));
+  }
   void Scroll(float dx, float dy, float x, float y) {
     capi::scroll((int32_t)dx, (int32_t)dy, (int32_t)x, (int32_t)y);
   }

support/hololens/ServoApp/ServoControl/ServoControl.cpp

@@ -195,25 +195,43 @@ void ServoControl::Reload() {
 void ServoControl::Stop() {
   RunOnGLThread([=] { mServo->Stop(); });
 }
-Uri ServoControl::LoadURIOrSearch(hstring input) {
+hstring ServoControl::LoadURIOrSearch(hstring input) {
-  auto uri = TryParseURI(input);
+  // Initial input is valid
-  if (uri == std::nullopt) {
+  if (mServo->IsUriValid(input)) {
+    TryLoadUri(input);
+    return input;
+  }
+
+  // Not valid. Maybe it's just missing the scheme.
+  hstring with_scheme = L"https://" + input;
+  // If the user only types "mozilla" we don't want to go to
+  // https://mozilla even though it's a valid url.
   bool has_dot = wcsstr(input.c_str(), L".") != nullptr;
-    hstring input2 = L"https://" + input;
+  if (mServo->IsUriValid(with_scheme) && has_dot) {
-    uri = TryParseURI(input2);
+    TryLoadUri(with_scheme);
-    if (uri == std::nullopt || !has_dot) {
+    return with_scheme;
-      hstring input3 =
+  }
+
+  // Doesn't look like a URI. Let's search for the string.
+  hstring searchUri =
       L"https://duckduckgo.com/html/?q=" + Uri::EscapeComponent(input);
-      uri = TryParseURI(input3);
+  TryLoadUri(searchUri);
-    }
+  return searchUri;
-  }
+}
-  auto finalUri = uri.value();
+
+void ServoControl::TryLoadUri(hstring input) {
   if (!mLooping) {
-    mInitialURL = finalUri.ToString();
+    mInitialURL = input;
   } else {
-    RunOnGLThread([=] { mServo->LoadUri(finalUri.ToString()); });
+    RunOnGLThread([=] {
+      if (!mServo->LoadUri(input)) {
+        RunOnUIThread([=] {
+          Windows::UI::Popups::MessageDialog msg{L"URI not valid"};
+          msg.ShowAsync();
+        });
+      }
+    });
   }
-  return finalUri;
 }
 
 void ServoControl::RunOnGLThread(std::function<void()> task) {

support/hololens/ServoApp/ServoControl/ServoControl.h

@@ -14,7 +14,7 @@ struct ServoControl : ServoControlT<ServoControl>, public servo::ServoDelegate {
   void Reload();
   void Stop();
   void Shutdown();
-  Windows::Foundation::Uri LoadURIOrSearch(hstring);
+  hstring LoadURIOrSearch(hstring);
 
   void OnLoaded(IInspectable const &,
                 Windows::UI::Xaml::RoutedEventArgs const &);
@@ -110,14 +110,6 @@ private:
   void StopRenderLoop();
   void Loop();
 
-  std::optional<Windows::Foundation::Uri> TryParseURI(hstring input) {
-    try {
-      return Windows::Foundation::Uri(input);
-    } catch (hresult_invalid_argument const &) {
-      return {};
-    }
-  }
-
   void OnSurfaceTapped(IInspectable const &,
                        Windows::UI::Xaml::Input::TappedRoutedEventArgs const &);
 
@@ -147,6 +139,8 @@ private:
   template <typename Callable> void RunOnUIThread(Callable);
   void RunOnGLThread(std::function<void()>);
 
+  void TryLoadUri(hstring);
+
   std::unique_ptr<servo::Servo> mServo;
   EGLSurface mRenderSurface{EGL_NO_SURFACE};
   OpenGLES mOpenGLES;

support/hololens/ServoApp/ServoControl/ServoControl.idl

@@ -9,7 +9,7 @@ namespace ServoApp {
       void GoForward();
       void Reload();
       void Stop();
-      Windows.Foundation.Uri LoadURIOrSearch(String url);
+      String LoadURIOrSearch(String url);
       void SetTransientMode(Boolean transient);
       void SetArgs(String args);
       void Shutdown();