ScriptState::contextIsEmpty shouldn't return true for a context whose global object is detached

ScriptState::contextIsEmpty shouldn't return true for a context whose global object is detached

I cannot reproduce bug 385366 but my best guess is that V8AbstractEventListener::handleEvent calls context->Global() after the global object is detached from the context (because of the frame navigation).

Currently ScriptState is used in the following programming pattern:

class DOMObject {
  DOMObject(ScriptState* scriptState) : m_scriptState(scriptState) { }  // Record the script state when the DOMObject is created.

  void methodTriggeredAsynchrnously() {  // Blink calls this sometime later.
    if (m_scriptState->contextIsEmpty())  // If the context is already gone, we do nothing.
      return;
    ScriptState::Scope scope(m_scriptState.get());  // Otherwise, enter the context and do the actual work.
    ...; // Do the actual work.
  }

  RefPtr<ScriptState> m_scriptState;
};

The problem is that m_scriptState->contextIsEmpty() returns true even if the global object is detached from the context. We should return false in that case.

This CL also renames contextIsEmpty to contextIsValid for clarity.

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

[haraken, 2014-09-04 08:33:07]

haraken@chromium.org changed reviewers:
+ adamk@chromium.org, dcarney@chromium.org, jochen@chromium.org

[haraken, 2014-09-04 08:36:23]

PTAL

(A better fix is to clear ScriptState::m_context when we call
m_context->DetachGlobal(), but we cannot do it at this point because
ScriptState::m_context can be used after m_context->DetachGlobal() is called. It
looks not easy to fix it.)

[jochen (gone - plz use gerrit), 2014-09-04 08:37:39]

https://codereview.chromium.org/538933002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptState.h (right):

https://codereview.chromium.org/538933002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptState.h:94: ScopedPersistent<v8::Context>
m_context;
you can have several pointers to the same context, no? How will other script
state objects know that the global object was detached?

[haraken, 2014-09-04 08:39:23]

https://codereview.chromium.org/538933002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptState.h (right):

https://codereview.chromium.org/538933002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptState.h:94: ScopedPersistent<v8::Context>
m_context;
On 2014/09/04 08:37:39, jochen wrote:
> you can have several pointers to the same context, no? How will other script
> state objects know that the global object was detached?

ScriptState and v8::Context have the 1:1 relationship.

[jochen (gone - plz use gerrit), 2014-09-04 09:43:36]

On 2014/09/04 at 08:39:23, haraken wrote:
>
https://codereview.chromium.org/538933002/diff/20001/Source/bindings/core/v8/...
> File Source/bindings/core/v8/ScriptState.h (right):
> 
>
https://codereview.chromium.org/538933002/diff/20001/Source/bindings/core/v8/...
> Source/bindings/core/v8/ScriptState.h:94: ScopedPersistent<v8::Context>
m_context;
> On 2014/09/04 08:37:39, jochen wrote:
> > you can have several pointers to the same context, no? How will other script
> > state objects know that the global object was detached?
> 
> ScriptState and v8::Context have the 1:1 relationship.

i see.

i guess you want to replace s/ContextIsValue/ContextIsValid/? with that, lgtm

[haraken, 2014-09-04 09:44:20]

On 2014/09/04 09:43:36, jochen wrote:
> On 2014/09/04 at 08:39:23, haraken wrote:
> >
>
https://codereview.chromium.org/538933002/diff/20001/Source/bindings/core/v8/...
> > File Source/bindings/core/v8/ScriptState.h (right):
> > 
> >
>
https://codereview.chromium.org/538933002/diff/20001/Source/bindings/core/v8/...
> > Source/bindings/core/v8/ScriptState.h:94: ScopedPersistent<v8::Context>
> m_context;
> > On 2014/09/04 08:37:39, jochen wrote:
> > > you can have several pointers to the same context, no? How will other
script
> > > state objects know that the global object was detached?
> > 
> > ScriptState and v8::Context have the 1:1 relationship.
> 
> i see.
> 
> i guess you want to replace s/ContextIsValue/ContextIsValid/? with that, lgtm

Yes... will do.

[haraken, 2014-09-04 11:30:34]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-04 11:31:05]

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

[commit-bot: I haz the power, 2014-09-04 11:31:15]

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

[commit-bot: I haz the power, 2014-09-04 11:31:16]

Failed to apply patch for Source/bindings/core/v8/ScheduledAction.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/core/v8/ScheduledAction.cpp
  Hunk #1 FAILED at 83.
  Hunk #2 succeeded at 112 (offset 10 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
Source/bindings/core/v8/ScheduledAction.cpp.rej

Patch:       Source/bindings/core/v8/ScheduledAction.cpp
Index: Source/bindings/core/v8/ScheduledAction.cpp
diff --git a/Source/bindings/core/v8/ScheduledAction.cpp
b/Source/bindings/core/v8/ScheduledAction.cpp
index
b0315ae806413692cd7657f28bab68feba0427c7..7cc3d618fe790ff6a41c6f7d718bf59f04338d65
100644
--- a/Source/bindings/core/v8/ScheduledAction.cpp
+++ b/Source/bindings/core/v8/ScheduledAction.cpp
@@ -83,7 +83,7 @@ void ScheduledAction::execute(ExecutionContext* context)
 
 void ScheduledAction::execute(LocalFrame* frame)
 {
-    if (m_scriptState->contextIsEmpty())
+    if (m_scriptState->contextIsValid())
         return;
 
     TRACE_EVENT0("v8", "ScheduledAction::execute");
@@ -102,7 +102,7 @@ void ScheduledAction::execute(LocalFrame* frame)
 void ScheduledAction::execute(WorkerGlobalScope* worker)
 {
     ASSERT(worker->thread()->isCurrentThread());
-    ASSERT(!m_scriptState->contextIsEmpty());
+    ASSERT(!m_scriptState->contextIsValid());
     if (!m_function.isEmpty()) {
         ScriptState::Scope scope(m_scriptState.get());
         Vector<v8::Handle<v8::Value> > info;

[haraken, 2014-09-04 11:34:02]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-04 11:34:18]

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

[commit-bot: I haz the power, 2014-09-04 19:26:13]

Committed patchset #4 (id:60001) as 181392