Make DOMWrapperWorld::current() callable from workers

Make DOMWrapperWorld::current() callable from workers

This CL does the following two things:

- Makes DOMWrapperWorld::current() callable from workers. Now that a world is always stored in PerContextDataHolder::world(), DOMWrapperWorld::current() can just look up the PerContextDataHolder::world().

- Makes DOMWrapperWorld::current() callable even when a window object is being initialized. It's possible that some methods call DOMWrapperWorld::current() while the window is being initialized. To handles the subtle situation, this CL adds DOMWrapperWorld::worldOfInitializingWindow.


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

[haraken, 2014-02-27 13:25:50]

PTAL

This change is needed to fix bot failures in
https://codereview.chromium.org/176963017/

[dcarney, 2014-02-27 17:03:44]

i'm not sure this makes sense. if a worker calls DOMWrapperWorld::current, we
never want to return worldOfInitializingWindow

https://codereview.chromium.org/182903003/diff/1/Source/bindings/v8/DOMWrappe...
File Source/bindings/v8/DOMWrapperWorld.h (left):

https://codereview.chromium.org/182903003/diff/1/Source/bindings/v8/DOMWrappe...
Source/bindings/v8/DOMWrapperWorld.h:71:
ASSERT(contextHasCorrectPrototype(context));
This ASSERT has saved us a lot of debugging in the past.  It would be nice not
to lose it.

[jochen (gone - plz use gerrit), 2014-02-27 22:40:59]

I agree, this looks way to magic.

[haraken, 2014-02-28 00:06:24]

> if a worker calls DOMWrapperWorld::current, we
> never want to return worldOfInitializingWindow

Good point. I added an isMainThread() check to DOMWrapperWorld::current().

This CL is doing something magical, but the magic has existed since a long time
ago. We need to handle a situation where current() is called while a window
object is being initialized. Previously the nasty situation was handled by
V8WindowShell::contextHasCorrectPrototype(). In this CL, the nasty situation is
handled by DOMWrapperWorld::worldOfWindowBeingInitialized.

https://codereview.chromium.org/182903003/diff/1/Source/bindings/v8/DOMWrappe...
File Source/bindings/v8/DOMWrapperWorld.h (left):

https://codereview.chromium.org/182903003/diff/1/Source/bindings/v8/DOMWrappe...
Source/bindings/v8/DOMWrapperWorld.h:71:
ASSERT(contextHasCorrectPrototype(context));
On 2014/02/27 17:03:44, dcarney wrote:
> This ASSERT has saved us a lot of debugging in the past.  It would be nice not
> to lose it.

Done. Reverted back the ASSERT.

[haraken, 2014-02-28 00:06:32]

PTAL

[haraken, 2014-02-28 07:46:31]

On 2014/02/28 00:06:32, haraken wrote:
> PTAL

Several tests are failing because in tests there is no world installed on the
current context...

To address the issue, we just need to install a world on the testing context.
However, in the current infrastructure, we need to create all of
DOMWrapperWorld, Context, V8PerContextData, V8PerContextDataHolder to install a
world. This is verbose.

I'll refactor V8PerContextData so that we can do that easily.

[dcarney, 2014-02-28 10:18:59]

On 2014/02/28 07:46:31, haraken wrote:
> On 2014/02/28 00:06:32, haraken wrote:
> > PTAL
> 
> Several tests are failing because in tests there is no world installed on the
> current context...
> 
> To address the issue, we just need to install a world on the testing context.
> However, in the current infrastructure, we need to create all of
> DOMWrapperWorld, Context, V8PerContextData, V8PerContextDataHolder to install
a
> world. This is verbose.
> 
> I'll refactor V8PerContextData so that we can do that easily.

lgtm

[haraken, 2014-03-04 04:19:44]

Now that I landed necessary changes to ensure that DOMWrapperWorld always exists
in all webkit_unit_tests, I think it's safe to land this CL.

Dan: Let me check one thing before landing.

https://codereview.chromium.org/182903003/diff/160001/Source/bindings/v8/DOMW...
File Source/bindings/v8/DOMWrapperWorld.h (left):

https://codereview.chromium.org/182903003/diff/160001/Source/bindings/v8/DOMW...
Source/bindings/v8/DOMWrapperWorld.h:71:
ASSERT(contextHasCorrectPrototype(context));

I removed this ASSERT for the following reasons:

- Now DOMWrapperWorld::world() is called from anywhere (worker worlds, testing
worlds etc). So if we really want to have the ASSERT, we need to write:

#ifdef NDEBUG
  if (world->isMainWorld() || world->isIsolatedWorld())
    ASSERT(contextHasCorrectPrototype(context));
#endif

- Instead of having such complicated ASSERT, we already have a better ASSERT in
V8PerContextData. Specifically, V8PerContextData already has an ASSERT to check
that the context has a non-0 world inside the embedder data of the context. I
think this ASSERT is enough to verify that we're calling
DOMWrapperWorld::world() for a valid context.

What do you think?

[dcarney, 2014-03-04 14:50:09]

On 2014/03/04 04:19:44, haraken wrote:
> Now that I landed necessary changes to ensure that DOMWrapperWorld always
exists
> in all webkit_unit_tests, I think it's safe to land this CL.
> 
> Dan: Let me check one thing before landing.
> 
>
https://codereview.chromium.org/182903003/diff/160001/Source/bindings/v8/DOMW...
> File Source/bindings/v8/DOMWrapperWorld.h (left):
> 
>
https://codereview.chromium.org/182903003/diff/160001/Source/bindings/v8/DOMW...
> Source/bindings/v8/DOMWrapperWorld.h:71:
> ASSERT(contextHasCorrectPrototype(context));
> 
> I removed this ASSERT for the following reasons:
> 
> - Now DOMWrapperWorld::world() is called from anywhere (worker worlds, testing
> worlds etc). So if we really want to have the ASSERT, we need to write:
> 
> #ifdef NDEBUG
>   if (world->isMainWorld() || world->isIsolatedWorld())
>     ASSERT(contextHasCorrectPrototype(context));
> #endif
> 
> - Instead of having such complicated ASSERT, we already have a better ASSERT
in
> V8PerContextData. Specifically, V8PerContextData already has an ASSERT to
check
> that the context has a non-0 world inside the embedder data of the context. I
> think this ASSERT is enough to verify that we're calling
> DOMWrapperWorld::world() for a valid context.
> 
> What do you think?

okay, lgtm

[haraken, 2014-03-11 01:00:58]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-03-11 01:01:12]

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

[commit-bot: I haz the power, 2014-03-11 01:58:25]

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

[commit-bot: I haz the power, 2014-03-11 01:58:26]

Try jobs failed on following builders: win_blink_rel

[haraken, 2014-03-11 02:23:49]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-03-11 02:23:58]

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

[commit-bot: I haz the power, 2014-03-11 03:09:23]

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

[commit-bot: I haz the power, 2014-03-11 03:09:23]

Try jobs failed on following builders: win_blink_rel

[haraken, 2014-03-11 03:44:44]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-03-11 03:44:54]

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

[commit-bot: I haz the power, 2014-03-11 03:45:19]

Change committed as 168880

[haraken, 2014-03-11 07:06:44]

The CQ bit was checked by haraken@chromium.org

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

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

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

Change committed as 168890

[haraken, 2014-03-11 07:11:38]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-03-11 07:11:53]

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

[commit-bot: I haz the power, 2014-03-11 08:40:51]

Change committed as 168898