[Extensions] Use SetLazyDataProperty on GinPort

Rather than using ObjectTemplateBuilder::SetProperty() on GinPort, use
ObjectTemplateBuilder::SetLazyDataProperty(). This allows author script
to reset the properties to be something else, which is more inline with
the majority of JS objects exposed to scripts.

Add tests for the same.

Bug: 927887
Change-Id: I8f983918014d69941992423eece5fd1eed9c58e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1506771Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638592}

chrome/test/data/extensions/api_test/bindings/invalidate_context/background.js

@@ -6,6 +6,7 @@ var frame;
 var frameRuntime;
 var frameStorage;
 var frameTabs;
+var nativeBindingsEnabled;
 
 function createFrame() {
   frame = document.createElement('iframe');
@@ -16,7 +17,7 @@ function createFrame() {
   });
 }
 
-function testPort(port) {
+function testPort(port, expectEventsValid) {
   var result = {
     disconnectThrow: false,
     postMessageThrow: false,
@@ -39,7 +40,7 @@ function testPort(port) {
   }
 
   try {
-    result.onDisconnect = port.onDisconnect;
+    result.onDisconnectEvent = port.onDisconnect;
   } catch (e) {
     result.getOnDisconnectThrow = true;
   }
@@ -52,13 +53,27 @@ function testPort(port) {
 
   chrome.test.assertTrue(result.postMessageThrow);
   chrome.test.assertTrue(result.disconnectThrow);
-  chrome.test.assertTrue(result.getOnMessageThrow);
-  chrome.test.assertTrue(result.getOnDisconnectThrow);
-  chrome.test.assertFalse(!!result.onMessageEvent);
-  chrome.test.assertFalse(!!result.onDisconnectEvent);
+
+  // With native bindings, the event object instantiated on a Port is set as a
+  // lazy data property, and thus is safe to access even after the context has
+  // been removed. JS bindings always throw errors when trying to access them
+  // after context invalidation.
+  expectEventsValid &= nativeBindingsEnabled;
+
+  if (expectEventsValid) {
+    chrome.test.assertFalse(result.getOnMessageThrow);
+    chrome.test.assertFalse(result.getOnDisconnectThrow);
+    chrome.test.assertTrue(!!result.onMessageEvent);
+    chrome.test.assertTrue(!!result.onDisconnectEvent);
+  } else {
+    chrome.test.assertTrue(result.getOnMessageThrow);
+    chrome.test.assertTrue(result.getOnDisconnectThrow);
+    chrome.test.assertEq(undefined, result.onMessageEvent);
+    chrome.test.assertEq(undefined, result.onDisconnectEvent);
+  }
 }
 
-chrome.test.runTests([
+const tests = [
   function useFrameStorageAndRuntime() {
     createFrame().then(() => {
       frameRuntime = frame.contentWindow.chrome.runtime;
@@ -104,7 +119,7 @@ chrome.test.runTests([
       port.postMessage;
       document.body.removeChild(frame);
 
-      testPort(port);
+      testPort(port, false);
       chrome.test.succeed();
     });
   },
@@ -117,7 +132,7 @@ chrome.test.runTests([
       port.disconnect();
       document.body.removeChild(frame);
 
-      testPort(port);
+      testPort(port, false);
       chrome.test.succeed();
     });
   },
@@ -131,8 +146,13 @@ chrome.test.runTests([
       port.onDisconnect;
       document.body.removeChild(frame);
 
-      testPort(port);
+      testPort(port, true);
       chrome.test.succeed();
     });
   },
-]);
+];
+
+chrome.test.getConfig((config) => {
+  nativeBindingsEnabled = config.nativeCrxBindingsEnabled;
+  chrome.test.runTests(tests);
+});

extensions/renderer/gin_port.cc

@@ -39,6 +39,7 @@ GinPort::GinPort(v8::Local<v8::Context> context,
       name_(name),
       event_handler_(event_handler),
       delegate_(delegate),
+      accessed_sender_(false),
       weak_factory_(this) {
   context_invalidation_listener_.emplace(
       context, base::BindOnce(&GinPort::OnContextInvalidated,
@@ -54,10 +55,10 @@ gin::ObjectTemplateBuilder GinPort::GetObjectTemplateBuilder(
   return Wrappable<GinPort>::GetObjectTemplateBuilder(isolate)
       .SetMethod("disconnect", &GinPort::DisconnectHandler)
       .SetMethod("postMessage", &GinPort::PostMessageHandler)
-      .SetProperty("name", &GinPort::GetName)
-      .SetProperty("onDisconnect", &GinPort::GetOnDisconnectEvent)
-      .SetProperty("onMessage", &GinPort::GetOnMessageEvent)
-      .SetProperty("sender", &GinPort::GetSender);
+      .SetLazyDataProperty("name", &GinPort::GetName)
+      .SetLazyDataProperty("onDisconnect", &GinPort::GetOnDisconnectEvent)
+      .SetLazyDataProperty("onMessage", &GinPort::GetOnMessageEvent)
+      .SetLazyDataProperty("sender", &GinPort::GetSender);
 }
 
 const char* GinPort::GetTypeName() {
@@ -108,6 +109,8 @@ void GinPort::DispatchOnDisconnect(v8::Local<v8::Context> context) {
 void GinPort::SetSender(v8::Local<v8::Context> context,
                         v8::Local<v8::Value> sender) {
   DCHECK_EQ(kActive, state_);
+  DCHECK(!accessed_sender_)
+      << "|sender| can only be set before its first access.";
 
   v8::Isolate* isolate = context->GetIsolate();
   v8::HandleScope handle_scope(isolate);
@@ -179,6 +182,7 @@ v8::Local<v8::Value> GinPort::GetOnMessageEvent(gin::Arguments* arguments) {
 }
 
 v8::Local<v8::Value> GinPort::GetSender(gin::Arguments* arguments) {
+  accessed_sender_ = true;
   v8::Isolate* isolate = arguments->isolate();
   v8::Local<v8::Object> wrapper = GetWrapper(isolate).ToLocalChecked();
   v8::Local<v8::Private> key =

extensions/renderer/gin_port.h

@@ -70,7 +70,9 @@ class GinPort final : public gin::Wrappable<GinPort> {
   // the port.
   void DispatchOnDisconnect(v8::Local<v8::Context> context);
 
-  // Sets the |sender| property on the port.
+  // Sets the |sender| property on the port. Note: this can only be called
+  // before the `sender` property is accessed on the JS object, since it is
+  // lazily set as a data property in first access.
   void SetSender(v8::Local<v8::Context> context, v8::Local<v8::Value> sender);
 
   const PortId& port_id() const { return port_id_; }
@@ -142,6 +144,10 @@ class GinPort final : public gin::Wrappable<GinPort> {
   // outlive this object.
   Delegate* const delegate_;
 
+  // Whether the `sender` property has been accessed, and thus set on the
+  // port JS object.
+  bool accessed_sender_;
+
   // A listener for context invalidation. Note: this isn't actually optional;
   // it just needs to be created after |weak_factory_|, which needs to be the
   // final member.

extensions/renderer/gin_port_unittest.cc

@@ -371,18 +371,24 @@ TEST_F(GinPortTest, TestSenderProperty) {
   v8::Local<v8::Context> context = MainContext();
 
   PortId port_id(base::UnguessableToken::Create(), 0, true);
-  gin::Handle<GinPort> port = CreatePort(context, port_id);
-
-  v8::Local<v8::Object> port_obj = port.ToV8().As<v8::Object>();
 
-  EXPECT_EQ("undefined",
-            GetStringPropertyFromObject(port_obj, context, "sender"));
-
-  port->SetSender(context,
-                  gin::DataObjectBuilder(isolate()).Set("prop", 42).Build());
+  {
+    gin::Handle<GinPort> port = CreatePort(context, port_id);
+    v8::Local<v8::Object> port_obj = port.ToV8().As<v8::Object>();
+    EXPECT_EQ("undefined",
+              GetStringPropertyFromObject(port_obj, context, "sender"));
+  }
 
-  EXPECT_EQ(R"({"prop":42})",
-            GetStringPropertyFromObject(port_obj, context, "sender"));
+  {
+    // SetSender() can only be called before the `sender` property is accessed,
+    // so we need to create a new port here.
+    gin::Handle<GinPort> port = CreatePort(context, port_id);
+    port->SetSender(context,
+                    gin::DataObjectBuilder(isolate()).Set("prop", 42).Build());
+    v8::Local<v8::Object> port_obj = port.ToV8().As<v8::Object>();
+    EXPECT_EQ(R"({"prop":42})",
+              GetStringPropertyFromObject(port_obj, context, "sender"));
+  }
 }
 
 TEST_F(GinPortTest, TryUsingPortAfterInvalidation) {
@@ -427,4 +433,22 @@ TEST_F(GinPortTest, TryUsingPortAfterInvalidation) {
   }
 }
 
+TEST_F(GinPortTest, AlteringPortName) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  PortId port_id(base::UnguessableToken::Create(), 0, true);
+  gin::Handle<GinPort> port = CreatePort(context, port_id);
+
+  v8::Local<v8::Object> port_obj = port.ToV8().As<v8::Object>();
+
+  v8::Local<v8::Function> change_port_name = FunctionFromString(
+      context, "(function(port) { port.name = 'foo'; return port.name; })");
+
+  v8::Local<v8::Value> args[] = {port_obj};
+  v8::Local<v8::Value> result =
+      RunFunction(change_port_name, context, base::size(args), args);
+  EXPECT_EQ(R"("foo")", V8ToString(result, context));
+}
+
 }  // namespace extensions