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)?
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
