On Thu, May 26, 2011 at 9:05 AM, Brady Eidson <[email protected]> wrote: > On May 25, 2011, at 21:30 , Adam Barth wrote: >> On Wed, May 25, 2011 at 6:26 PM, Brady Eidson <[email protected]> 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 _______________________________________________ webkit-dev mailing list [email protected] http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
