V8ValueConverter for the activity logger that does not invoke interceptors and

accessors when converting V8 values to base values.

BUG=260978, 259093

Review URL: https://chromiumcodereview.appspot.com/19730002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@217032 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 299 insertions, 6 deletions

diff --git a/chrome/chrome_renderer.gypi b/chrome/chrome_renderer.gypi
index ccb508e..5b0d88c 100644
--- a/chrome/chrome_renderer.gypi
+++ b/chrome/chrome_renderer.gypi
@@ -49,6 +49,8 @@
       'sources': [
         'renderer/benchmarking_extension.cc',
         'renderer/benchmarking_extension.h',
+        'renderer/extensions/activity_log_converter_strategy.cc',
+        'renderer/extensions/activity_log_converter_strategy.h',
         'renderer/extensions/api_activity_logger.cc',
         'renderer/extensions/api_activity_logger.h',
         'renderer/extensions/api_definitions_natives.cc',
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index e2dda05..6fdcd3c 100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -1786,6 +1786,7 @@
         'common/worker_thread_ticker_unittest.cc',
         'renderer/chrome_content_renderer_client_unittest.cc',
         'renderer/content_settings_observer_unittest.cc',
+        'renderer/extensions/activity_log_converter_strategy_unittest.cc',
         'renderer/extensions/chrome_v8_context_set_unittest.cc',
         'renderer/extensions/event_unittest.cc',
         'renderer/extensions/extension_localization_peer_unittest.cc',
diff --git a/chrome/renderer/extensions/activity_log_converter_strategy.cc b/chrome/renderer/extensions/activity_log_converter_strategy.cc
new file mode 100644
index 0000000..1c5906e
--- /dev/null
+++ b/chrome/renderer/extensions/activity_log_converter_strategy.cc
@@ -0,0 +1,54 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/renderer/extensions/activity_log_converter_strategy.h"
+#include "base/values.h"
+
+namespace extensions {
+
+bool ActivityLogConverterStrategy::FromV8Object(v8::Handle<v8::Object> value,
+                                                base::Value** out) const {
+  return FromV8ObjectInternal(value, out);
+}
+
+bool ActivityLogConverterStrategy::FromV8Array(v8::Handle<v8::Array> value,
+                                               base::Value** out) const {
+  return FromV8ObjectInternal(value, out);
+}
+
+bool ActivityLogConverterStrategy::FromV8ObjectInternal(
+    v8::Handle<v8::Object> value,
+    base::Value** out) const {
+
+  // Handle JSObject.
+  // We cannot use value->Get(key/index) as there may be a getter method,
+  // accessor, interceptor/handler, or access check callback set on the
+  // property. If that it is the case, any of those may invoke JS code that
+  // may result on logging extension activity events caused by value conversion
+  // rather than extension itself.
+
+  // V8 arrays are handled here in the same way as other JSObjects as they may
+  // also have getter methods, accessor, interceptor/handler, and access check
+  // callback.
+
+  v8::Handle<v8::String> name = v8::String::New("[");
+  if (value->IsFunction()) {
+    name =  v8::String::Concat(name, v8::String::New("Function"));
+    v8::Handle<v8::Value> fname =
+        v8::Handle<v8::Function>::Cast(value)->GetName();
+    if (fname->IsString() && v8::Handle<v8::String>::Cast(fname)->Length()) {
+      name = v8::String::Concat(name, v8::String::New(" "));
+      name = v8::String::Concat(name, v8::Handle<v8::String>::Cast(fname));
+      name = v8::String::Concat(name, v8::String::New("()"));
+    }
+  } else {
+    name = v8::String::Concat(name, value->GetConstructorName());
+  }
+  name = v8::String::Concat(name, v8::String::New("]"));
+  *out = new base::StringValue(std::string(*v8::String::Utf8Value(name)));
+  // Prevent V8ValueConverter from further processing this object.
+  return true;
+}
+
+}  // namespace extensions
diff --git a/chrome/renderer/extensions/activity_log_converter_strategy.h b/chrome/renderer/extensions/activity_log_converter_strategy.h
new file mode 100644
index 0000000..aa5af28
--- /dev/null
+++ b/chrome/renderer/extensions/activity_log_converter_strategy.h
@@ -0,0 +1,36 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_RENDERER_EXTENSIONS_ACTIVITY_LOG_CONVERTER_STRATEGY_H_
+#define CHROME_RENDERER_EXTENSIONS_ACTIVITY_LOG_CONVERTER_STRATEGY_H_
+
+#include "base/compiler_specific.h"
+#include "content/public/renderer/v8_value_converter.h"
+#include "v8/include/v8.h"
+
+namespace extensions {
+
+// This class is used by activity logger and extends behavior of
+// V8ValueConverter. It overwrites conversion of V8 arrays and objects.  When
+// converting arrays and objects, we must not invoke any JS code which may
+// result in triggering activity logger. In such case, the log entries will be
+// generated due to V8 object conversion rather than extension activity.
+class ActivityLogConverterStrategy
+    : public content::V8ValueConverter::Strategy {
+ public:
+  virtual ~ActivityLogConverterStrategy() {}
+
+  virtual bool FromV8Object(v8::Handle<v8::Object> value,
+                            base::Value** out) const OVERRIDE;
+  virtual bool FromV8Array(v8::Handle<v8::Array> value,
+                           base::Value** out) const OVERRIDE;
+
+ private:
+  bool FromV8ObjectInternal(v8::Handle<v8::Object> value,
+                            base::Value** out) const;
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_RENDERER_EXTENSIONS_ACTIVITY_LOG_CONVERTER_STRATEGY_H_
diff --git a/chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc b/chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc
new file mode 100644
index 0000000..b113d7a
--- /dev/null
+++ b/chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc
@@ -0,0 +1,158 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/memory/scoped_ptr.h"
+#include "base/values.h"
+#include "chrome/renderer/extensions/activity_log_converter_strategy.h"
+#include "chrome/renderer/extensions/scoped_persistent.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "v8/include/v8.h"
+
+using content::V8ValueConverter;
+
+namespace extensions {
+
+class ActivityLogConverterStrategyTest : public testing::Test {
+ public:
+  ActivityLogConverterStrategyTest()
+      : isolate_(v8::Isolate::GetCurrent())
+      , handle_scope_(isolate_)
+      , context_(v8::Context::New(isolate_))
+      , context_scope_(context_.get()) {
+  }
+
+ protected:
+  virtual void SetUp() {
+    converter_.reset(V8ValueConverter::create());
+    strategy_.reset(new ActivityLogConverterStrategy());
+    converter_->SetFunctionAllowed(true);
+    converter_->SetStrategy(strategy_.get());
+  }
+
+  testing::AssertionResult VerifyNull(v8::Local<v8::Value> v8_value) {
+    scoped_ptr<base::Value> value(
+        converter_->FromV8Value(v8_value, context_.get()));
+    if (value->IsType(base::Value::TYPE_NULL))
+      return testing::AssertionSuccess();
+    return testing::AssertionFailure();
+  }
+
+  testing::AssertionResult VerifyBoolean(v8::Local<v8::Value> v8_value,
+                                         bool expected) {
+    bool out;
+    scoped_ptr<base::Value> value(
+        converter_->FromV8Value(v8_value, context_.get()));
+    if (value->IsType(base::Value::TYPE_BOOLEAN)
+        && value->GetAsBoolean(&out)
+        && out == expected)
+      return testing::AssertionSuccess();
+    return testing::AssertionFailure();
+  }
+
+  testing::AssertionResult VerifyInteger(v8::Local<v8::Value> v8_value,
+                                         int expected) {
+    int out;
+    scoped_ptr<base::Value> value(
+        converter_->FromV8Value(v8_value, context_.get()));
+    if (value->IsType(base::Value::TYPE_INTEGER)
+        && value->GetAsInteger(&out)
+        && out == expected)
+      return testing::AssertionSuccess();
+    return testing::AssertionFailure();
+  }
+
+  testing::AssertionResult VerifyDouble(v8::Local<v8::Value> v8_value,
+                                        double expected) {
+    double out;
+    scoped_ptr<base::Value> value(
+        converter_->FromV8Value(v8_value, context_.get()));
+    if (value->IsType(base::Value::TYPE_DOUBLE)
+        && value->GetAsDouble(&out)
+        && out == expected)
+      return testing::AssertionSuccess();
+    return testing::AssertionFailure();
+  }
+
+  testing::AssertionResult VerifyString(v8::Local<v8::Value> v8_value,
+                                        const std::string& expected) {
+    std::string out;
+    scoped_ptr<base::Value> value(
+        converter_->FromV8Value(v8_value, context_.get()));
+    if (value->IsType(base::Value::TYPE_STRING)
+        && value->GetAsString(&out)
+        && out == expected)
+      return testing::AssertionSuccess();
+    return testing::AssertionFailure();
+  }
+
+  v8::Isolate* isolate_;
+  v8::HandleScope handle_scope_;
+  ScopedPersistent<v8::Context> context_;
+  v8::Context::Scope context_scope_;
+  scoped_ptr<V8ValueConverter> converter_;
+  scoped_ptr<ActivityLogConverterStrategy> strategy_;
+};
+
+TEST_F(ActivityLogConverterStrategyTest, ConversionTest) {
+  const char* source = "(function() {"
+      "function foo() {}"
+      "return {"
+        "null: null,"
+        "true: true,"
+        "false: false,"
+        "positive_int: 42,"
+        "negative_int: -42,"
+        "zero: 0,"
+        "double: 88.8,"
+        "big_integral_double: 9007199254740992.0,"  // 2.0^53
+        "string: \"foobar\","
+        "empty_string: \"\","
+        "dictionary: {"
+          "foo: \"bar\","
+          "hot: \"dog\","
+        "},"
+        "empty_dictionary: {},"
+        "list: [ \"monkey\", \"balls\" ],"
+        "empty_list: [],"
+        "function: function() {},"
+        "named_function: foo"
+      "};"
+      "})();";
+
+  v8::Handle<v8::Script> script(v8::Script::New(v8::String::New(source)));
+  v8::Handle<v8::Object> v8_object = script->Run().As<v8::Object>();
+
+  EXPECT_TRUE(VerifyString(v8_object, "[Object]"));
+  EXPECT_TRUE(VerifyNull(v8_object->Get(v8::String::New("null"))));
+  EXPECT_TRUE(VerifyBoolean(v8_object->Get(v8::String::New("true")), true));
+  EXPECT_TRUE(VerifyBoolean(v8_object->Get(v8::String::New("false")), false));
+  EXPECT_TRUE(VerifyInteger(v8_object->Get(v8::String::New("positive_int")),
+                            42));
+  EXPECT_TRUE(VerifyInteger(v8_object->Get(v8::String::New("negative_int")),
+                            -42));
+  EXPECT_TRUE(VerifyInteger(v8_object->Get(v8::String::New("zero")), 0));
+  EXPECT_TRUE(VerifyDouble(v8_object->Get(v8::String::New("double")), 88.8));
+  EXPECT_TRUE(VerifyDouble(
+      v8_object->Get(v8::String::New("big_integral_double")),
+      9007199254740992.0));
+  EXPECT_TRUE(VerifyString(v8_object->Get(v8::String::New("string")),
+                           "foobar"));
+  EXPECT_TRUE(VerifyString(v8_object->Get(v8::String::New("empty_string")),
+                           ""));
+  EXPECT_TRUE(VerifyString(v8_object->Get(v8::String::New("dictionary")),
+                           "[Object]"));
+  EXPECT_TRUE(VerifyString(v8_object->Get(v8::String::New("empty_dictionary")),
+                           "[Object]"));
+  EXPECT_TRUE(VerifyString(v8_object->Get(v8::String::New("list")),
+                           "[Array]"));
+  EXPECT_TRUE(VerifyString(v8_object->Get(v8::String::New("empty_list")),
+                           "[Array]"));
+  EXPECT_TRUE(VerifyString(v8_object->Get(v8::String::New("function")),
+                           "[Function]"));
+  EXPECT_TRUE(VerifyString(v8_object->Get(v8::String::New("named_function")),
+                           "[Function foo()]"));
+}
+
+}  // namespace extensions
+
diff --git a/chrome/renderer/extensions/api_activity_logger.cc b/chrome/renderer/extensions/api_activity_logger.cc
index 28a4367..a6b95af 100644
--- a/chrome/renderer/extensions/api_activity_logger.cc
+++ b/chrome/renderer/extensions/api_activity_logger.cc
@@ -6,6 +6,7 @@
 #include "base/bind.h"
 #include "chrome/common/extensions/extension_messages.h"
 #include "chrome/renderer/chrome_render_process_observer.h"
+#include "chrome/renderer/extensions/activity_log_converter_strategy.h"
 #include "chrome/renderer/extensions/api_activity_logger.h"
 #include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/v8_value_converter.h"
@@ -56,6 +57,9 @@ void APIActivityLogger::LogInternal(
   v8::Local<v8::Array> arg_array = v8::Local<v8::Array>::Cast(args[2]);
   if (arg_array->Length() > 0) {
     scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
+    ActivityLogConverterStrategy strategy;
+    converter->SetFunctionAllowed(true);
+    converter->SetStrategy(&strategy);
     scoped_ptr<ListValue> arg_list(new ListValue());
     for (size_t i = 0; i < arg_array->Length(); ++i) {
       arg_list->Set(i,
diff --git a/chrome/renderer/extensions/dom_activity_logger.cc b/chrome/renderer/extensions/dom_activity_logger.cc
index 95d1c11..3c7a8cf 100644
--- a/chrome/renderer/extensions/dom_activity_logger.cc
+++ b/chrome/renderer/extensions/dom_activity_logger.cc
@@ -8,6 +8,7 @@
 #include "chrome/common/extensions/dom_action_types.h"
 #include "chrome/common/extensions/extension_messages.h"
 #include "chrome/renderer/chrome_render_process_observer.h"
+#include "chrome/renderer/extensions/activity_log_converter_strategy.h"
 #include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/v8_value_converter.h"
 #include "third_party/WebKit/public/platform/WebString.h"
@@ -30,6 +31,9 @@ void DOMActivityLogger::log(
     const v8::Handle<v8::Value> argv[],
     const WebString& call_type) {
   scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
+  ActivityLogConverterStrategy strategy;
+  converter->SetFunctionAllowed(true);
+  converter->SetStrategy(&strategy);
   scoped_ptr<ListValue> argv_list_value(new ListValue());
   for (int i =0; i < argc; i++) {
     argv_list_value->Set(
diff --git a/chrome/test/data/extensions/api_test/activity_log_private/test/test.js b/chrome/test/data/extensions/api_test/activity_log_private/test/test.js
index 0143cce..e1572a0 100644
--- a/chrome/test/data/extensions/api_test/activity_log_private/test/test.js
+++ b/chrome/test/data/extensions/api_test/activity_log_private/test/test.js
@@ -217,13 +217,8 @@ domExpectedActivity = [
      // Dom mutations
     'Document.createElement',
     'Document.createElement',
-    'Document.location',
     'Node.appendChild',
-    'Document.location',
-    'Document.location',
     'Node.insertBefore',
-    'Document.location',
-    'Document.location',
     'Node.replaceChild',
     //'Document.location',
     'HTMLDocument.write',
diff --git a/content/public/renderer/v8_value_converter.h b/content/public/renderer/v8_value_converter.h
index b71b32d..11514f7 100644
--- a/content/public/renderer/v8_value_converter.h
+++ b/content/public/renderer/v8_value_converter.h
@@ -24,6 +24,20 @@ namespace content {
 // ArrayBufferView subclasses (Uint8Array, etc.).
 class CONTENT_EXPORT V8ValueConverter {
  public:
+  // Extends the default behaviour of V8ValueConverter.
+  class CONTENT_EXPORT Strategy {
+   public:
+    virtual ~Strategy() {}
+    // If false is returned, V8ValueConverter proceeds with the default
+    // behavior.
+    virtual bool FromV8Object(v8::Handle<v8::Object> value,
+                              base::Value** out) const = 0;
+    // If false is returned, V8ValueConverter proceeds with the default
+    // behavior.
+    virtual bool FromV8Array(v8::Handle<v8::Array> value,
+                             base::Value** out) const = 0;
+  };
+
   static V8ValueConverter* create();
 
   virtual ~V8ValueConverter() {}
@@ -52,6 +66,9 @@ class CONTENT_EXPORT V8ValueConverter {
   // converting arguments to extension APIs.
   virtual void SetStripNullFromObjects(bool val) = 0;
 
+  // Extend default behavior of V8ValueConverter.
+  virtual void SetStrategy(Strategy* strategy) = 0;
+
   // Converts a base::Value to a v8::Value.
   //
   // Unsupported types are replaced with null.  If an array or object throws
diff --git a/content/renderer/v8_value_converter_impl.cc b/content/renderer/v8_value_converter_impl.cc
index cb4b9af..efd3d44 100644
--- a/content/renderer/v8_value_converter_impl.cc
+++ b/content/renderer/v8_value_converter_impl.cc
@@ -86,7 +86,8 @@ V8ValueConverterImpl::V8ValueConverterImpl()
       reg_exp_allowed_(false),
       function_allowed_(false),
       strip_null_from_objects_(false),
-      avoid_identity_hash_for_testing_(false) {}
+      avoid_identity_hash_for_testing_(false),
+      strategy_(NULL) {}
 
 void V8ValueConverterImpl::SetDateAllowed(bool val) {
   date_allowed_ = val;
@@ -104,6 +105,10 @@ void V8ValueConverterImpl::SetStripNullFromObjects(bool val) {
   strip_null_from_objects_ = val;
 }
 
+void V8ValueConverterImpl::SetStrategy(Strategy* strategy) {
+  strategy_ = strategy;
+}
+
 v8::Handle<v8::Value> V8ValueConverterImpl::ToV8Value(
     const base::Value* value, v8::Handle<v8::Context> context) const {
   v8::Context::Scope context_scope(context);
@@ -302,6 +307,12 @@ base::Value* V8ValueConverterImpl::FromV8Array(
       val->CreationContext() != v8::Context::GetCurrent())
     scope.reset(new v8::Context::Scope(val->CreationContext()));
 
+  if (strategy_) {
+    Value* out = NULL;
+    if (strategy_->FromV8Array(val, &out))
+      return out;
+  }
+
   base::ListValue* result = new base::ListValue();
 
   // Only fields with integer keys are carried over to the ListValue.
@@ -357,6 +368,7 @@ base::Value* V8ValueConverterImpl::FromV8Object(
     FromV8ValueState* state) const {
   if (!state->UpdateAndCheckUniqueness(val))
     return base::Value::CreateNullValue();
+
   scoped_ptr<v8::Context::Scope> scope;
   // If val was created in a different context than our current one, change to
   // that context, but change back after val is converted.
@@ -364,6 +376,12 @@ base::Value* V8ValueConverterImpl::FromV8Object(
       val->CreationContext() != v8::Context::GetCurrent())
     scope.reset(new v8::Context::Scope(val->CreationContext()));
 
+  if (strategy_) {
+    Value* out = NULL;
+    if (strategy_->FromV8Object(val, &out))
+      return out;
+  }
+
   scoped_ptr<base::DictionaryValue> result(new base::DictionaryValue());
   v8::Handle<v8::Array> property_names(val->GetOwnPropertyNames());
 
diff --git a/content/renderer/v8_value_converter_impl.h b/content/renderer/v8_value_converter_impl.h
index f5ad10a..2033164 100644
--- a/content/renderer/v8_value_converter_impl.h
+++ b/content/renderer/v8_value_converter_impl.h
@@ -30,6 +30,7 @@ class CONTENT_EXPORT V8ValueConverterImpl : public V8ValueConverter {
   virtual void SetRegExpAllowed(bool val) OVERRIDE;
   virtual void SetFunctionAllowed(bool val) OVERRIDE;
   virtual void SetStripNullFromObjects(bool val) OVERRIDE;
+  virtual void SetStrategy(Strategy* strategy) OVERRIDE;
   virtual v8::Handle<v8::Value> ToV8Value(
       const base::Value* value,
       v8::Handle<v8::Context> context) const OVERRIDE;
@@ -76,6 +77,9 @@ class CONTENT_EXPORT V8ValueConverterImpl : public V8ValueConverter {
 
   bool avoid_identity_hash_for_testing_;
 
+  // Strategy object that changes the converter's behavior.
+  Strategy* strategy_;
+
   DISALLOW_COPY_AND_ASSIGN(V8ValueConverterImpl);
 };