I "wrote" most of this code in the sense that I took the old garbage
collector design and refactored it to make isolated worlds possible.
I don't understand the MessagePorts lifetime issues in detail,
however.
Adam
On Fri, Nov 20, 2009 at 7:02 PM, Drew Wilson <atwil...@chromium.org> wrote:
> So I looked into the problem further - the issue was that calling close()
> would still leave MessagePorts in a seemingly entangled state, which meant
> that they would never be freed (turns out this caused some related issues on
> the WebKit side related to delaying worker exit).
> I updated the code so that MessagePorts now are disentangled when they are
> close()-d, and now I'm getting a failed assertion in the V8GCPrologue code
> in the WebKit v8 bindings.
> Here's the sequence of events:
> 1) I hit reload on a page - this forces a GC to clean up the old context and
> related objects.
> 2) GCPrologueVisitor is called by our GCPrologueCallback for a set of
> objects. It has code that looks like this:
> void visitDOMWrapper(void* object, v8::Persistent<v8::Object> wrapper)
> {
> ASSERT(wrapper.IsWeak());
> V8ClassIndex::V8WrapperType type =
> V8DOMWrapper::domWrapperType(wrapper);
> ...
> if (type == V8ClassIndex::MESSAGEPORT) {
> MessagePort* port1 = static_cast<MessagePort*>(object);
> if (port1->isEntangled())
> wrapper.ClearWeak();
> ...
> }
> The end result is that if MessagePort::isEntangled() returns true, the
> MessagePort has its Weak flag cleared, and is not garbage collected.
> 3) GC happens
> 4) After garbage collection, GCEpilogueVisitor is called via our
> GCEpilogueCallback. It walks through all the various nodes, and does this:
> void visitDOMWrapper(void* object, v8::Persistent<v8::Object> wrapper)
> {
> V8ClassIndex::V8WrapperType type =
> V8DOMWrapper::domWrapperType(wrapper);
> ....
> if (type == V8ClassIndex::MESSAGEPORT) {
> MessagePort* port1 = static_cast<MessagePort*>(object);
> if (port1->isEntangled()) {
> // We marked this port as reachable in GCPrologueVisitor.
> Undo this now since the
> // port could be not reachable in the future if it gets
> disentangled (and also
> // GCPrologueVisitor expects to see all handles marked as
> weak).
> wrapper.MakeWeak(port1,
> &DOMDataStore::weakActiveDOMObjectCallback);
> }
> ...
> }
> Looks hunky-dory, except... with my latest change, isEntangled() can now
> (correctly) change state when close() is called. What happens in step 3 is
> garbage collection causes the ScriptExecutionContext (Document object) for
> the old page to get freed. As part of this, MessagePort::close() is invoked
> on all of the message ports associated with that context. So they are
> entangled in step 2, so ClearWeak is called on them, but they *aren't*
> entangled in step 4, so MarkWeak is no longer called. The next time we do a
> GC, we die due to the ASSERT(wrapper->IsWeak()) in GCPrologueVisitor.
> So I'm not sure exactly what the right fix is here, and I'm hoping that
> perhaps someone who wrote this code initially could chime in - I'm thinking
> that we should just call MarkWeak on all MessagePorts in GCEpilogue (since
> GCEpilogue wouldn't be called on them if they weren't supposed to be weak),
> but I'm not sure if that will have any adverse side-effects if there are
> references. For example, if I do this:
> var p = new MessageChannel().port1;
> p.close();
> I'd still like that MessagePort to hang around as long as there's a
> reachable reference (i.e. "p").
> Any thoughts from someone more familiar with this code?
> -atw
>
> On Thu, Nov 19, 2009 at 11:07 PM, Mads Ager <a...@google.com> wrote:
>>
>> Let us know what you find Drew. I'll be happy to help figure this out as
>> well.
>>
>> -- Mads
>>
>> On Thu, Nov 19, 2009 at 11:46 PM, Drew Wilson <atwil...@chromium.org>
>> wrote:
>> > There's no bug on this yet - I ran across this when I ran out of memory
>> > in
>> > my SharedWorkers soak test I was running (turns out that this bug may be
>> > unrelated to the SharedWorkers bug).
>> > It's quite possible that this is related to the V8GCController code -
>> > I'd
>> > forgotten about that. In fact, it's quite possible that the true cause
>> > is
>> > "ports are not getting closed when their parent document exits", in
>> > which
>> > case leaking the port is the expected behavior. I'll look at this a bit
>> > more
>> > - thanks for the pointer.
>> > -atw
>> >
>> > On Thu, Nov 19, 2009 at 2:35 PM, Vitaly Repeshko <vita...@chromium.org>
>> > wrote:
>> >>
>> >> On Fri, Nov 20, 2009 at 1:20 AM, Drew Wilson <atwil...@chromium.org>
>> >> wrote:
>> >> > I'm investigating a leak in the MessagePort/MessageChannel code.
>> >> > Basically,
>> >> > if I have a page that looks like this:
>> >> > <script>
>> >> > new MessageChannel();
>> >> > </script>
>> >> > We leak two MessagePorts every time we reload the page.
>> >> > The WebCore::MessageChannel impl object has two RefPtrs to two
>> >> > MessagePort
>> >> > objects. When I reload the page, the MessageChannel DOMWrapper V8
>> >> > object
>> >> > is
>> >> > GC'd, leading to the MessageChannel impl object getting deref'd and
>> >> > freed.
>> >> > The problem is that the V8 MessageChannel constructor has this code:
>> >> >
>> >> > // Create references from the MessageChannel wrapper to the two
>> >> > // MessagePort wrappers to make sure that the MessagePort
>> >> > wrappers
>> >> > // stay alive as long as the MessageChannel wrapper is around.
>> >> > messageChannel->SetInternalField(kMessageChannelPort1Index,
>> >> > V8DOMWrapper::convertToV8Object(V8ClassIndex::MESSAGEPORT,
>> >> > obj->port1()));
>> >> > messageChannel->SetInternalField(kMessageChannelPort2Index,
>> >> > V8DOMWrapper::convertToV8Object(V8ClassIndex::MESSAGEPORT,
>> >> > obj->port2()));
>> >> > // Setup the standard wrapper object internal fields.
>> >> > V8DOMWrapper::setDOMWrapper(messageChannel,
>> >> > V8ClassIndex::MESSAGECHANNEL, obj.get());
>> >> > Those two MessagePort DOMWrappers don't seem to get freed, so they
>> >> > hold
>> >> > a
>> >> > reference to the underlying MessagePort impl objects.
>> >> > There are a few things I don't understand here:
>> >> > 1) I don't get why we need to explicitly keep a special reference to
>> >> > the
>> >> > MessagePorts here in internal fields - we don't seem to do that for
>> >> > other
>> >> > DOM Wrappers that have references like this (e.g. SharedWorker.port).
>> >>
>> >> There are a few corner-cases where we have to create additional links
>> >> in JS world to prevent things from being collected. Another example of
>> >> this are hidden dependencies that are used to keep event listeners on
>> >> DOM objects alive. IIRC, there is another part of this in
>> >> V8GCController dealing with message ports. Perhaps these two parts
>> >> conflict.
>> >>
>> >> > 2) Is there a good way to figure out why the garbage collector isn't
>> >> > collecting these references also, short of stepping through the V8 GC
>> >> > code?
>> >> > How do people typically track this stuff?
>> >> > Any pointers would be much appreciated.
>> >>
>> >> You could try using DevTools heap profiler. Alternatively in debug
>> >> mode there is TracePathToObject() (see
>> >>
>> >>
>> >> http://code.google.com/p/v8/source/browse/branches/bleeding_edge/src/heap.cc#3853).
>> >>
>> >> Is there a bug on this? I'm happy to have a look and fix it.
>> >>
>> >>
>> >> -- Vitaly
>> >
>> > --
>> > Chromium Developers mailing list: chromium-dev@googlegroups.com
>> > View archives, change email options, or unsubscribe:
>> > http://groups.google.com/group/chromium-dev
>
>
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
