Bug 27104 – [V8] evaluateInNewContext should send a notification that a context was created

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

[Matt Perry]

Content scripts want this notification as well.

[Matt Perry]

Created attachment 32490 [details]
simple patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 32490 [details]
simple patch

> Index: WebCore/bindings/v8/V8Proxy.cpp
> ===================================================================
> --- WebCore/bindings/v8/V8Proxy.cpp	(revision 45651)
> +++ WebCore/bindings/v8/V8Proxy.cpp	(working copy)
> @@ -1050,6 +1050,8 @@ void V8Proxy::evaluateInNewContext(const
>      // original context.
>      global->Set(v8::String::New("contentWindow"), windowGlobal);
>  
> +    m_frame->loader()->client()->didCreateScriptContext();
> +
>      // Run code in the new context.
>      for (size_t i = 0; i < sources.size(); ++i)
>          evaluate(sources[i], 0);

Shouldn't we also call didDestroyScriptContext()?

[Matt Perry]

(In reply to comment #2)
> Shouldn't we also call didDestroyScriptContext()?

There's no definitive place where this context is destroyed.  It's just garbage collected when it's not used.

The chrome side of this change listens for its GC, and also associates sub-contexts with their frame so that we can notify the script and clean up after it.

[Darin Fisher (:fishd, Google)]

> > Shouldn't we also call didDestroyScriptContext()?
> 
> There's no definitive place where this context is destroyed.  It's just garbage
> collected when it's not used.

A didCreateScriptContext without a corresponding didDestroyScriptContext seems like a recipe for memory leaks.  Someone not knowing any better might assume that for each "create" call they will get a "destroy" call, and on the destroy call expect to free resources.  Can you do anything to mitigate that kind of problem?

[Matt Perry]

(In reply to comment #4)
> Can you do anything to mitigate that kind of
> problem?

The first idea I had was to add a GC callback, and send out the didDestroyScriptContext then.  Unfortunately, since the content script context can outlive the frame, and we need to use the frameloader to send the event, we have nowhere to send it.  Maybe these events should be on the ChromiumClient API?

[Matt Perry]

Created attachment 32598 [details]
new notification

OK, instead of having didDestroyScriptContext only sometimes getting called, I renamed the create/destroy notifications for frames, and added a new one for evaluateInNewContext.  Hopefully this will make it clearer to callers what to expect.

[Matt Perry]

Committed http://trac.webkit.org/changeset/45871.