Remove ScriptState::current() from IDBRequest

Remove ScriptState::current() from IDBRequest

- ScriptState::current() should be called only from the binding layer. This CL removes ScriptState::current() from IDBRequest. For that goal, this CL passes ScriptState around in a lot of places in IDB code.

- This CL also introduces ScriptStateForTesting. It's hard to use ScriptState for testing purposes because webkit_unit_tests doesn't create DOMWindow nor ExecutionContext, and thus ScriptState cannot be associated with a valid ExecutionContext. On the other hand, ScriptStateForTesting allows us to mock the ExecutionContext for testing purpose.

- This CL renames V8ExecutionScope to V8TestingScope. This is because V8TestingScope uses ScriptStateForTesting, and thus it's only for testing.

- This CL stops using V8ExecutionScope in V8GCController, because V8GCController is not for testing.

BUG=357144
TBR=adamk

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

[haraken, 2014-05-23 16:50:49]

PTAL

[jsbell, 2014-05-23 17:21:34]

This patch looks very familiar - prior to the NewScriptState change I had a
nearly identical patch locally! So the bulk of the change l-g-t-m but...

This failure is real:

[ RUN      ] IDBTransactionTest.EnsureLifetime
../../third_party/WebKit/Source/modules/indexeddb/IDBTransactionTest.cpp:132:
Failure
Value of: transaction->refCount()
  Actual: 2
Expected: 1
[  FAILED  ] IDBTransactionTest.EnsureLifetime (0 ms)

I remember seeing the same failure when playing with my version of this patch,
but didn't get around to debugging it.

https://codereview.chromium.org/295163005/diff/1/Source/modules/indexeddb/IDB...
File Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/295163005/diff/1/Source/modules/indexeddb/IDB...
Source/modules/indexeddb/IDBObjectStore.cpp:351: virtual void
handleEvent(ExecutionContext* executionContext, Event* event) OVERRIDE
The executionContext parameter is now unused.

https://codereview.chromium.org/295163005/diff/1/Source/modules/indexeddb/IDB...
Source/modules/indexeddb/IDBObjectStore.cpp:374:
generateIndexKeysForValue(m_scriptState->isolate(), m_indexMetadata, value,
&indexKeys);
And this is the only real change in the patch - moving from using the passed in
context to the stored context.

Given that the IndexPopulator is entirely hidden from script it shouldn't matter
- can we ASSERT that m_scriptState's context is the same as the passed
ExecutionContext?

[haraken, 2014-05-23 18:07:15]

Thanks for review!

> This failure is real:
> 
> [ RUN      ] IDBTransactionTest.EnsureLifetime
> ../../third_party/WebKit/Source/modules/indexeddb/IDBTransactionTest.cpp:132:
> Failure
> Value of: transaction->refCount()
>   Actual: 2
> Expected: 1
> [  FAILED  ] IDBTransactionTest.EnsureLifetime (0 ms)
> 
> I remember seeing the same failure when playing with my version of this patch,
> but didn't get around to debugging it.

The cause is that...

- IDBTransactionTest creates a dedicated ExecutionContext for the test
(IDBTransactionTest::m_document).
- IDBTransactionTest creates a dedicated ScriptState for the test
(IDBTransactionTest::m_scope).
- The test uses IDBTransactionTest::m_document as its execution context, but if
the test calls IDBRequest::create(ScriptState::current()), the IDBRequest uses
the execution context associated with the ScriptState. These two execution
contexts are different, so the test fails.

Probably we need a way to inject an execution context to the ScriptState, so
that the ScriptState can be associated with the execution context that the test
is actually using.

[haraken, 2014-05-26 07:01:41]

On 2014/05/23 18:07:15, haraken wrote:
> Thanks for review!
> 
> > This failure is real:
> > 
> > [ RUN      ] IDBTransactionTest.EnsureLifetime
> >
../../third_party/WebKit/Source/modules/indexeddb/IDBTransactionTest.cpp:132:
> > Failure
> > Value of: transaction->refCount()
> >   Actual: 2
> > Expected: 1
> > [  FAILED  ] IDBTransactionTest.EnsureLifetime (0 ms)
> > 
> > I remember seeing the same failure when playing with my version of this
patch,
> > but didn't get around to debugging it.
> 
> The cause is that...
> 
> - IDBTransactionTest creates a dedicated ExecutionContext for the test
> (IDBTransactionTest::m_document).
> - IDBTransactionTest creates a dedicated ScriptState for the test
> (IDBTransactionTest::m_scope).
> - The test uses IDBTransactionTest::m_document as its execution context, but
if
> the test calls IDBRequest::create(ScriptState::current()), the IDBRequest uses
> the execution context associated with the ScriptState. These two execution
> contexts are different, so the test fails.
> 
> Probably we need a way to inject an execution context to the ScriptState, so
> that the ScriptState can be associated with the execution context that the
test
> is actually using.

jsbell@: Fixed the issue by introducing ScriptStateForTesting. (See the CL
description for more details.)

PTAL.

[haraken, 2014-05-26 07:04:02]

https://codereview.chromium.org/295163005/diff/1/Source/modules/indexeddb/IDB...
File Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/295163005/diff/1/Source/modules/indexeddb/IDB...
Source/modules/indexeddb/IDBObjectStore.cpp:374:
generateIndexKeysForValue(m_scriptState->isolate(), m_indexMetadata, value,
&indexKeys);
On 2014/05/23 17:21:35, jsbell wrote:
> And this is the only real change in the patch - moving from using the passed
in
> context to the stored context.
> 
> Given that the IndexPopulator is entirely hidden from script it shouldn't
matter
> - can we ASSERT that m_scriptState's context is the same as the passed
> ExecutionContext?

I added the ASSERT.

However, this change won't change anything. Here we're just getting an Isolate,
and the Isolate must be the same no matter how we get it :)

[haraken, 2014-05-27 00:35:19]

jsbell@: friendly ping :)

[jsbell, 2014-05-27 01:45:46]

On 2014/05/27 00:35:19, haraken wrote:
> jsbell@: friendly ping :)

Yep - holiday here, will review when I have a bigger screen than my phone
tomorrow. I'll CQ+ it if you want.

[haraken, 2014-05-27 01:51:43]

On 2014/05/27 01:45:46, jsbell wrote:
> On 2014/05/27 00:35:19, haraken wrote:
> > jsbell@: friendly ping :)
> 
> Yep - holiday here, will review when I have a bigger screen than my phone
> tomorrow. I'll CQ+ it if you want.

oh, sorry I didn't notice that. Tomorrow is completely fine with me :)

[jsbell, 2014-05-27 18:38:47]

lgtm

https://codereview.chromium.org/295163005/diff/40001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptState.h (right):

https://codereview.chromium.org/295163005/diff/40001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptState.h:72: virtual void
setExecutionContext(PassRefPtr<ExecutionContext>) { }
ASSERT_NOT_REACHED() ?

https://codereview.chromium.org/295163005/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequestTest.cpp (right):

https://codereview.chromium.org/295163005/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequestTest.cpp:96: RefPtrWillBeRawPtr<IDBRequest>
request = IDBRequest::create(ScriptState::current(isolate()),
IDBAny::createUndefined(), transaction);
Just isolate() here, rather than ScriptState::current(isolate()) ?

(Here and 3 more cases below)

https://codereview.chromium.org/295163005/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBTransactionTest.cpp (right):

https://codereview.chromium.org/295163005/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBTransactionTest.cpp:111:
RefPtrWillBePersistent<IDBRequest> request =
IDBRequest::create(ScriptState::current(isolate()), IDBAny::createUndefined(),
transaction.get());
Just isolate() here rather than ScriptState::current(isolate()) ?

[haraken, 2014-05-28 00:35:49]

Thanks for review!

https://codereview.chromium.org/295163005/diff/40001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptState.h (right):

https://codereview.chromium.org/295163005/diff/40001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptState.h:72: virtual void
setExecutionContext(PassRefPtr<ExecutionContext>) { }
On 2014/05/27 18:38:47, jsbell wrote:
> ASSERT_NOT_REACHED() ?

Done.

https://codereview.chromium.org/295163005/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequestTest.cpp (right):

https://codereview.chromium.org/295163005/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequestTest.cpp:96: RefPtrWillBeRawPtr<IDBRequest>
request = IDBRequest::create(ScriptState::current(isolate()),
IDBAny::createUndefined(), transaction);
On 2014/05/27 18:38:47, jsbell wrote:
> Just isolate() here, rather than ScriptState::current(isolate()) ?
> 
> (Here and 3 more cases below)

We need to pass ScriptState*, so replaced with scriptState().

https://codereview.chromium.org/295163005/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBTransactionTest.cpp (right):

https://codereview.chromium.org/295163005/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBTransactionTest.cpp:111:
RefPtrWillBePersistent<IDBRequest> request =
IDBRequest::create(ScriptState::current(isolate()), IDBAny::createUndefined(),
transaction.get());
On 2014/05/27 18:38:47, jsbell wrote:
> Just isolate() here rather than ScriptState::current(isolate()) ?

Done.

[haraken, 2014-05-28 00:35:53]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-28 00:36:16]

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

[jsbell, 2014-05-28 01:21:56]

lgtm again!

https://codereview.chromium.org/295163005/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequestTest.cpp (right):

https://codereview.chromium.org/295163005/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequestTest.cpp:96: RefPtrWillBeRawPtr<IDBRequest>
request = IDBRequest::create(ScriptState::current(isolate()),
IDBAny::createUndefined(), transaction);
On 2014/05/28 00:35:49, haraken wrote:
> On 2014/05/27 18:38:47, jsbell wrote:
> > Just isolate() here, rather than ScriptState::current(isolate()) ?
> > 
> > (Here and 3 more cases below)
> 
> We need to pass ScriptState*, so replaced with scriptState().

Oops, yeah. That. :)

[haraken, 2014-05-28 01:46:08]

This CL caused serious conflicts with the oilpan CL. Will rebase and land.

[commit-bot: I haz the power, 2014-05-28 02:08:00]

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/6300)
  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/9383)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12819)

[commit-bot: I haz the power, 2014-05-28 02:17:13]

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

[commit-bot: I haz the power, 2014-05-28 02:17:14]

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

[haraken, 2014-05-28 02:32:33]

TBRing adamk@ for web/

[haraken, 2014-05-28 02:32:38]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-28 02:33:12]

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

[commit-bot: I haz the power, 2014-05-28 04:29:12]

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/9920)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9730)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9186)
  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/9395)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12860)

[commit-bot: I haz the power, 2014-05-28 04:55:36]

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

[commit-bot: I haz the power, 2014-05-28 04:55:37]

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

[haraken, 2014-05-28 05:04:29]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-28 05:04:48]

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

[commit-bot: I haz the power, 2014-05-28 06:21:58]

Change committed as 174937

[adamk, 2014-05-28 16:14:25]

rslgtm