[Webkit-unassigned] [Bug 96522] Give isolated shells a lifecycle like that of main shells

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 12 11:41:16 PDT 2012


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





--- Comment #3 from Adam Barth <abarth at webkit.org>  2012-09-12 11:41:41 PST ---
(From update of attachment 163637)
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();

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


More information about the webkit-unassigned mailing list