Early deletion of DocumentLoader instances
Hey there, While working on the EFL port, I've noticed that sometimes a FrameLoader's DocumentLoader ends up being deleted too early (FrameLoader::setDocumentLoader causes the current DocumentLoader to be deref'ed and freed), in the sense that later on Document::explicitClose tries to access this DocumentLoader instance that has already been freed, causing a crash. I couldn't find any abnormal behaviour that could deref the DocumentLoaders more than they should have been. Are there any recommended places where I should look to check what's going on? -- Raphael Kubo da Costa ProFUSION embedded systems http://profusion.mobi
On May 23, 2011, at 1:34 PM, Raphael Kubo da Costa wrote:
While working on the EFL port, I've noticed that sometimes a FrameLoader's DocumentLoader ends up being deleted too early (FrameLoader::setDocumentLoader causes the current DocumentLoader to be deref'ed and freed), in the sense that later on Document::explicitClose tries to access this DocumentLoader instance that has already been freed, causing a crash.
This looks like a basic design problem. The document has a pointer to the document loader, and keeps that pointer even after the document loader has been destroyed. That is a broken design. Also, there is a Document::setDocumentLoader function, but nobody ever calls it. What we need are some test cases showing problems caused by this mistake that we can use as regression tests; then 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. -- Darin
Darin Adler <darin@apple.com> writes:
On May 23, 2011, at 1:34 PM, Raphael Kubo da Costa wrote:
While working on the EFL port, I've noticed that sometimes a FrameLoader's DocumentLoader ends up being deleted too early (FrameLoader::setDocumentLoader causes the current DocumentLoader to be deref'ed and freed), in the sense that later on Document::explicitClose tries to access this DocumentLoader instance that has already been freed, causing a crash.
This looks like a basic design problem. The document has a pointer to the document loader, and keeps that pointer even after the document loader has been destroyed. That is a broken design.
Also, there is a Document::setDocumentLoader function, but nobody ever calls it.
I could submit a patch to remove it, if that's desirable.
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?
then 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? -- Raphael Kubo da Costa ProFUSION embedded systems http://profusion.mobi
On May 24, 2011, at 6:56 AM, Raphael Kubo da Costa wrote:
Darin Adler <darin@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
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 webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Wed, May 25, 2011 at 6:26 PM, Brady Eidson <beidson@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 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
On May 25, 2011, at 9:30 PM, Adam Barth wrote:
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 for the suggestion. I think Brady can probably take a shot at this before Friday, but we'll keep you in the loop. Cheers, Maciej
On May 25, 2011, at 21:30 , Adam Barth wrote:
On Wed, May 25, 2011 at 6:26 PM, Brady Eidson <beidson@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. Thanks, ~Brady
On Thu, May 26, 2011 at 9:05 AM, Brady Eidson <beidson@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@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
On May 26, 2011, at 9:52 AM, Adam Barth wrote:
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.
Sure, that will be great eventually; I think a document loader and the document it’s loading should be able to find each other without passing through the frame. When we make the change we just have to get the ownership and object lifetimes right and avoid the mistake of adding a pointer without suitable object lifetime tracking. I think the issue with this patch is not that it was a step in the wrong direction exactly, but that the change wasn’t careful enough about object lifetime. Even if the document loader is at some point changed to have “document-lifetime” roughly speaking, I still don’t think adding a raw pointer to it in the document is likely to be right for teardown and other such edge cases. -- Darin
It looks like this was resolved in https://bugs.webkit.org/show_bug.cgi?id=61494, which resulted in http://trac.webkit.org/changeset/87756. I can't access the bug, so I'm following up here ... This change is causing problems on Android. We use the AllowLocalLoadsForLocalAndSubstituteData policy but Document::initSecurityContext() no longer calls securityOrigin().grantLoadLocalResources() because of the check for 'm_frame->document() == this' added by this change in DocumentLoader::loader(). In DocumentWriter::begin(), we create a new Document, and the Document constructor calls Document::initSecurityContext(). However, at this point the Frame has not yet been updated with the new Document, so we fail the new check in DocumentLoader::loader(). However, it looks like the Frame does have the correct DocumentLoader at this point. So it seems wrong for Document::loader() to return 0 and seems that the new check isn't valid in all cases? Or am I missing something? Any advice would be greatly appreciated. Thanks, Steve -- Google UK Limited Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W 9TQ Registered in England Number: 3977902
On Sep 22, 2011, at 11:07 AM, Steve Block wrote:
This change is causing problems on Android. We use the AllowLocalLoadsForLocalAndSubstituteData policy but Document::initSecurityContext() no longer calls securityOrigin().grantLoadLocalResources() because of the check for 'm_frame->document() == this' added by this change in DocumentLoader::loader().
I tried to look into this, but couldn’t find a function named DocumentLoader::loader. -- Darin
On Thu, Sep 22, 2011 at 3:07 PM, Darin Adler <darin@apple.com> wrote:
On Sep 22, 2011, at 11:07 AM, Steve Block wrote:
This change is causing problems on Android. We use the AllowLocalLoadsForLocalAndSubstituteData policy but Document::initSecurityContext() no longer calls securityOrigin().grantLoadLocalResources() because of the check for 'm_frame->document() == this' added by this change in DocumentLoader::loader().
I tried to look into this, but couldn’t find a function named DocumentLoader::loader.
Maybe he means Document::loader? http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.h#L583 Adam
participants (6)
-
Adam Barth
-
Brady Eidson
-
Darin Adler
-
Maciej Stachowiak
-
Raphael Kubo da Costa
-
Steve Block