From 0cb56819a97cd0de768ce288809617e4084992d5 Mon Sep 17 00:00:00 2001
From: Tommy Martino <[email protected]>
Date: Wed, 06 May 2026 09:40:53 -0700
Subject: [PATCH] [iOS] Fix threading model of IOSBookmarkParser

This CL restructures the implementation of IOSBookmarkParser to avoid
potentially unsafe interactions caused by the use of WKWebView (a UIKit
class) on a non-main thread.

The new design quarantines all main thread interactions into a new
WebViewRunner. This class now owns the WKWebView and is SequenceBound
to the main/UI thread, so all WebKit interactions (construction,
destruction, loading, triggering, receiving result) occur on main. The
actual JS execution happens out-of-process.

The main IOSBookmarkParser class (designed to be run off-main) still
handles all potentially-expensive operations, and now communicates
safely with WebViewRunner across sequence boundaries. The loaded script
and the JSON result of parsing are now passed-by-move as std::strings,
to ensure thread-safety.

Fixed: 504194494

Change-Id: Ic31502f4c218c26d8b338f36f5b27f911431993d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7810016
Reviewed-by: Alexis Hétu <[email protected]>
Commit-Queue: Tommy Martino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1626247}
---

diff --git a/components/user_data_importer/ios/BUILD.gn b/components/user_data_importer/ios/BUILD.gn
index a1440db..64694d1 100644
--- a/components/user_data_importer/ios/BUILD.gn
+++ b/components/user_data_importer/ios/BUILD.gn
@@ -16,6 +16,7 @@
     "//base",
     "//components/user_data_importer/utility:bookmarks",
     "//ios/web/public/js_messaging:web_view_js_utils",
+    "//ios/web/public/thread",
     "//url",
   ]
 }
diff --git a/components/user_data_importer/ios/ios_bookmark_parser.h b/components/user_data_importer/ios/ios_bookmark_parser.h
index c46d692..1d3341f 100644
--- a/components/user_data_importer/ios/ios_bookmark_parser.h
+++ b/components/user_data_importer/ios/ios_bookmark_parser.h
@@ -5,10 +5,11 @@
 #ifndef COMPONENTS_USER_DATA_IMPORTER_IOS_IOS_BOOKMARK_PARSER_H_
 #define COMPONENTS_USER_DATA_IMPORTER_IOS_IOS_BOOKMARK_PARSER_H_
 
-#import "components/user_data_importer/utility/bookmark_parser.h"
+#include <string>
 
-@class LocalNavigationForwarder;
-@class WKWebView;
+#include "base/threading/sequence_bound.h"
+#include "base/values.h"
+#import "components/user_data_importer/utility/bookmark_parser.h"
 
 namespace base {
 class FilePath;
@@ -16,6 +17,8 @@
 
 namespace user_data_importer {
 
+class WebViewRunner;
+
 // iOS implementation of the BookmarkParser interface. Uses WKWebView as a
 // JavaScript environment where parsing can occur in a memory-safe language.
 class IOSBookmarkParser : public BookmarkParser {
@@ -27,18 +30,15 @@
              BookmarkParser::BookmarkParsingCallback callback) override;
 
  private:
-  // Injects JS into the WebView to cause parsing of the currently loaded
-  // content.
-  void TriggerParseInJS(BookmarkParser::BookmarkParsingCallback callback);
+  // Invoked on this object's default sequence when the WebViewRunner has
+  // completed parsing in JavaScript.
+  void OnJSResult(BookmarkParser::BookmarkParsingCallback callback,
+                  std::string result,
+                  NSError* error);
 
-  // Delegate used to observe loading in `web_view_` and trigger parsing at the
-  // appropriate time. Declared as a member here because it is not retained
-  // by the WebView once set.
-  LocalNavigationForwarder* forwarder_;
-
-  // Environment where the bookmarks HTML file is loaded and JS is executed
-  // to parse the contents.
-  WKWebView* web_view_;
+  // Encapsulates the parts of this flow that must be run on the main (UI)
+  // thread.
+  base::SequenceBound<WebViewRunner> runner_;
 
   base::WeakPtrFactory<IOSBookmarkParser> weak_factory_{this};
 };
diff --git a/components/user_data_importer/ios/ios_bookmark_parser.mm b/components/user_data_importer/ios/ios_bookmark_parser.mm
index 80aef5e..ee028742 100644
--- a/components/user_data_importer/ios/ios_bookmark_parser.mm
+++ b/components/user_data_importer/ios/ios_bookmark_parser.mm
@@ -8,11 +8,18 @@
 
 #import "base/apple/foundation_util.h"
 #import "base/functional/callback_helpers.h"
+#import "base/json/json_reader.h"
 #import "base/memory/weak_ptr.h"
+#import "base/strings/sys_string_conversions.h"
 #import "base/strings/utf_string_conversions.h"
+#import "base/task/bind_post_task.h"
+#import "base/task/sequenced_task_runner.h"
+#import "base/threading/sequence_bound.h"
 #import "base/types/expected_macros.h"
 #import "base/values.h"
 #import "ios/web/public/js_messaging/web_view_js_utils.h"
+#import "ios/web/public/thread/web_task_traits.h"
+#import "ios/web/public/thread/web_thread.h"
 #import "url/gurl.h"
 
 // Object that conforms to WKNavigationDelegate and runs a provided OnceClosure
@@ -69,6 +76,8 @@
 
 namespace user_data_importer {
 
+using JSONOrErrorCallback = base::OnceCallback<void(std::string, NSError*)>;
+
 namespace {
 
 // Turns a list representing a path to a bookmark/folder from a JSON-source
@@ -150,19 +159,12 @@
 
 // Transforms the result or error of the JS call into a result or error suitable
 // for invoking a BookmarkParsingCallback.
-BookmarkParser::BookmarkParsingResult TranslateJSResult(id result,
-                                                        NSError* error) {
-  if (error) {
+BookmarkParser::BookmarkParsingResult TranslateJSResult(base::Value value) {
+  if (!value.is_dict()) {
     return base::unexpected(
         BookmarkParser::BookmarkParsingError::kParsingFailed);
   }
-  std::unique_ptr<base::Value> value_result =
-      web::ValueResultFromWKResult(result);
-  if (!value_result || !value_result->is_dict()) {
-    return base::unexpected(
-        BookmarkParser::BookmarkParsingError::kParsingFailed);
-  }
-  base::DictValue dict = std::move(*value_result).TakeDict();
+  base::DictValue dict = std::move(value).TakeDict();
 
   BookmarkParser::ParsedBookmarks parsing_result;
 
@@ -188,49 +190,88 @@
 
 }  // namespace
 
+// Helper class that encapsulates the parts of the import flow that interact
+// with WKWebView and thus must run on the main thread. Because this is on the
+// main thread, it should be kept to the minimal necessary set of work; in
+// particular, expensive work like file I/O or parsing belongs in the main
+// class (which is safe to use on non-main sequences).
+class WebViewRunner {
+ public:
+  WebViewRunner() {
+    web_view_ =
+        [[WKWebView alloc] initWithFrame:CGRectZero
+                           configuration:[[WKWebViewConfiguration alloc] init]];
+  }
+
+  ~WebViewRunner() { web_view_.navigationDelegate = nil; }
+
+  // Parses the given `file` using the given `script`. Executes `callback` with
+  // the returned JSON or an NSError if execution fails.
+  void LoadAndParse(base::FilePath file,
+                    std::string script,
+                    JSONOrErrorCallback callback) {
+    NSURL* url = base::apple::FilePathToNSURL(file);
+    NSMutableURLRequest* request = [NSMutableURLRequest requestWithURL:url];
+    [request addValue:@"text/html; charset=utf-8"
+        forHTTPHeaderField:@"Content-Type"];
+
+    // Configure the WKWebView so that the parsing JS is injected and run once
+    // the content has loaded.
+    base::OnceClosure on_load = base::BindOnce(
+        &WebViewRunner::TriggerParseInJS, weak_factory_.GetWeakPtr(),
+        std::move(script), std::move(callback));
+    forwarder_ =
+        [[LocalNavigationForwarder alloc] initWithClosure:std::move(on_load)];
+    web_view_.navigationDelegate = forwarder_;
+
+    // Passing `url` as the second parameter prevents any resources other than
+    // `url` from being opened (e.g. in iframes). This is a security mitigation,
+    // so don't change it unless you're sure you're doing the right thing.
+    [web_view_ loadFileRequest:request allowingReadAccessToURL:url];
+  }
+
+ private:
+  // Executes the given `script` in `web_view_`. Should be run only once the
+  // target file has finished loading in `web_view_`.
+  void TriggerParseInJS(std::string script, JSONOrErrorCallback callback) {

diff --git a/components/user_data_importer/utility/safari_data_importer_unittest.cc b/components/user_data_importer/utility/safari_data_importer_unittest.cc
index 8716c81..0ae0e402 100644
--- a/components/user_data_importer/utility/safari_data_importer_unittest.cc
+++ b/components/user_data_importer/utility/safari_data_importer_unittest.cc
@@ -51,12 +51,14 @@
 #include "mojo/public/cpp/bindings/remote.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#if !BUILDFLAG(IS_IOS)
+#if BUILDFLAG(IS_IOS)
+#include "ios/web/public/test/web_task_environment.h"
+#else
 #include "components/user_data_importer/content/content_bookmark_parser.h"
 #include "components/user_data_importer/content/fake_bookmark_html_parser.h"
 #include "components/user_data_importer/mojom/bookmark_html_parser.mojom.h"
 #include "content/public/test/browser_task_environment.h"  // nogncheck
-#endif  // !BUILDFLAG(IS_IOS)
+#endif  // BUILDFLAG(IS_IOS)
 
 using bookmarks::test::IsFolder;
 using bookmarks::test::IsUrlBookmark;
@@ -132,7 +134,7 @@
 
  protected:
 #if BUILDFLAG(IS_IOS)
-  base::test::TaskEnvironment task_environment_{
+  web::WebTaskEnvironment task_environment_{
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
 #else
   content::BrowserTaskEnvironment task_environment_{