Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(258)

Side by Side Diff: content/child/v8_value_converter_impl.cc

Issue 1939233002: Properly detect cycles in V8ValueConverter, current impl is too strict. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: address comments from asargent@ Created 4 years, 7 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
OLDNEW
1 // Copyright (c) 2012 The Chromium Authors. All rights reserved. 1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
2 // Use of this source code is governed by a BSD-style license that can be 2 // Use of this source code is governed by a BSD-style license that can be
3 // found in the LICENSE file. 3 // found in the LICENSE file.
4 4
5 #include "content/child/v8_value_converter_impl.h" 5 #include "content/child/v8_value_converter_impl.h"
6 6
7 #include <stddef.h> 7 #include <stddef.h>
8 #include <stdint.h> 8 #include <stdint.h>
9 9
10 #include <cmath> 10 #include <cmath>
(...skipping 73 matching lines...) Expand 10 before | Expand all | Expand 10 after
84 : max_recursion_depth_(kMaxRecursionDepth), 84 : max_recursion_depth_(kMaxRecursionDepth),
85 avoid_identity_hash_for_testing_(avoid_identity_hash_for_testing) {} 85 avoid_identity_hash_for_testing_(avoid_identity_hash_for_testing) {}
86 86
87 // If |handle| is not in |unique_map_|, then add it to |unique_map_| and 87 // If |handle| is not in |unique_map_|, then add it to |unique_map_| and
88 // return true. 88 // return true.
89 // 89 //
90 // Otherwise do nothing and return false. Here "A is unique" means that no 90 // Otherwise do nothing and return false. Here "A is unique" means that no
91 // other handle B in the map points to the same object as A. Note that A can 91 // other handle B in the map points to the same object as A. Note that A can
92 // be unique even if there already is another handle with the same identity 92 // be unique even if there already is another handle with the same identity
93 // hash (key) in the map, because two objects can have the same hash. 93 // hash (key) in the map, because two objects can have the same hash.
94 bool UpdateAndCheckUniqueness(v8::Local<v8::Object> handle) { 94 bool AddToUniquenessCheck(v8::Local<v8::Object> handle) {
95 typedef HashToHandleMap::const_iterator Iterator; 95 int hash;
96 int hash = avoid_identity_hash_for_testing_ ? 0 : handle->GetIdentityHash(); 96 Iterator iter = GetIteratorInMap(handle, &hash);
97 // We only compare using == with handles to objects with the same identity 97 if (iter != unique_map_.end())
98 // hash. Different hash obviously means different objects, but two objects 98 return false;
99 // in a couple of thousands could have the same identity hash. 99
100 std::pair<Iterator, Iterator> range = unique_map_.equal_range(hash);
101 for (Iterator it = range.first; it != range.second; ++it) {
102 // Operator == for handles actually compares the underlying objects.
103 if (it->second == handle)
104 return false;
105 }
106 unique_map_.insert(std::make_pair(hash, handle)); 100 unique_map_.insert(std::make_pair(hash, handle));
107 return true; 101 return true;
108 } 102 }
109 103
104 bool RemoveFromUniquenessCheck(v8::Local<v8::Object> handle) {
105 int unused_hash;
106 Iterator iter = GetIteratorInMap(handle, &unused_hash);
107 if (iter == unique_map_.end())
108 return false;
109 unique_map_.erase(iter);
110 return true;
111 }
112
110 bool HasReachedMaxRecursionDepth() { 113 bool HasReachedMaxRecursionDepth() {
111 return max_recursion_depth_ < 0; 114 return max_recursion_depth_ < 0;
112 } 115 }
113 116
114 private: 117 private:
115 typedef std::multimap<int, v8::Local<v8::Object> > HashToHandleMap; 118 using HashToHandleMap = std::multimap<int, v8::Local<v8::Object>>;
119 using Iterator = HashToHandleMap::const_iterator;
120
121 Iterator GetIteratorInMap(v8::Local<v8::Object> handle, int* hash) {
122 *hash = avoid_identity_hash_for_testing_ ? 0 : handle->GetIdentityHash();
123 // We only compare using == with handles to objects with the same identity
124 // hash. Different hash obviously means different objects, but two objects
125 // in a couple of thousands could have the same identity hash.
126 std::pair<Iterator, Iterator> range = unique_map_.equal_range(*hash);
127 for (Iterator it = range.first; it != range.second; ++it) {
128 // Operator == for handles actually compares the underlying objects.
129 if (it->second == handle)
130 return it;
131 }
132 // Not found.
133 return unique_map_.end();
134 }
135
116 HashToHandleMap unique_map_; 136 HashToHandleMap unique_map_;
117 137
118 int max_recursion_depth_; 138 int max_recursion_depth_;
119 139
120 bool avoid_identity_hash_for_testing_; 140 bool avoid_identity_hash_for_testing_;
141
142 DISALLOW_COPY_AND_ASSIGN(FromV8ValueState);
143 };
144
145 // A class to ensure that objects/arrays that are being converted by
146 // this V8ValueConverterImpl do not have cycles.
147 //
148 // An example of cycle: var v = {}; v = {key: v};
149 // Not an example of cycle: var v = {}; a = [v, v]; or w = {a: v, b: v};
150 class V8ValueConverterImpl::ScopedUniquenessGuard {
151 public:
152 ScopedUniquenessGuard(V8ValueConverterImpl::FromV8ValueState* state,
153 v8::Local<v8::Object> value)
154 : state_(state),
155 value_(value),
156 is_valid_(state_->AddToUniquenessCheck(value_)) {}
157 ~ScopedUniquenessGuard() {
158 if (is_valid_) {
159 bool removed = state_->RemoveFromUniquenessCheck(value_);
160 DCHECK(removed);
161 }
162 }
163
164 bool is_valid() const { return is_valid_; }
165
166 private:
167 typedef std::multimap<int, v8::Local<v8::Object> > HashToHandleMap;
168 V8ValueConverterImpl::FromV8ValueState* state_;
169 v8::Local<v8::Object> value_;
170 bool is_valid_;
171
172 DISALLOW_COPY_AND_ASSIGN(ScopedUniquenessGuard);
121 }; 173 };
122 174
123 V8ValueConverter* V8ValueConverter::create() { 175 V8ValueConverter* V8ValueConverter::create() {
124 return new V8ValueConverterImpl(); 176 return new V8ValueConverterImpl();
125 } 177 }
126 178
127 V8ValueConverterImpl::V8ValueConverterImpl() 179 V8ValueConverterImpl::V8ValueConverterImpl()
128 : date_allowed_(false), 180 : date_allowed_(false),
129 reg_exp_allowed_(false), 181 reg_exp_allowed_(false),
130 function_allowed_(false), 182 function_allowed_(false),
(...skipping 235 matching lines...) Expand 10 before | Expand all | Expand 10 after
366 return FromV8Object(val.As<v8::Object>(), state, isolate); 418 return FromV8Object(val.As<v8::Object>(), state, isolate);
367 419
368 LOG(ERROR) << "Unexpected v8 value type encountered."; 420 LOG(ERROR) << "Unexpected v8 value type encountered.";
369 return NULL; 421 return NULL;
370 } 422 }
371 423
372 base::Value* V8ValueConverterImpl::FromV8Array( 424 base::Value* V8ValueConverterImpl::FromV8Array(
373 v8::Local<v8::Array> val, 425 v8::Local<v8::Array> val,
374 FromV8ValueState* state, 426 FromV8ValueState* state,
375 v8::Isolate* isolate) const { 427 v8::Isolate* isolate) const {
376 if (!state->UpdateAndCheckUniqueness(val)) 428 ScopedUniquenessGuard uniqueness_guard(state, val);
429 if (!uniqueness_guard.is_valid())
377 return base::Value::CreateNullValue().release(); 430 return base::Value::CreateNullValue().release();
378 431
379 std::unique_ptr<v8::Context::Scope> scope; 432 std::unique_ptr<v8::Context::Scope> scope;
380 // If val was created in a different context than our current one, change to 433 // If val was created in a different context than our current one, change to
381 // that context, but change back after val is converted. 434 // that context, but change back after val is converted.
382 if (!val->CreationContext().IsEmpty() && 435 if (!val->CreationContext().IsEmpty() &&
383 val->CreationContext() != isolate->GetCurrentContext()) 436 val->CreationContext() != isolate->GetCurrentContext())
384 scope.reset(new v8::Context::Scope(val->CreationContext())); 437 scope.reset(new v8::Context::Scope(val->CreationContext()));
385 438
386 if (strategy_) { 439 if (strategy_) {
(...skipping 64 matching lines...) Expand 10 before | Expand all | Expand 10 after
451 if (data) 504 if (data)
452 return base::BinaryValue::CreateWithCopiedBuffer(data, length); 505 return base::BinaryValue::CreateWithCopiedBuffer(data, length);
453 else 506 else
454 return NULL; 507 return NULL;
455 } 508 }
456 509
457 base::Value* V8ValueConverterImpl::FromV8Object( 510 base::Value* V8ValueConverterImpl::FromV8Object(
458 v8::Local<v8::Object> val, 511 v8::Local<v8::Object> val,
459 FromV8ValueState* state, 512 FromV8ValueState* state,
460 v8::Isolate* isolate) const { 513 v8::Isolate* isolate) const {
461 if (!state->UpdateAndCheckUniqueness(val)) 514 ScopedUniquenessGuard uniqueness_guard(state, val);
515 if (!uniqueness_guard.is_valid())
462 return base::Value::CreateNullValue().release(); 516 return base::Value::CreateNullValue().release();
463 517
464 std::unique_ptr<v8::Context::Scope> scope; 518 std::unique_ptr<v8::Context::Scope> scope;
465 // If val was created in a different context than our current one, change to 519 // If val was created in a different context than our current one, change to
466 // that context, but change back after val is converted. 520 // that context, but change back after val is converted.
467 if (!val->CreationContext().IsEmpty() && 521 if (!val->CreationContext().IsEmpty() &&
468 val->CreationContext() != isolate->GetCurrentContext()) 522 val->CreationContext() != isolate->GetCurrentContext())
469 scope.reset(new v8::Context::Scope(val->CreationContext())); 523 scope.reset(new v8::Context::Scope(val->CreationContext()));
470 524
471 if (strategy_) { 525 if (strategy_) {
(...skipping 80 matching lines...) Expand 10 before | Expand all | Expand 10 after
552 continue; 606 continue;
553 607
554 result->SetWithoutPathExpansion(std::string(*name_utf8, name_utf8.length()), 608 result->SetWithoutPathExpansion(std::string(*name_utf8, name_utf8.length()),
555 child.release()); 609 child.release());
556 } 610 }
557 611
558 return result.release(); 612 return result.release();
559 } 613 }
560 614
561 } // namespace content 615 } // namespace content
OLDNEW
« no previous file with comments | « content/child/v8_value_converter_impl.h ('k') | content/child/v8_value_converter_impl_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698