On Sat, Nov 21, 2009 at 9:40 PM, Drew Wilson <atwil...@chromium.org> wrote:
> Can anyone explain (or point me at some docs) for how the
> gcPrologue/gcEpilogue stuff should work, wrt the state of the underlying
> handles (what assumptions does that code make)? I guess I don't really
> understand when objects are taken out of the DOMMap. Let's say I have an
> unentangled MessagePort (so, its state is WEAK, and we don't change this in
> gcPrologue) - if garbage collection decides that it can be freed, is it
> actually freed and removed from the DOMMap at that time (so it won't be
> iterated over in gcEpilogue)?
The order of operations here is:
1. GC prologue.
2. Actual GC. Here we determine if an object is alive only because
weak handles reference it. If this is the case, such handles become
"pending".
3. Post GC processing. Here we invoke callbacks on pending weak
handles allowing the binding layer to dispose associated native
objects and the handles. (Note: JS objects are still alive here. They
will be collected on the next GC cycle since then there will be no
handles referencing them.)
4. GC epilogue.
So stuff in weak callbacks happens before the epilogue. But it is also
possible that a callback triggers another (nested) GC before post
processing (and epilogue) for the current one is done.
I haven't really looked into MessagePort problem, but my gut feeling
is that in general we should avoid manipulating handle states in
prologue/epilogue and prefer creating extra links in JS instead.
-- Vitaly
> I'm trying to make sure that if a MessagePort is determined to be
> unreferenced, I don't somehow resurrect it by incorrectly changing its state
> back to WEAK.
> -atw
>
> On Fri, Nov 20, 2009 at 7:41 PM, Adam Barth <aba...@chromium.org> wrote:
>>
>> 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
