Content scripts want this notification as well.
Created attachment 32490 [details] simple patch
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()?
(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.
> > 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?
(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?
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.
Committed http://trac.webkit.org/changeset/45871.