[webkit-dev] Early deletion of DocumentLoader instances

[Darin Adler]

On May 24, 2011, at 6:56 AM, Raphael Kubo da Costa wrote:

> Darin Adler <darin at apple.com> 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