Bug 100235 - [V8] Remove ScriptController::windowShell()
Summary: [V8] Remove ScriptController::windowShell()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-24 05:02 PDT by Dan Carney
Modified: 2012-11-09 11:11 PST (History)
5 users (show)

See Also:


Attachments
Patch (24.60 KB, patch)
2012-10-24 05:06 PDT, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (25.82 KB, patch)
2012-10-26 05:04 PDT, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (26.15 KB, patch)
2012-10-29 09:08 PDT, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (26.18 KB, patch)
2012-11-09 00:07 PST, Dan Carney
no flags Details | Formatted Diff | Diff
Patch for landing (24.89 KB, patch)
2012-11-09 10:26 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Carney 2012-10-24 05:02:36 PDT
[V8] Remove ScriptController::windowShell()
Comment 1 Dan Carney 2012-10-24 05:06:51 PDT
Created attachment 170370 [details]
Patch
Comment 2 WebKit Review Bot 2012-10-24 06:24:27 PDT
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
Comment 3 WebKit Review Bot 2012-10-24 07:20:12 PDT
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
Comment 4 Adam Barth 2012-10-24 09:00:11 PDT
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?
Comment 5 Dan Carney 2012-10-26 05:04:24 PDT
Created attachment 170898 [details]
Patch
Comment 6 Dan Carney 2012-10-26 05:12:20 PDT
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.
Comment 7 Adam Barth 2012-10-26 15:55:24 PDT
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?
Comment 8 Adam Barth 2012-10-26 15:57:45 PDT
> 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.
Comment 9 Dan Carney 2012-10-29 09:08:22 PDT
Created attachment 171254 [details]
Patch
Comment 10 Dan Carney 2012-10-29 09:14:08 PDT
(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()
Comment 11 Dan Carney 2012-10-29 09:21:43 PDT
> 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.
Comment 12 Dan Carney 2012-11-09 00:07:58 PST
Created attachment 173219 [details]
Patch
Comment 13 Dan Carney 2012-11-09 00:08:30 PST
Comment on attachment 173219 [details]
Patch

rebased
Comment 14 Adam Barth 2012-11-09 10:01:06 PST
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.
Comment 15 Adam Barth 2012-11-09 10:26:04 PST
Created attachment 173333 [details]
Patch for landing
Comment 16 Adam Barth 2012-11-09 10:26:46 PST
To save you the timezone round-trip, I applied these trivial comments for you.
Comment 17 WebKit Review Bot 2012-11-09 11:11:24 PST
Comment on attachment 173333 [details]
Patch for landing

Clearing flags on attachment: 173333

Committed r134089: <http://trac.webkit.org/changeset/134089>
Comment 18 WebKit Review Bot 2012-11-09 11:11:28 PST
All reviewed patches have been landed.  Closing bug.