Blake Kaplan (:mrbkap) (inactive)
	Description • 18 years ago

This is some debug code that sicking and I used to track down the problem. I'm hoping that he'll comment on what we found.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 1 • 18 years ago

Comment on attachment 233014 [details] [diff] [review]
Debug code

Could you add an ASSERT_IN_SYNC to nsContentList::PopulateSelf after the call to PopulateWithStartingAfter

>+nsContentList::AssertInSync()
>+{
>+  if (mState == LIST_DIRTY) {
>+    return;
>+  }
>+
>+  if (!mRootNode) {
>+    NS_ASSERTION(mElements.Count() == 0 && mState == LIST_DIRTY,
>+                 "Empty iterator isn't quite empty?");
>+    return;
>+  }
>+
>+  nsIContent *root;
>+  if (mRootNode->IsNodeOfType(nsINode::eDOCUMENT)) {
>+    root = NS_STATIC_CAST(nsIDocument*, mRootNode)->GetRootContent();
>+  }
>+  else {
>+    root = NS_STATIC_CAST(nsIContent*, mRootNode);
>+  }

Please add an XXX comment here indicating that we need to change this code if we ever make contentlists be able to match nodes outside of the documentElement.

r/sr=sicking

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 2 • 18 years ago

Here is what happens:

1. We create an nsContentList by calling document.getElementByTagName('tr') and
   populate it by iterating through its items.
2. We call reload() on the current document which makes nsDocument::Destroy get
   called.
3. nsDocument::Destroy calls UnbindFromTree on all its children making them null
   out their parent pointer.
4. We remove a <tr> element from the document and then cause a GC which causes
   the element to get destroyed.
5. We iterate through the nsContentList.

The problem is that in step 4 we don't send out any nsIMutationObserver notifications to the nsContentList since the elements think they were removed from the document in step 3.

However since we in 3 didn't send out any nsIMutationObserver notifications the nsContentList still holds a pointer to the <tr>. So the now deleted <tr> is returned from the content list and we crash.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 3 • 18 years ago

I checked the debug code in.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 18 years ago

> +#else
> +#define ASSERT_IN_SYNC
> +#endif

This means an ASSERT_IN_SYNC  (no trailing ';') will compile in all builds but DEBUG_CONTENT LIST builds.  How about:

  #define ASSERT_IN_SYNC PR_BEGIN_MACRO PR_END_MACRO

or something similar that the compiler can then optimize away?

	Daniel Veditz [:dveditz]
	Comment 5 • 18 years ago

If this is assigned to "general@dom.bugs" (i.e. nobody) can we really be blocking 1.8.1 and 1.8.0.7?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 6 • 18 years ago

(In reply to comment #6)
> This means an ASSERT_IN_SYNC  (no trailing ';') will compile in all builds but
> DEBUG_CONTENT LIST builds.  How about:

I just checked this fix into trunk.

	Johnny Stenback (:jst) Assignee
	Comment 7 • 18 years ago

This fixes this bug by making nsContentList hold strong references to the nodes in the list. While a bit more expensive than the code we've got now, it's for sure safer, and AFAICT it's leak-safe too.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 18 years ago

Comment on attachment 235330 [details] [diff] [review]
Make nsContentList hold strong references to the nodes in the list.

>diff --git a/content/base/src/nsContentList.cpp 

>+nsContentList::OnDocumentDestroy(nsIDocument *aDocument)

Shouldn't we call this from somewhere too?

r+sr=bzbarsky with that.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 9 • 18 years ago

If we do this (which i'm not sure we can), we need to hook it up to graydons cycle collector to avoid leaks if someone sets a list as an expando property on an element.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 18 years ago

jst and I talked about that, actually.  That should still be OK, since the list holds refs to nsIContent nodes, not JSObjects of any sort.  So it can't cause cycles through JS/XPConnect.  And a list will never holds refs to nodes that might hold refs to it.

	Johnny Stenback (:jst) Assignee
	Comment 11 • 18 years ago

Duh, yes. I put that at the end of nsDocument::Destroy().

	Johnny Stenback (:jst) Assignee
	Comment 12 • 18 years ago

Fix checked in on the trunk. Marking FIXED.

	Daniel Veditz [:dveditz]
	Comment 13 • 18 years ago

Comment on attachment 235330 [details] [diff] [review]
Make nsContentList hold strong references to the nodes in the list.

approved for 1.8.0 branch, a=dveditz for drivers

Code freeze is today (+weekend), please land ASAP.

	Johnny Stenback (:jst) Assignee
	Comment 14 • 18 years ago

This is the same code ported to the branches, there's a couple of differences that made the trunk patch not apply cleanly, but it was a trivial merge so essentially these changes are identical. Some names differ, and the nsCOMArray changes are not relevant for the branches.

	Johnny Stenback (:jst) Assignee
	Comment 15 • 18 years ago

Oh, and the branch patch does fix the crash on both branches, but on the 1.8.0 branch there's another crash (in nsCellMap::GetCellInfoAt()) that'll make this bug harder to verify there.

	Mike Schroepfer
	Comment 16 • 18 years ago

Comment on attachment 235330 [details] [diff] [review]
Make nsContentList hold strong references to the nodes in the list.

a=schrep approving all previously triaged patches which were baking on trunk.

	Johnny Stenback (:jst) Assignee
	Comment 17 • 18 years ago

Fix checked in on both branches.

	Jay Patel [:jay]
	Comment 18 • 18 years ago

v.fixed on both branches, no crash with testcase (did not see crash with 1.8.0 that jst mentions in comment #17):
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060829 Firefox/1.5.0.7  
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060829 BonEcho/2.0b2

	Daniel Veditz [:dveditz]
	Comment 19 • 18 years ago

This bug lost its 1.8.0.8 nomination in a bugzilla mixup, but I think it was a stale nomination not cleared when this made it into 1.8.0.7 after all.