Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Side by Side Diff: src/JsEngine.cpp

Issue 29371607: Issue #3593 - Make isolate a fully internal member of the engine
Patch Set: improve unit tests to go with isolate change Created Jan. 16, 2017, 3:53 p.m.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View unified diff | Download patch
« no previous file with comments | « include/AdblockPlus/JsEngine.h ('k') | src/JsEngineInternal.h » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
1 /* 1 /*
2 * This file is part of Adblock Plus <https://adblockplus.org/>, 2 * This file is part of Adblock Plus <https://adblockplus.org/>,
3 * Copyright (C) 2006-2016 Eyeo GmbH 3 * Copyright (C) 2006-2016 Eyeo GmbH
4 * 4 *
5 * Adblock Plus is free software: you can redistribute it and/or modify 5 * Adblock Plus is free software: you can redistribute it and/or modify
6 * it under the terms of the GNU General Public License version 3 as 6 * it under the terms of the GNU General Public License version 3 as
7 * published by the Free Software Foundation. 7 * published by the Free Software Foundation.
8 * 8 *
9 * Adblock Plus is distributed in the hope that it will be useful, 9 * Adblock Plus is distributed in the hope that it will be useful,
10 * but WITHOUT ANY WARRANTY; without even the implied warranty of 10 * but WITHOUT ANY WARRANTY; without even the implied warranty of
(...skipping 60 matching lines...) Expand 10 before | Expand all | Expand 10 after
71 }; 71 };
72 } 72 }
73 73
74 V8ExecutionScope::V8ExecutionScope(JsEngineInternal* engine) 74 V8ExecutionScope::V8ExecutionScope(JsEngineInternal* engine)
75 : lock(engine->GetIsolate()), 75 : lock(engine->GetIsolate()),
76 isolateScope(engine->GetIsolate()), 76 isolateScope(engine->GetIsolate()),
77 handleScope(engine->GetIsolate()), 77 handleScope(engine->GetIsolate()),
78 contextScope(engine->GetContextAsLocal()) 78 contextScope(engine->GetContextAsLocal())
79 {} 79 {}
80 80
81 AdblockPlus::ScopedV8Isolate::ScopedV8Isolate() 81 AdblockPlus::JsEngine::JsEngine(v8::Isolate* isolate)
82 {
83 V8Initializer::Init();
84 isolate = v8::Isolate::New();
85 }
86
87 AdblockPlus::ScopedV8Isolate::~ScopedV8Isolate()
88 {
89 isolate->Dispose();
90 isolate = nullptr;
91 }
92
93 AdblockPlus::JsEngine::JsEngine(const ScopedV8IsolatePtr& isolate)
94 : isolate(isolate) 82 : isolate(isolate)
95 {} 83 {}
96 84
97 JsEngineInternal::JsEngineInternal(const AdblockPlus::ScopedV8IsolatePtr& isolat e) 85 AdblockPlus::JsEngine::~JsEngine()
86 {
87 isolate->Dispose();
88 }
89
90 JsEngineInternal::JsEngineInternal(v8::Isolate* isolate)
98 : AdblockPlus::JsEngine(isolate), 91 : AdblockPlus::JsEngine(isolate),
99 context(isolate->Get(),v8::Context::New(isolate->Get())) 92 context(isolate,v8::Context::New(isolate))
100 { 93 {
101 /* 94 /*
102 * Enter v8 scope for our context so that we can initialize its global object. 95 * Enter v8 scope for our context so that we can initialize its global object.
103 */ 96 */
104 const v8::Context::Scope contextScope(GetContextAsLocal()); 97 const v8::Context::Scope contextScope(GetContextAsLocal());
105 auto globalObject = GetGlobalObject(); 98 auto globalObject = GetGlobalObject();
106 // Timeout 99 // Timeout
107 globalObject->Set(ToV8String("setTimeout"), MakeCallback(::CallbackForSetTimeo ut)); 100 globalObject->Set(ToV8String("setTimeout"), MakeCallback(::CallbackForSetTimeo ut));
108 // Web request 101 // Web request
109 auto auxiliaryObject = v8::Object::New(); 102 auto auxiliaryObject = v8::Object::New();
(...skipping 13 matching lines...) Expand all
123 } 116 }
124 117
125 /** 118 /**
126 * \par Design Notes 119 * \par Design Notes
127 * It is technically necessary to construct JsEngine instances *only* within a f actory. 120 * It is technically necessary to construct JsEngine instances *only* within a f actory.
128 * Initialization requires that certain transient v8 scopes be set up 121 * Initialization requires that certain transient v8 scopes be set up
129 * before initialization and torn down afterwards. 122 * before initialization and torn down afterwards.
130 * C++ has no syntax to use anything like a sentry object in the constructor its elf. 123 * C++ has no syntax to use anything like a sentry object in the constructor its elf.
131 * Thus we need to establish v8 scope within every C++ scope that constructs an object. 124 * Thus we need to establish v8 scope within every C++ scope that constructs an object.
132 */ 125 */
133 AdblockPlus::JsEnginePtr AdblockPlus::JsEngine::New(const AppInfo& appInfo, cons t ScopedV8IsolatePtr& isolate) 126 AdblockPlus::JsEnginePtr AdblockPlus::JsEngine::New(const AppInfo& appInfo)
134 { 127 {
135 auto isolateP = isolate->Get(); 128 /* Global initialization for v8 required before first construction of isolate
129 */
130 V8Initializer::Init();
131
136 /* 132 /*
137 * TODO: Remove `locker`. 133 * The constructors require v8 scopes in order to work, and we only need
138 * Until #3595 is fixed, unit tests allocate isolates outside of this class. 134 * these scopes for the duration of construction, not the entire duration
139 * Once we're no longer doing that, we may assume that this class holds 135 * of the engine. We create the isolate outside the constructors so that
140 * exclusive access to the nascent isolate. 136 * these v8 scopes end when the factory returns.
141 * The factory and constructor constitute a single-threaded usage,
142 * and there will be no need to lock the isolate.
143 */ 137 */
144 const v8::Locker locker(isolateP); 138 auto isolate(v8::Isolate::New());
139
145 /* 140 /*
146 * Set up v8 scopes for isolate and handle. 141 * Set up v8 scopes for isolate and handle.
147 * We cannot set up the v8 scope for context because it doesn't exist 142 * We cannot set up the v8 scope for context because it doesn't exist
148 * until after the constructor for `JsEngineInternal` returns. 143 * until after the constructor for `JsEngineInternal` returns.
149 */ 144 */
150 const v8::Isolate::Scope isolateScope(isolateP); 145 const v8::Locker locker(isolate);
151 const v8::HandleScope handleScope(isolateP); 146 const v8::Isolate::Scope isolateScope(isolate);
147 const v8::HandleScope handleScope(isolate);
152 148
153 std::shared_ptr<JsEngineInternal> engine(std::make_shared<JsEngineInternal>(is olate)); 149 std::shared_ptr<JsEngineInternal> engine(std::make_shared<JsEngineInternal>(is olate));
154 150
155 JsEnginePtr result(engine); 151 JsEnginePtr result(engine);
156 // Establish a context scope for the legacy setup of the global object 152 // Establish a context scope for the legacy setup of the global object
157 const v8::Context::Scope contextScope(engine->GetContextAsLocal()); 153 const v8::Context::Scope contextScope(engine->GetContextAsLocal());
158 AdblockPlus::GlobalJsObject::Setup(result, appInfo, result->GetGlobalObject()) ; 154 AdblockPlus::GlobalJsObject::Setup(result, appInfo, result->GetGlobalObject()) ;
159 return result; 155 return result;
160 } 156 }
161 157
162 v8::Local<v8::Context> JsEngineInternal::GetContextAsLocal() const 158 v8::Local<v8::Context> JsEngineInternal::GetContextAsLocal() const
163 { 159 {
164 return v8::Local<v8::Context>::New(isolate->Get(), context); 160 return v8::Local<v8::Context>::New(isolate, context);
165 } 161 }
166 162
167 AdblockPlus::JsValuePtr AdblockPlus::JsEngine::GetGlobalObject() 163 AdblockPlus::JsValuePtr AdblockPlus::JsEngine::GetGlobalObject()
168 { 164 {
169 return JsValuePtr(new JsValue(shared_from_this(), ToInternal(this)->GetGlobalO bject())); 165 return JsValuePtr(new JsValue(shared_from_this(), ToInternal(this)->GetGlobalO bject()));
170 } 166 }
171 167
172 v8::Local<v8::Object> JsEngineInternal::GetGlobalObject() 168 v8::Local<v8::Object> JsEngineInternal::GetGlobalObject()
173 { 169 {
174 return GetContextAsLocal()->Global(); 170 return GetContextAsLocal()->Global();
(...skipping 215 matching lines...) Expand 10 before | Expand all | Expand 10 after
390 386
391 JsEngineInternal* ToInternal(AdblockPlus::JsEnginePtr p) 387 JsEngineInternal* ToInternal(AdblockPlus::JsEnginePtr p)
392 { 388 {
393 return static_cast<JsEngineInternal*>(p.get()); 389 return static_cast<JsEngineInternal*>(p.get());
394 } 390 }
395 391
396 JsEngineInternal* ToInternal(AdblockPlus::JsEngine* p) 392 JsEngineInternal* ToInternal(AdblockPlus::JsEngine* p)
397 { 393 {
398 return static_cast<JsEngineInternal*>(p); 394 return static_cast<JsEngineInternal*>(p);
399 } 395 }
OLDNEW
« no previous file with comments | « include/AdblockPlus/JsEngine.h ('k') | src/JsEngineInternal.h » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld