Replace Context::Scope with ScriptState::Scope

Replace Context::Scope with ScriptState::Scope

It's easy to misuse context. We should pass ScriptState around instead of contexts.

BUG=357144
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175344

[haraken, 2014-05-29 09:42:42]

PTAL

[haraken, 2014-05-30 12:45:57]

Dan or AdamK: Would you take a look at this?

[adamk, 2014-05-30 19:59:21]

I guess this is fine, but I'm not clear on the purpose of these changes: you
still use V8 API, but now need special Blink knowledge to know that a
WebCore::ScriptState gives you both a HandleScope and a Context::Scope. As
someone who's comfortable with the V8 API, I guess I'm a bit weird, though.

https://codereview.chromium.org/306853002/diff/1/Source/bindings/v8/ScriptCon...
File Source/bindings/v8/ScriptController.cpp (right):

https://codereview.chromium.org/306853002/diff/1/Source/bindings/v8/ScriptCon...
Source/bindings/v8/ScriptController.cpp:430: v8::HandleScope
handleScope(m_isolate);
Is this HandleScope still needed?

https://codereview.chromium.org/306853002/diff/1/Source/bindings/v8/ScriptCon...
Source/bindings/v8/ScriptController.cpp:593: v8::HandleScope
handleScope(m_isolate);
Is this HandleScope still needed?

https://codereview.chromium.org/306853002/diff/1/Source/bindings/v8/ScriptDeb...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/306853002/diff/1/Source/bindings/v8/ScriptDeb...
Source/bindings/v8/ScriptDebugServer.cpp:341: v8::Handle<v8::Context>
pausedContext = m_pausedContext.IsEmpty() ? m_isolate->GetCurrentContext() :
m_pausedContext;
Should m_pausedContext be a ScriptState?

[haraken, 2014-06-02 01:14:49]

Thanks for review! Updated the CL, PTAL.

> I guess this is fine, but I'm not clear on the purpose of these changes: you
> still use V8 API, but now need special Blink knowledge to know that a
> WebCore::ScriptState gives you both a HandleScope and a Context::Scope. As
> someone who's comfortable with the V8 API, I guess I'm a bit weird, though.

I understand your concern. The purpose of this change is just that we don't want
to have two ways to enter a context scope; currently we sometimes use
ScriptState::Scope and sometimes use Context::Scope (In particular, it's weird
to enter the Context::Scope after getting the context by
toV8Context(executionContext, frame) even though we already have a ScriptState
on that object). Given that we're passing ScriptState to almost all places, I
guess it's better to use ScriptState::Scope as much as possible.

https://codereview.chromium.org/306853002/diff/1/Source/bindings/v8/ScriptCon...
File Source/bindings/v8/ScriptController.cpp (right):

https://codereview.chromium.org/306853002/diff/1/Source/bindings/v8/ScriptCon...
Source/bindings/v8/ScriptController.cpp:430: v8::HandleScope
handleScope(m_isolate);
On 2014/05/30 19:59:22, adamk wrote:
> Is this HandleScope still needed?

Removed.

https://codereview.chromium.org/306853002/diff/1/Source/bindings/v8/ScriptCon...
Source/bindings/v8/ScriptController.cpp:593: v8::HandleScope
handleScope(m_isolate);
On 2014/05/30 19:59:22, adamk wrote:
> Is this HandleScope still needed?

Removed.

https://codereview.chromium.org/306853002/diff/1/Source/bindings/v8/ScriptDeb...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/306853002/diff/1/Source/bindings/v8/ScriptDeb...
Source/bindings/v8/ScriptDebugServer.cpp:341: v8::Handle<v8::Context>
pausedContext = m_pausedContext.IsEmpty() ? m_isolate->GetCurrentContext() :
m_pausedContext;
On 2014/05/30 19:59:22, adamk wrote:
> Should m_pausedContext be a ScriptState?

Done.

[dcarney, 2014-06-02 07:01:33]

lgtm

[haraken, 2014-06-02 07:02:09]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-02 07:02:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/306853002/40001

[commit-bot: I haz the power, 2014-06-02 07:02:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-02 07:02:50]

Failed to apply patch for Source/web/InspectorFrontendClientImpl.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/web/InspectorFrontendClientImpl.cpp
  Hunk #1 FAILED at 62.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/web/InspectorFrontendClientImpl.cpp.rej

Patch:       Source/web/InspectorFrontendClientImpl.cpp
Index: Source/web/InspectorFrontendClientImpl.cpp
diff --git a/Source/web/InspectorFrontendClientImpl.cpp
b/Source/web/InspectorFrontendClientImpl.cpp
index
daf28786c6bc1f392d854f738fc234fa74ba0e11..6203bb51ef105e9f86a2571ed8ee62537b91c17e
100644
--- a/Source/web/InspectorFrontendClientImpl.cpp
+++ b/Source/web/InspectorFrontendClientImpl.cpp
@@ -62,15 +62,15 @@ InspectorFrontendClientImpl::~InspectorFrontendClientImpl()
 void InspectorFrontendClientImpl::windowObjectCleared()
 {
     v8::Isolate* isolate = v8::Isolate::GetCurrent();
-    v8::HandleScope handleScope(isolate);
-    v8::Handle<v8::Context> frameContext = m_frontendPage->mainFrame() ?
toV8Context(isolate, m_frontendPage->mainFrame(), DOMWrapperWorld::mainWorld())
: v8::Local<v8::Context>();
-    v8::Context::Scope contextScope(frameContext);
+    ASSERT(m_frontendPage->mainFrame());
+    ScriptState* scriptState =
ScriptState::forMainWorld(m_frontendPage->mainFrame());
+    ScriptState::Scope scope(scriptState);
 
     if (m_frontendHost)
         m_frontendHost->disconnectClient();
     m_frontendHost = InspectorFrontendHost::create(this, m_frontendPage);
-    v8::Handle<v8::Value> frontendHostObj = toV8(m_frontendHost.get(),
v8::Handle<v8::Object>(), frameContext->GetIsolate());
-    v8::Handle<v8::Object> global = frameContext->Global();
+    v8::Handle<v8::Object> global = scriptState->context()->Global();
+    v8::Handle<v8::Value> frontendHostObj = toV8(m_frontendHost.get(), global,
scriptState->isolate());
 
     global->Set(v8::String::NewFromUtf8(isolate, "InspectorFrontendHost"),
frontendHostObj);
     ScriptController* scriptController = m_frontendPage->mainFrame() ?
&m_frontendPage->mainFrame()->script() : 0;

[haraken, 2014-06-02 07:05:43]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-02 07:05:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/306853002/60001

[commit-bot: I haz the power, 2014-06-02 08:12:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6644)

[commit-bot: I haz the power, 2014-06-02 08:13:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-02 08:13:34]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6650)

[haraken, 2014-06-02 08:16:03]

jochen@: rubberstamp to web/ please?

[haraken, 2014-06-02 16:04:00]

adamk@: Would you take a look at this when you have a cycle?

[adamk, 2014-06-02 16:16:47]

lgtm

[haraken, 2014-06-02 16:16:58]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-02 16:17:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/306853002/60001

[commit-bot: I haz the power, 2014-06-02 16:17:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-02 16:17:25]

Failed to apply patch for Source/bindings/v8/NPV8Object.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/v8/NPV8Object.cpp
  Hunk #1 FAILED at 77.
  Hunk #2 FAILED at 230.
  Hunk #3 FAILED at 286.
  Hunk #4 FAILED at 335.
  Hunk #6 FAILED at 371.
  Hunk #7 FAILED at 405.
  Hunk #8 FAILED at 434.
  Hunk #9 FAILED at 454.
  Hunk #10 FAILED at 477.
  Hunk #11 FAILED at 504.
  Hunk #12 FAILED at 522.
  Hunk #15 FAILED at 578.
  12 out of 15 hunks FAILED -- saving rejects to file
Source/bindings/v8/NPV8Object.cpp.rej

Patch:       Source/bindings/v8/NPV8Object.cpp
Index: Source/bindings/v8/NPV8Object.cpp
diff --git a/Source/bindings/v8/NPV8Object.cpp
b/Source/bindings/v8/NPV8Object.cpp
index
1c432fe8839e4be934a40e8786b1d78d94b2ab83..f17980a9fc9cb8ac4e9d4dcf6b9bd0245c126818
100644
--- a/Source/bindings/v8/NPV8Object.cpp
+++ b/Source/bindings/v8/NPV8Object.cpp
@@ -77,14 +77,16 @@ static NPClass V8NPObjectClass = {
     0, 0, 0, 0, 0, 0, 0, 0, 0, 0
 };
 
-static v8::Local<v8::Context> mainWorldContext(v8::Isolate* isolate, NPP npp,
NPObject* npObject)
+static ScriptState* mainWorldScriptState(v8::Isolate* isolate, NPP npp,
NPObject* npObject)
 {
     ASSERT(npObject->_class == &V8NPObjectClass);
     V8NPObject* object = reinterpret_cast<V8NPObject*>(npObject);
     DOMWindow* window = object->rootObject;
     if (!window || !window->isCurrentlyDisplayedInFrame())
-        return v8::Local<v8::Context>();
-    return toV8Context(isolate, object->rootObject->frame(),
DOMWrapperWorld::mainWorld());
+        return 0;
+    v8::HandleScope handleScope(isolate);
+    v8::Handle<v8::Context> context = toV8Context(isolate,
object->rootObject->frame(), DOMWrapperWorld::mainWorld());
+    return ScriptState::from(context);
 }
 
 static PassOwnPtr<v8::Handle<v8::Value>[]> createValueListFromVariantArgs(const
NPVariant* arguments, uint32_t argumentCount, NPObject* owner, v8::Isolate*
isolate)
@@ -230,13 +232,12 @@ bool _NPN_Invoke(NPP npp, NPObject* npObject, NPIdentifier
methodName, const NPV
         return _NPN_Evaluate(npp, npObject,
const_cast<NPString*>(&arguments[0].value.stringValue), result);
     }
 
-    v8::HandleScope handleScope(isolate);
     // FIXME: should use the plugin's owner frame as the security context.
-    v8::Handle<v8::Context> context = mainWorldContext(isolate, npp, npObject);
-    if (context.IsEmpty())
+    ScriptState* scriptState = mainWorldScriptState(isolate, npp, npObject);
+    if (!scriptState)
         return false;
 
-    v8::Context::Scope scope(context);
+    ScriptState::Scope scope(scriptState);
     ExceptionCatcher exceptionCatcher;
 
     v8::Handle<v8::Object> v8Object = v8::Local<v8::Object>::New(isolate,
v8NpObject->v8Object);
@@ -286,12 +287,11 @@ bool _NPN_InvokeDefault(NPP npp, NPObject* npObject, const
NPVariant* arguments,
 
     VOID_TO_NPVARIANT(*result);
 
-    v8::HandleScope handleScope(isolate);
-    v8::Handle<v8::Context> context = mainWorldContext(isolate, npp, npObject);
-    if (context.IsEmpty())
+    ScriptState* scriptState = mainWorldScriptState(isolate, npp, npObject);
+    if (!scriptState)
         return false;
 
-    v8::Context::Scope scope(context);
+    ScriptState::Scope scope(scriptState);
     ExceptionCatcher exceptionCatcher;
 
     // Lookup the function object and call it.
@@ -335,12 +335,11 @@ bool _NPN_EvaluateHelper(NPP npp, bool popupsAllowed,
NPObject* npObject, NPStri
         return false;
 
     v8::Isolate* isolate = v8::Isolate::GetCurrent();
-    v8::HandleScope handleScope(isolate);
-    v8::Handle<v8::Context> context = mainWorldContext(isolate, npp, npObject);
-    if (context.IsEmpty())
+    ScriptState* scriptState = mainWorldScriptState(isolate, npp, npObject);
+    if (!scriptState)
         return false;
 
-    v8::Context::Scope scope(context);
+    ScriptState::Scope scope(scriptState);
     ExceptionCatcher exceptionCatcher;
 
     // FIXME: Is this branch still needed after switching to using
UserGestureIndicator?
@@ -354,7 +353,7 @@ bool _NPN_EvaluateHelper(NPP npp, bool popupsAllowed,
NPObject* npObject, NPStri
     String script = String::fromUTF8(npScript->UTF8Characters,
npScript->UTF8Length);
 
     UserGestureIndicator gestureIndicator(popupsAllowed ?
DefinitelyProcessingNewUserGesture : PossiblyProcessingUserGesture);
-    v8::Local<v8::Value> v8result =
frame->script().executeScriptAndReturnValue(context, ScriptSourceCode(script,
KURL(ParsedURLString, filename)));
+    v8::Local<v8::Value> v8result =
frame->script().executeScriptAndReturnValue(scriptState->context(),
ScriptSourceCode(script, KURL(ParsedURLString, filename)));
 
     if (v8result.IsEmpty())
         return false;
@@ -371,12 +370,11 @@ bool _NPN_GetProperty(NPP npp, NPObject* npObject,
NPIdentifier propertyName, NP
 
     if (V8NPObject* object = npObjectToV8NPObject(npObject)) {
         v8::Isolate* isolate = v8::Isolate::GetCurrent();
-        v8::HandleScope handleScope(isolate);
-        v8::Handle<v8::Context> context = mainWorldContext(isolate, npp,
npObject);
-        if (context.IsEmpty())
+        ScriptState* scriptState = mainWorldScriptState(isolate, npp,
npObject);
+        if (!scriptState)
             return false;
 
-        v8::Context::Scope scope(context);
+        ScriptState::Scope scope(scriptState);
         ExceptionCatcher exceptionCatcher;
 
         v8::Handle<v8::Object> obj = v8::Local<v8::Object>::New(isolate,
object->v8Object);
@@ -405,16 +403,15 @@ bool _NPN_SetProperty(NPP npp, NPObject* npObject,
NPIdentifier propertyName, co
 
     if (V8NPObject* object = npObjectToV8NPObject(npObject)) {
         v8::Isolate* isolate = v8::Isolate::GetCurrent();
-        v8::HandleScope handleScope(isolate);
-        v8::Handle<v8::Context> context = mainWorldContext(isolate, npp,
npObject);
-        if (context.IsEmpty())
+        ScriptState* scriptState = mainWorldScriptState(isolate, npp,
npObject);
+        if (!scriptState)
             return false;
 
-        v8::Context::Scope scope(context);
+        ScriptState::Scope scope(scriptState);
         ExceptionCatcher exceptionCatcher;
 
         v8::Handle<v8::Object> obj = v8::Local<v8::Object>::New(isolate,
object->v8Object);
-        obj->Set(npIdentifierToV8Identifier(propertyName,
context->GetIsolate()), convertNPVariantToV8Object(value,
object->rootObject->frame()->script().windowScriptNPObject(),
context->GetIsolate()));
+        obj->Set(npIdentifierToV8Identifier(propertyName, isolate),
convertNPVariantToV8Object(value,
object->rootObject->frame()->script().windowScriptNPObject(), isolate));
         return true;
     }
 
@@ -434,11 +431,10 @@ bool _NPN_RemoveProperty(NPP npp, NPObject* npObject,
NPIdentifier propertyName)
         return false;
 
     v8::Isolate* isolate = v8::Isolate::GetCurrent();
-    v8::HandleScope handleScope(isolate);
-    v8::Handle<v8::Context> context = mainWorldContext(isolate, npp, npObject);
-    if (context.IsEmpty())
+    ScriptState* scriptState = mainWorldScriptState(isolate, npp, npObject);
+    if (!scriptState)
         return false;
-    v8::Context::Scope scope(context);
+    ScriptState::Scope scope(scriptState);
     ExceptionCatcher exceptionCatcher;
 
     v8::Handle<v8::Object> obj = v8::Local<v8::Object>::New(isolate,
object->v8Object);
@@ -454,11 +450,10 @@ bool _NPN_HasProperty(NPP npp, NPObject* npObject,
NPIdentifier propertyName)
 
     if (V8NPObject* object = npObjectToV8NPObject(npObject)) {
         v8::Isolate* isolate = v8::Isolate::GetCurrent();
-        v8::HandleScope handleScope(isolate);
-        v8::Handle<v8::Context> context = mainWorldContext(isolate, npp,
npObject);
-        if (context.IsEmpty())
+        ScriptState* scriptState = mainWorldScriptState(isolate, npp,
npObject);
+        if (!scriptState)
             return false;
-        v8::Context::Scope scope(context);
+        ScriptState::Scope scope(scriptState);
         ExceptionCatcher exceptionCatcher;
 
         v8::Handle<v8::Object> obj = v8::Local<v8::Object>::New(isolate,
object->v8Object);
@@ -477,11 +472,10 @@ bool _NPN_HasMethod(NPP npp, NPObject* npObject,
NPIdentifier methodName)
 
     if (V8NPObject* object = npObjectToV8NPObject(npObject)) {
         v8::Isolate* isolate = v8::Isolate::GetCurrent();
-        v8::HandleScope handleScope(isolate);
-        v8::Handle<v8::Context> context = mainWorldContext(isolate, npp,
npObject);
-        if (context.IsEmpty())
+        ScriptState* scriptState = mainWorldScriptState(isolate, npp,
npObject);
+        if (!scriptState)
             return false;
-        v8::Context::Scope scope(context);
+        ScriptState::Scope scope(scriptState);
         ExceptionCatcher exceptionCatcher;
 
         v8::Handle<v8::Object> obj = v8::Local<v8::Object>::New(isolate,
object->v8Object);
@@ -504,15 +498,14 @@ void _NPN_SetException(NPObject* npObject, const NPUTF8
*message)
     }
 
     v8::Isolate* isolate = v8::Isolate::GetCurrent();
-    v8::HandleScope handleScope(isolate);
-    v8::Handle<v8::Context> context = mainWorldContext(isolate, 0, npObject);
-    if (context.IsEmpty())
+    ScriptState* scriptState = mainWorldScriptState(isolate, 0, npObject);
+    if (!scriptState)
         return;
 
-    v8::Context::Scope scope(context);
+    ScriptState::Scope scope(scriptState);
     ExceptionCatcher exceptionCatcher;
 
-    throwError(v8GeneralError, message, context->GetIsolate());
+    throwError(v8GeneralError, message, isolate);
 }
 
 bool _NPN_Enumerate(NPP npp, NPObject* npObject, NPIdentifier** identifier,
uint32_t* count)
@@ -522,11 +515,10 @@ bool _NPN_Enumerate(NPP npp, NPObject* npObject,
NPIdentifier** identifier, uint
 
     if (V8NPObject* object = npObjectToV8NPObject(npObject)) {
         v8::Isolate* isolate = v8::Isolate::GetCurrent();
-        v8::HandleScope handleScope(isolate);
-        v8::Local<v8::Context> context = mainWorldContext(isolate, npp,
npObject);
-        if (context.IsE…
(message too large)

[haraken, 2014-06-03 00:33:42]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-03 00:34:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/306853002/70001

[commit-bot: I haz the power, 2014-06-03 03:53:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9978)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10266)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/12068)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/15153)

[commit-bot: I haz the power, 2014-06-03 04:07:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-03 04:07:03]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/12086)

[haraken, 2014-06-03 06:57:20]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-03 06:57:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/306853002/90001

[commit-bot: I haz the power, 2014-06-03 11:05:29]

Change committed as 175344