diff options
author | pmarch@chromium.org <pmarch@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-08-12 17:51:36 +0000 |
---|---|---|
committer | pmarch@chromium.org <pmarch@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-08-12 17:51:36 +0000 |
commit | d754cbb0f9c177e9e881e2dc2e3b721c4a1b7048 (patch) | |
tree | c84a4b1fe2893ef510fd0825993c248d0f8124de | |
parent | 581de1f37dd528c558ad5cca2fae0d95cd7c07fe (diff) | |
download | chromium_src-d754cbb0f9c177e9e881e2dc2e3b721c4a1b7048.zip chromium_src-d754cbb0f9c177e9e881e2dc2e3b721c4a1b7048.tar.gz chromium_src-d754cbb0f9c177e9e881e2dc2e3b721c4a1b7048.tar.bz2 |
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
-rw-r--r-- | chrome/chrome_renderer.gypi | 2 | ||||
-rw-r--r-- | chrome/chrome_tests_unit.gypi | 1 | ||||
-rw-r--r-- | chrome/renderer/extensions/activity_log_converter_strategy.cc | 54 | ||||
-rw-r--r-- | chrome/renderer/extensions/activity_log_converter_strategy.h | 36 | ||||
-rw-r--r-- | chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc | 158 | ||||
-rw-r--r-- | chrome/renderer/extensions/api_activity_logger.cc | 4 | ||||
-rw-r--r-- | chrome/renderer/extensions/dom_activity_logger.cc | 4 | ||||
-rw-r--r-- | chrome/test/data/extensions/api_test/activity_log_private/test/test.js | 5 | ||||
-rw-r--r-- | content/public/renderer/v8_value_converter.h | 17 | ||||
-rw-r--r-- | content/renderer/v8_value_converter_impl.cc | 20 | ||||
-rw-r--r-- | content/renderer/v8_value_converter_impl.h | 4 |
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); }; |