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 put this at the top of my TODO list for Friday. (Unfortunately, I'm at the W2SP workshop tomorrow and can't fix the issue immediately.) Thanks, Adam _______________________________________________ webkit-dev mailing list [email protected] http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
