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. ~Brady > > -- Darin > > _______________________________________________ > webkit-dev mailing list > [email protected] > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev _______________________________________________ webkit-dev mailing list [email protected] http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
