Always create JavaScriptContentWorld with a WKContentWorld

This ensures that the execution of JavaScript goes through WKFrameInfo instead of `WebFrameImpl::EncryptPayload`. The `__gCrWeb.message.routeMessage` API is not yet removed in this CL, but a NOTREACHED() call is added at the point where this code is called. Since it is no longer used, outdated tests which verify these code paths are removed in this CL. The API itself will be removed in a follow-up CL. Bug: 1322482 Change-Id: I33eb36e3f0d26c902ca2541b1624817e5cca55ed Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3696560 Commit-Queue: Mike Dougherty <[email protected]> Reviewed-by: Ali Juma <[email protected]> Cr-Commit-Position: refs/heads/main@{#1018275}
hadissulejmani · Jun 27, 2022 · dd207fc · dd207fc
1 parent bfc4ae0
commit dd207fc
Show file tree

Hide file tree

Showing 9 changed files with 65 additions and 326 deletions.
diff --git a/ios/web/js_messaging/BUILD.gn b/ios/web/js_messaging/BUILD.gn
@@ -10,6 +10,7 @@ source_set("js_messaging") {
   deps = [
     ":frame_listeners_js",
     ":java_script_content_world_header",
+    ":java_script_feature_manager_header",
     ":scoped_wk_script_message_handler",
     ":setup_frame_js",
     ":web_frames_manager_impl_header",
@@ -58,7 +59,10 @@ source_set("web_frames_manager_impl_header") {
 
 source_set("java_script_feature") {
   configs += [ "//build/config/compiler:enable_arc" ]
-  public_deps = [ ":java_script_content_world_header" ]
+  public_deps = [
+    ":java_script_content_world_header",
+    ":java_script_feature_manager_header",
+  ]
   deps = [
     ":js_messaging",
     ":scoped_wk_script_message_handler",
@@ -73,7 +77,6 @@ source_set("java_script_feature") {
   sources = [
     "java_script_content_world.mm",
     "java_script_feature.mm",
-    "java_script_feature_manager.h",
     "java_script_feature_manager.mm",
     "script_message.mm",
   ]
@@ -91,6 +94,17 @@ source_set("java_script_content_world_header") {
   sources = [ "java_script_content_world.h" ]
 }
 
+source_set("java_script_feature_manager_header") {
+  configs += [ "//build/config/compiler:enable_arc" ]
+
+  deps = [
+    ":java_script_content_world_header",
+    "//base",
+  ]
+
+  sources = [ "java_script_feature_manager.h" ]
+}
+
 source_set("java_script_feature_util") {
   configs += [ "//build/config/compiler:enable_arc" ]
 

diff --git a/ios/web/js_messaging/java_script_content_world.h b/ios/web/js_messaging/java_script_content_world.h
@@ -33,10 +33,6 @@ class JavaScriptContentWorld {
   ~JavaScriptContentWorld();
   JavaScriptContentWorld(const JavaScriptContentWorld&) = delete;
 
-  // Creates a content world for features which will interact with the page
-  // content world shared by the webpage's JavaScript.
-  explicit JavaScriptContentWorld(BrowserState* browser_state);
-
   // Creates a content world for features which will interact with the given
   // |content_world|.
   JavaScriptContentWorld(BrowserState* browser_state,

diff --git a/ios/web/js_messaging/java_script_content_world.mm b/ios/web/js_messaging/java_script_content_world.mm
@@ -53,11 +53,6 @@ WKUserScriptInjectionTime InjectionTimeToWKUserScriptInjectionTime(
 
 }  // namespace
 
-JavaScriptContentWorld::JavaScriptContentWorld(BrowserState* browser_state)
-    : browser_state_(browser_state),
-      user_content_controller_(GetUserContentController(browser_state)),
-      weak_factory_(this) {}
-
 #if defined(__IPHONE_14_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_14_0
 JavaScriptContentWorld::JavaScriptContentWorld(BrowserState* browser_state,
                                                WKContentWorld* content_world)

diff --git a/ios/web/js_messaging/java_script_content_world_unittest.mm b/ios/web/js_messaging/java_script_content_world_unittest.mm
@@ -30,13 +30,14 @@
       [[user_content_controller userScripts] count];
   ASSERT_GT(initial_scripts_count, 0ul);
 
-  web::JavaScriptContentWorld world(GetBrowserState());
+  web::JavaScriptContentWorld world(GetBrowserState(),
+                                    WKContentWorld.pageWorld);
 
   FakeJavaScriptFeature feature(
       JavaScriptFeature::ContentWorld::kAnyContentWorld);
   world.AddFeature(&feature);
   EXPECT_TRUE(world.HasFeature(&feature));
-  EXPECT_FALSE(world.GetWKContentWorld());
+  EXPECT_EQ(WKContentWorld.pageWorld, world.GetWKContentWorld());
 
   unsigned long scripts_count = [[user_content_controller userScripts] count];
   ASSERT_GT(scripts_count, initial_scripts_count);
@@ -54,14 +55,14 @@ FakeJavaScriptFeature feature(
   ASSERT_GT(initial_scripts_count, 0ul);
 
   web::JavaScriptContentWorld world(GetBrowserState(),
-                                    [WKContentWorld defaultClientWorld]);
+                                    WKContentWorld.defaultClientWorld);
 
   FakeJavaScriptFeature feature(
       JavaScriptFeature::ContentWorld::kAnyContentWorld);
   world.AddFeature(&feature);
   EXPECT_TRUE(world.HasFeature(&feature));
 
-  EXPECT_EQ([WKContentWorld defaultClientWorld], world.GetWKContentWorld());
+  EXPECT_EQ(WKContentWorld.defaultClientWorld, world.GetWKContentWorld());
 
   unsigned long scripts_count = [[user_content_controller userScripts] count];
   ASSERT_GT(scripts_count, initial_scripts_count);
@@ -79,14 +80,14 @@ FakeJavaScriptFeature feature(
   ASSERT_GT(initial_scripts_count, 0ul);
 
   web::JavaScriptContentWorld world(GetBrowserState(),
-                                    [WKContentWorld defaultClientWorld]);
+                                    WKContentWorld.defaultClientWorld);
 
   FakeJavaScriptFeature feature(
       JavaScriptFeature::ContentWorld::kIsolatedWorldOnly);
   world.AddFeature(&feature);
   EXPECT_TRUE(world.HasFeature(&feature));
 
-  EXPECT_EQ([WKContentWorld defaultClientWorld], world.GetWKContentWorld());
+  EXPECT_EQ(WKContentWorld.defaultClientWorld, world.GetWKContentWorld());
 
   unsigned long scripts_count = [[user_content_controller userScripts] count];
   ASSERT_GT(scripts_count, initial_scripts_count);
@@ -96,7 +97,7 @@ FakeJavaScriptFeature feature(
 // content world triggers a DCHECK.
 TEST_F(JavaScriptContentWorldTest, AddIsolatedWorldFeatureToPageWorld) {
   web::JavaScriptContentWorld world(GetBrowserState(),
-                                    [WKContentWorld pageWorld]);
+                                    WKContentWorld.pageWorld);
   FakeJavaScriptFeature feature(
       JavaScriptFeature::ContentWorld::kIsolatedWorldOnly);
 

diff --git a/ios/web/js_messaging/java_script_feature_manager.h b/ios/web/js_messaging/java_script_feature_manager.h
@@ -32,6 +32,13 @@ class JavaScriptFeatureManager : public base::SupportsUserData::Data {
   static JavaScriptFeatureManager* FromBrowserState(
       BrowserState* browser_state);
 
+  // Returns the JavaScriptContentWorld for the page content world associated
+  // with |browser_state|. If a JavaScriptFeatureManager does not already exist,
+  // one will be created and associated with |browser_state|. |browser_state|
+  // must not be null.
+  static JavaScriptContentWorld* GetPageContentWorldForBrowserState(
+      BrowserState* browser_state);
+
   // Configures |features| on |user_content_controller_| by adding user scripts
   // and script message handlers.
   // NOTE: |page_content_world_| and |isolated_world_| will be recreated.

diff --git a/ios/web/js_messaging/java_script_feature_manager.mm b/ios/web/js_messaging/java_script_feature_manager.mm
@@ -58,10 +58,18 @@ void AddSharedCommonFeatures(web::JavaScriptContentWorld* world) {
   return feature_manager;
 }
 
+JavaScriptContentWorld*
+JavaScriptFeatureManager::GetPageContentWorldForBrowserState(
+    BrowserState* browser_state) {
+  DCHECK(browser_state);
+  JavaScriptFeatureManager* feature_manager = FromBrowserState(browser_state);
+  return feature_manager->page_content_world_.get();
+}
+
 void JavaScriptFeatureManager::ConfigureFeatures(
     std::vector<JavaScriptFeature*> features) {
-  page_content_world_ =
-      std::make_unique<JavaScriptContentWorld>(browser_state_);
+  page_content_world_ = std::make_unique<JavaScriptContentWorld>(
+      browser_state_, WKContentWorld.pageWorld);
   AddSharedCommonFeatures(page_content_world_.get());
 
   isolated_world_ = std::make_unique<JavaScriptContentWorld>(

diff --git a/ios/web/js_messaging/web_frame_impl.mm b/ios/web/js_messaging/web_frame_impl.mm
@@ -20,6 +20,7 @@
 #include "crypto/aead.h"
 #include "crypto/random.h"
 #import "ios/web/js_messaging/java_script_content_world.h"
+#import "ios/web/js_messaging/java_script_feature_manager.h"
 #import "ios/web/js_messaging/web_view_js_utils.h"
 #include "ios/web/public/thread/web_task_traits.h"
 #include "ios/web/public/thread/web_thread.h"
@@ -114,7 +115,7 @@
   // because calling the function directly on the webstate with
   // |ExecuteJavaScript| is secure. However, iframes require an encryption key
   // in order to securely pass the function name and parameters to the frame.
-  return is_main_frame_ || frame_key_;
+  return is_main_frame_ || frame_key_ || frame_info_;
 }
 
 BrowserState* WebFrameImpl::GetBrowserState() {
@@ -173,6 +174,11 @@
                                      reply_with_result);
   }
 
+  // There should always be a content_world now and
+  // `__gCrWeb.message.routeMessage` calls shouldn't be necessary.
+  // TODO(crbug.com/1339441): Remove custom iFrame messaging system.
+  NOTREACHED();
+
   base::Value message_payload(base::Value::Type::DICTIONARY);
   message_payload.SetKey("messageId", base::Value(message_id));
   message_payload.SetKey("replyWithResult", base::Value(reply_with_result));
@@ -203,8 +209,11 @@
 bool WebFrameImpl::CallJavaScriptFunction(
     const std::string& name,
     const std::vector<base::Value>& parameters) {
-  return CallJavaScriptFunctionInContentWorld(name, parameters,
-                                              /*content_world=*/nullptr,
+  JavaScriptContentWorld* content_world =
+      JavaScriptFeatureManager::GetPageContentWorldForBrowserState(
+          GetBrowserState());
+
+  return CallJavaScriptFunctionInContentWorld(name, parameters, content_world,
                                               /*reply_with_result=*/false);
 }
 
@@ -221,8 +230,10 @@
     const std::vector<base::Value>& parameters,
     base::OnceCallback<void(const base::Value*)> callback,
     base::TimeDelta timeout) {
-  return CallJavaScriptFunctionInContentWorld(name, parameters,
-                                              /*content_world=*/nullptr,
+  JavaScriptContentWorld* content_world =
+      JavaScriptFeatureManager::GetPageContentWorldForBrowserState(
+          GetBrowserState());
+  return CallJavaScriptFunctionInContentWorld(name, parameters, content_world,
                                               std::move(callback), timeout);
 }