On May 24, 2011, at 6:56 AM, Raphael Kubo da Costa wrote:
> Darin Adler <[email protected]> writes:
>
>> Also, there is a Document::setDocumentLoader function, but nobody ever calls
>> it.
>
> I could submit a patch to remove it, if that's desirable.
Yes, we do want to remove it.
>> What we need are some test cases showing problems caused by this
>> mistake that we can use as regression tests;
>
> Hmm, I wonder how to write a test for this. I've experienced this crash while
> writing some DRT code for the EFL port. In short, I reuse the same Frame and
> Frame::loader() to load each layout test page, and the code will occasionally
> crash when some page's contents are delivered. Should I try to create a
> manual-test for WebCore?
Any kind of test would be helpful. Further study about why it crashed the times
you saw a crash might lead to some inspiration about how to make a test that
does this reliably on demand. Another things that might make the crash more
common would be some technique to more often detect when we tried to use the
deleted object. For example:
ASSERT(!m_documentLoader->deletionHasBegun());
Putting that in some document functions that are going to use the document
loader could help us detect the problem in cases where otherwise the code might
work.
>> we should fix it by making some better relationship between the Document and
>> DocumentLoader that guarantees we won’t have a dangling pointer. Either
>> reference counting to keep the object alive, or code to zero out the pointer
>> at some point before the object is deleted.
>
> r80507 added a check for m_frame before using it (Document also has a raw
> Frame pointer). Perhaps the same should be done here?
That fix is not a good quality one. It’s a fragile approach. The relationship
of a document to its frame is that the association between the two is
explicitly broken by the detach function in document. But it’s not clear this
is the right way to break the association with a document loader. Here are
three specific points:
1) It’s poor design that the document grabs and keeps a pointer to the
document loader in its constructor. The document is not in a position to
monitor the lifetime of the loader. It would be far more maintainable if the
same code/class both set up and maintained the pointer.
2) It's not clear that detach time is the right moment to invalidate the
pointer from a document to its document loader. It would be better to study the
lifetime of document loader and loading process to get a clearer idea of
exactly what the right time is and what the best relationship between these
objects is.
3) Keeping a dangling m_documentLoader pointer around with no guarantee
that it points to a live object is a bad design pattern. If the loader is no
longer valid when the document is not associated with a frame, then right way
to deal with that is to zero out m_documentLoader in the detach function, not
to check m_frame each time before checking m_documentLoader.
This fragile design was introduced just three months ago in
<http://trac.webkit.org/changeset/78342>. It might be worth asking Nate Chapin
or Adam Barth if they had some plans for further refinement. They may have
overlooked these design mistakes, but it’s likely they have future plans that
will rectify this.
Maybe we could continue this discussion in a bug report?
-- Darin
_______________________________________________
webkit-dev mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
