Bug 96522 – [V8] Give isolated shells a lifecycle like that of main shells

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dan Carney]

Give isolated shells a lifecycle like that of main shells

[Dan Carney]

Created attachment 163637 [details]
Patch

[Dan Carney]

I don't fully trust this, as the context global object stores a raw pointer to the isolated shell, so a callback coming back after the death of the shell, which happens on ScriptController deletion would segfault.  Is it possible for such a callback to happen?

[Adam Barth]

Comment on attachment 163637 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptController.cpp:115
> +    for (IsolatedWorldMap::iterator iter = m_isolatedWorlds.begin();
> +         iter != m_isolatedWorlds.end(); ++iter) {
> +        iter->second->destroyGlobal();
> +    }

Typically, we'd put the loop condition on one line and drop the { }

> Source/WebCore/bindings/v8/ScriptController.cpp:337
>      OwnPtr<V8DOMWindowShell> isolatedWorldShell = V8DOMWindowShell::create(m_frame, world);
> -    m_isolatedWorlds.set(world->worldId(), isolatedWorldShell.get());
> +    m_isolatedWorlds.set(world->worldId(), adoptPtr(isolatedWorldShell.get()));
>      return isolatedWorldShell.leakPtr();

This isn't quite the right idiom.  What we typically do in these cases is to introduce a local variable to hold a V8DOMWidowShell* like this:

V8DOMWindowShell* result = isolatedWorldShell.get();
m_isolatedWorlds.set(world->worldId(), isolatedWorldShell.release());
return result;

The reason we do this is because we use calls to leakPtr as a signal that we might have a memory leak.  Keeping the call to leakPtr here will make this show up on our periodic leakPtr audits.

> Source/WebCore/bindings/v8/ScriptController.cpp:360
>      OwnPtr<V8DOMWindowShell> isolatedWorldShell = V8DOMWindowShell::create(m_frame, world);
> -    m_isolatedWorlds.set(world->worldId(), isolatedWorldShell.get());
> +    m_isolatedWorlds.set(world->worldId(), adoptPtr(isolatedWorldShell.get()));
>      return isolatedWorldShell.leakPtr();
>  }

Same here.

> Source/WebCore/bindings/v8/ScriptController.cpp:403
> +            V8DOMWindowShell* ownedShell = it->second.leakPtr();
> +            ASSERT(ownedShell == isolatedWorldShell);
> +            m_isolatedWorlds.remove(it);
> +            ownedShell->destroyIsolatedShell();

Again, we should avoid calling leakPtr.  The point of OwnPtr is for the C++ type system to help us call new and delete exactly the right number of times.  Calling leakPtr is fighting the type system rather than working with it.

How about:

OwnPtr<V8DOMWindowShell> windowShell = it->second.release();
m_isolatedWorlds.remove(it);
windowShell->destroyIsolatedShell();

[Adam Barth]

Comment on attachment 163637 [details]
Patch

I don't understand how you avoided removing this line in V8DOMWindowShell:

    delete static_cast<V8DOMWindowShell*>(parameter);

It seems like we're still managing the lifetime of the V8DOMWindowShell via the V8 GC.

[Dan Carney]

Comment on attachment 163637 [details]
Patch

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

>> Source/WebCore/bindings/v8/ScriptController.cpp:115
>> +    }
> 
> Typically, we'd put the loop condition on one line and drop the { }

ok

>> Source/WebCore/bindings/v8/ScriptController.cpp:337
>>      return isolatedWorldShell.leakPtr();
> 
> This isn't quite the right idiom.  What we typically do in these cases is to introduce a local variable to hold a V8DOMWidowShell* like this:
> 
> V8DOMWindowShell* result = isolatedWorldShell.get();
> m_isolatedWorlds.set(world->worldId(), isolatedWorldShell.release());
> return result;
> 
> The reason we do this is because we use calls to leakPtr as a signal that we might have a memory leak.  Keeping the call to leakPtr here will make this show up on our periodic leakPtr audits.

That makes sense.  Okay, I wasn't sure what the typical usage was, and I was trying to avoid a local variable for something I already had.

>> Source/WebCore/bindings/v8/ScriptController.cpp:360
>>  }
> 
> Same here.

yep.

>> Source/WebCore/bindings/v8/ScriptController.cpp:403
>> +            ownedShell->destroyIsolatedShell();
> 
> Again, we should avoid calling leakPtr.  The point of OwnPtr is for the C++ type system to help us call new and delete exactly the right number of times.  Calling leakPtr is fighting the type system rather than working with it.
> 
> How about:
> 
> OwnPtr<V8DOMWindowShell> windowShell = it->second.release();
> m_isolatedWorlds.remove(it);
> windowShell->destroyIsolatedShell();

Actually, here, we need to specifically take ownership and leak the pointer as this shell must remain weak.  A strong deletion here is a bad thing because of callbacks.

[Adam Barth]

Comment on attachment 163637 [details]
Patch

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

>>> Source/WebCore/bindings/v8/ScriptController.cpp:403
>>> +            ownedShell->destroyIsolatedShell();
>> 
>> Again, we should avoid calling leakPtr.  The point of OwnPtr is for the C++ type system to help us call new and delete exactly the right number of times.  Calling leakPtr is fighting the type system rather than working with it.
>> 
>> How about:
>> 
>> OwnPtr<V8DOMWindowShell> windowShell = it->second.release();
>> m_isolatedWorlds.remove(it);
>> windowShell->destroyIsolatedShell();
> 
> Actually, here, we need to specifically take ownership and leak the pointer as this shell must remain weak.  A strong deletion here is a bad thing because of callbacks.

Why you say callbacks, what do you mean?  If we're trying to make the lifetime of the isolated window shells the same as the main window shells, that means they need to live as long as the frame lives and not have their lifetime determined by the garbage collector.

[Dan Carney]

(In reply to comment #6)
> (From update of attachment 163637 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163637&action=review
> 
> >>> Source/WebCore/bindings/v8/ScriptController.cpp:403
> >>> +            ownedShell->destroyIsolatedShell();
> >> 
> >> Again, we should avoid calling leakPtr.  The point of OwnPtr is for the C++ type system to help us call new and delete exactly the right number of times.  Calling leakPtr is fighting the type system rather than working with it.
> >> 
> >> How about:
> >> 
> >> OwnPtr<V8DOMWindowShell> windowShell = it->second.release();
> >> m_isolatedWorlds.remove(it);
> >> windowShell->destroyIsolatedShell();
> > 
> > Actually, here, we need to specifically take ownership and leak the pointer as this shell must remain weak.  A strong deletion here is a bad thing because of callbacks.
> 
> Why you say callbacks, what do you mean?  If we're trying to make the lifetime of the isolated window shells the same as the main window shells, that means they need to live as long as the frame lives and not have their lifetime determined by the garbage collector.

Just for the single use shells.  The ones that enter evaluateInIsolatedWorld with an uninitializedWorldId or in the test environment with a worldId of 0.  I'm not exactly sure how to get one of them to execute outside of the test environment, but I know that PageGroup keeps a bunch of references to them.  I 'm happy to remove all that code and leave the references alive until ScriptController deletion.  It's just that those particular shell can't be accessed except from callbacks at that point, so we're keeping a lot of stuff alive unnecessarily.

[Dan Carney]

> Why you say callbacks, what do you mean?  

Sorry, asynchronous javascript callbacks.   The shell needs to stay alive long enough for them not to segfault as a raw pointer to the shell exists in the context's global object.

[Adam Barth]

> Sorry, asynchronous javascript callbacks.   The shell needs to stay alive long enough for them not to segfault as a raw pointer to the shell exists in the context's global object.

Can we clear out their pointer to the WindowShell?  In the main world, we detach the v8::Context from the WindowShell and it continues to function.

[Adam Barth]

Comment on attachment 163637 [details]
Patch

By the way, I think we should land this patch and then iterate on this temporary world issue.  If they're only used in testing, then it is not super important that we'll keep them alive longer.

It's important that we don't have a different code path for testing than in production.  We want our testing to be as close to production as possible.

[Dan Carney]

(In reply to comment #9)
> > Sorry, asynchronous javascript callbacks.   The shell needs to stay alive long enough for them not to segfault as a raw pointer to the shell exists in the context's global object.
> 
> Can we clear out their pointer to the WindowShell?  In the main world, we detach the v8::Context from the WindowShell and it continues to function.

It's a bit problematic. If it were cleared, the call to V8DOMWindowShell::entered() return null and which indicated that we're in the mainworld, which is not where the callbacks are suppose to execute.

[Dan Carney]

(In reply to comment #10)
> (From update of attachment 163637 [details])
> By the way, I think we should land this patch and then iterate on this temporary world issue.  If they're only used in testing, then it is not super important that we'll keep them alive longer.
> 
> It's important that we don't have a different code path for testing than in production.  We want our testing to be as close to production as possible.

There's definitely used.  But I think you need a particular extension property or maybe a platform app to hit the code path.  I've tried a number of variants, but haven't found one that works.  This code path trigger a canary crash a few weeks ago when I tried to remove IsolatedWorld, so it's  definitely used.

[Adam Barth]

> It's a bit problematic. If it were cleared, the call to V8DOMWindowShell::entered() return null and which indicated that we're in the mainworld, which is not where the callbacks are suppose to execute.

Yeah, we probably need to change that design as well.

Ok, let's keep this for now.  It shouldn't block the main thing you're trying to accomplish.

[Dan Carney]

I was too worried about the raw pointer in the context global object, so I've refactored it out in another patch.

[Dan Carney]

Created attachment 164116 [details]
Patch

[WebKit Review Bot]

Comment on attachment 164116 [details]
Patch

Rejecting attachment 164116 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
1 FAILED at 246.
Hunk #2 succeeded at 264 (offset 1 line).
1 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/bindings/v8/V8DOMWindowShell.cpp.rej
patching file Source/WebCore/bindings/v8/V8DOMWindowShell.h
Hunk #1 FAILED at 74.
Hunk #2 FAILED at 104.
2 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/bindings/v8/V8DOMWindowShell.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13839932

[Kentaro Hara]

Looks like you need to rebase it.

[Dan Carney]

(In reply to comment #17)
> Looks like you need to rebase it.

No, it got applied just after a previous patch got reverted.  I'll have to resubmit this once that patch goes through again.

[Dan Carney]

Created attachment 175804 [details]
Patch

[Kentaro Hara]

Comment on attachment 175804 [details]
Patch

LGTM. abarth will want to look.

[WebKit Review Bot]

Comment on attachment 175804 [details]
Patch

Clearing flags on attachment: 175804

Committed r135687: <http://trac.webkit.org/changeset/135687>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.