[Webkit-unassigned] [Bug 96637] Remove V8DOMWindowShell::getEntered

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=96637





--- Comment #25 from Dan Carney <dcarney at google.com>  2012-09-17 13:06:16 PST ---
(In reply to comment #23)
> (From update of attachment 164371 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164371&action=review
> 
> > Source/WebCore/bindings/v8/ScriptController.cpp:447
> >  v8::Local<v8::Context> ScriptController::currentWorldContext()
> >  {
> > -    if (V8DOMWindowShell* isolatedShell = V8DOMWindowShell::getEntered()) {
> > -        v8::Persistent<v8::Context> context = isolatedShell->context();
> > -        if (context.IsEmpty() || m_frame != toFrameIfNotDetached(context))
> > +    V8DOMWindowShell::IsolatedContextData* isolatedContextData = V8DOMWindowShell::enteredIsolatedContextData();
> > +    if (UNLIKELY(!!isolatedContextData)) {
> > +        V8DOMWindowShell* isolatedShell = existingWindowShellInternal(isolatedContextData->world());
> > +        // A temporary isolated world has been deleted, so use the current context.
> > +        if (UNLIKELY(!isolatedShell)) {
> > +            v8::Handle<v8::Context> context = v8::Context::GetEntered();
> > +            if (m_frame != toFrameIfNotDetached(context))
> > +                return v8::Local<v8::Context>();
> > +            return v8::Local<v8::Context>::New(context);
> > +        }
> > +        // The shell exists, but potentially it has a new context, so use it.
> > +        if (isolatedShell->context().IsEmpty() || m_frame != toFrameIfNotDetached(isolatedShell->context()))
> >              return v8::Local<v8::Context>();
> > -        return v8::Local<v8::Context>::New(context);
> > +        return v8::Local<v8::Context>::New(isolatedShell->context());
> >      }
> 
> This function is getting way too complicated.  What's causing this complexity?  Perhaps we need to remove the concept of a temporary isolated world?  Maybe we need to treat temporary isolated worlds more like regular isolated worlds?

I think ultimately this function needs some refactoring.  It's odd to want to retrieve the entered context and then not use it, but instead use the context from the same world that it currently attached to the frame.  I suspect there are two distinct use cases for this function, and we are using it for both.  I also don't fully understand why the main world case gets initialized if it's empty and the isolated world case doesn't.

Anyway, that function is slated for a major reworking anyway to fix bug 93646, so I'd like to hold off until then if this patch stops hitting assertions in worker threads.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.