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

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Dan Carney <dcarney at google.com>  2012-09-12 12:04:33 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
>> +    }
> 
> 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.

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