Bug 100235 – [V8] Remove ScriptController::windowShell()

• Component: New Bugs
• Status: RESOLVED FIXED

[Dan Carney]

[V8] Remove ScriptController::windowShell()

[Dan Carney]

Created attachment 170370 [details]
Patch

[WebKit Review Bot]

Comment on attachment 170370 [details]
Patch

Attachment 170370 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14545216

New failing tests:
fast/frames/frame-limit.html

[WebKit Review Bot]

Comment on attachment 170370 [details]
Patch

Attachment 170370 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14553208

New failing tests:
fast/frames/frame-limit.html

[Adam Barth]

Comment on attachment 170370 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170370&action=review

This looks great!  I'm not sure what's going on with the test failure, but you might want to investigate that before landing.

> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:70
> +    for (WorldMap::iterator i = isolatedWorlds.begin(), e = isolatedWorlds.end(); i != e; ++i)

style nit: we usually use "it" rather than "i" for iterators.  Also, there's no reason to store "e" in a variable.  You can just call end() each time in the loop condition.

> Source/WebCore/bindings/v8/ScriptController.cpp:492
> +    v8::Handle<v8::Context> v8Context = m_windowShell->context();
>      v8Context->AllowCodeGenerationFromStrings(true);

I would combine these lines.

> Source/WebCore/bindings/v8/ScriptController.h:176
>      // Dummy method to avoid a bunch of ifdef's in WebCore.

Is this comment still accurate?

[Dan Carney]

Created attachment 170898 [details]
Patch

[Dan Carney]

Adam, the comments you've made have been addressed. The failing test was timing out because ScriptController::updateDocument() was causing unnecessary calls to v8::Context::New (2X the number needed), which it apparently a very slow process. To fix this I added the below clause:

void ScriptController::updateDocument()
{
    if (!m_frame->loader()->frameHasLoaded())
        return;

I really have no particular idea if this is the correct solution here or if there is a more appropriate check, but this solution at least passes all the tests in debug.

[Adam Barth]

Comment on attachment 170898 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170898&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3421
> +        if (frame->script()->initializeMainWorld()) {

There's no "IfNeeded" here?

> Source/WebCore/bindings/v8/ScriptController.cpp:328
> +static DOMWrapperWorld* existingWindowShellWorkaroundWorld()

Sometimes we use the name "depreciated" in functions we don't want more people to call, but this is fine too.

> Source/WebCore/bindings/v8/ScriptController.cpp:720
> +    if (!m_frame->loader()->frameHasLoaded())
> +        return;

What is this?

[Adam Barth]

> Adam, the comments you've made have been addressed. The failing test was timing out because ScriptController::updateDocument() was causing unnecessary calls to v8::Context::New (2X the number needed), which it apparently a very slow process.

Uh...  Yes!  We don't want to call v8::Context::New any extra times.  It's not that it is slow.  We don't wan to to be making extra context objects!

> To fix this I added the below clause:
> 
> void ScriptController::updateDocument()
> {
>     if (!m_frame->loader()->frameHasLoaded())
>         return;
> 
> I really have no particular idea if this is the correct solution here or if there is a more appropriate check, but this solution at least passes all the tests in debug.

This is very unlikely to be the right thing.  I don't really understand the issue.  Why were we creating more contexts than we should have?  The way this worked before is that we'd always try to initialize things, and that would bail out early if the context was already initialized.  It seems odd that we'd end up calling v8::Context::New multiple times.

[Dan Carney]

Created attachment 171254 [details]
Patch

[Dan Carney]

(In reply to comment #7)
> (From update of attachment 170898 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170898&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3421
> > +        if (frame->script()->initializeMainWorld()) {
> 
> There's no "IfNeeded" here?
> 
> > Source/WebCore/bindings/v8/ScriptController.cpp:328

This is a new method. It returns true if the world wasn't initialized and initialization was successful. The semantics are slightly different that the method on V8DOMWindowShell, so I wasn't sure what to call it. It is exactly equivalent to:

windowShell.context().IsEmpty() && windowShell. initializeIfNeeded()

[Dan Carney]

> This is very unlikely to be the right thing.  I don't really understand the issue.  Why were we creating more contexts than we should have?  The way this worked before is that we'd always try to initialize things, and that would bail out early if the context was already initialized.  It seems odd that we'd end up calling v8::Context::New multiple times.

The reason this has changes is that I have moved V8DOMWindowShell initialization into ScriptController.  In fact almost all functions in V8DOMWindowShell are now only called by ScriptController function. The previous implementation managed to not init the window shell during FrameLoader::init(), which is when a frame is initially attached but instead on frame access, which is what we want. An empty frame that is never used (as in the failing test), does not need to init the context. I have implemented that same workaround in this time around in ScriptController::updateDocument, albeit with slightly different logic, since I do in ScriptController rather than in the window shell.

[Dan Carney]

Created attachment 173219 [details]
Patch

[Dan Carney]

Comment on attachment 173219 [details]
Patch

rebased

[Adam Barth]

Comment on attachment 173219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173219&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3410
> -        if (frame->script()->windowShell()->context().IsEmpty() && frame->script()->windowShell()->initializeIfNeeded()) {
> -            // initializeIfNeeded may have created a wrapper for the object, retry from the start.
> +        if (frame->script()->initializeMainWorld()) {
> +            // initializeMainWorld may have created a wrapper for the object, retry from the start.

You need to run-bindings-tests --reset-results to avoid turning the bot red.

> Source/WebCore/bindings/v8/ScriptController.cpp:348
> -    return iter == m_isolatedWorlds.end() ? 0 : iter->value;
> +    return iter == m_isolatedWorlds.end() ? 0 : iter->value->isContextInitialized() ? iter->value : 0;

I would probably have broken this down into multiple statements.  Nested ternary operators can be slightly hard to read.

> Source/WebCore/bindings/v8/ScriptController.cpp:369
> +    if (!shell->isContextInitialized()) {
> +        if (shell->initializeIfNeeded()) {

These lines can be combined.  Do we really need to call isContextInitialized?  I would think that initializeIfNeeded would check isContextInitialized because it says "if needed".

> Source/WebCore/bindings/v8/ScriptController.cpp:727
> +    if ((!m_windowShell->isContextInitialized() || !m_windowShell->isGlobalInitialized()) && m_frame->loader()->stateMachine()->creatingInitialEmptyDocument())

This line still seems strange, but I think we should worry about it later.

[Adam Barth]

Created attachment 173333 [details]
Patch for landing

[Adam Barth]

To save you the timezone round-trip, I applied these trivial comments for you.

[WebKit Review Bot]

Comment on attachment 173333 [details]
Patch for landing

Clearing flags on attachment: 173333

Committed r134089: <http://trac.webkit.org/changeset/134089>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.