[webkit-dev] Early deletion of DocumentLoader instances

[Adam Barth]

On Thu, May 26, 2011 at 9:05 AM, Brady Eidson <beidson at apple.com> wrote:
> On May 25, 2011, at 21:30 , Adam Barth wrote:
>> On Wed, May 25, 2011 at 6:26 PM, Brady Eidson <beidson at apple.com> wrote:
>>> On May 24, 2011, at 09:31 , Darin Adler wrote:
>>>> On May 24, 2011, at 6:56 AM, Raphael Kubo da Costa wrote:
>>>>>> 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?
>>>
>>> We now have crash report data showing that this is hitting the Mac port in the field.
>>>
>>> I filed https://bugs.webkit.org/show_bug.cgi?id=61494
>>>
>>> At this point, I will be working to see how easy it is to simply roll out 78342 since it was only refactoring and not fixing any particular bug.
>>
>> Reading my comments on the bug, I was happy that the document had a
>> pointer to the DocumentLoader.  My apologies for misunderstanding the
>> ownership relations between these objects.  I thought that
>> DocumentLoader had Document-lifetime, but it appears that it has
>> neither Document-lifetime nor Frame-lifetime.
>>
>> The correct fix is likely to change the way to locate the writer() as follows:
>>
>> - document->loader()->writer()
>> + frame->loader()->activeDocumentLoader()->writer()
>
> I'll definitely explore this today, and it might help make some crashes go away.
>
> But the realization that DocumentLoaders have neither Frame nor Document lifetime lines up with Darin's comments on how the very design is fragile.  The fact that Document's *can* outlive their DocumentLoader and therefore can point to a garbage DocumentLoader is a greater design flaw that we need to fix.

Definitely.  We should remove the pointer from Document to
DocumentLoader.  Instead of finding the DocumentLoader via this
pointer, we should find it via Frame->FrameLoader->DocumentLoader.

The original reason why I was excited about changing to locating the
DocumentLoader via the Document was to avoid confusing where the Frame
now contains a different active DocumentLoader.  However, those
concerns are theoretical, unlike these crashes.

Adam