348062 - Crash [@ nsArraySH::GetProperty] with stirtable and removestyles stuff and removing iframe

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Debug code

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.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

I checked the debug code in.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> +#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?

Comment 7

[Daniel Veditz [:dveditz]]

If this is assigned to "[e-mail]" (i.e. nobody) can we really be blocking 1.8.1 and 1.8.0.7?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 9

[Johnny Stenback (:jst)]

Attachment: Make nsContentList hold strong references to the nodes in the list.

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Johnny Stenback (:jst)]

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

Comment 14

[Johnny Stenback (:jst)]

Fix checked in on the trunk. Marking FIXED.

Comment 15

[Daniel Veditz [:dveditz]]

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.

Comment 16

[Johnny Stenback (:jst)]

Attachment: Branch version of the above.

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.

Comment 17

[Johnny Stenback (:jst)]

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.

Comment 18

[Mike Schroepfer]

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.

Comment 19

[Johnny Stenback (:jst)]

Fix checked in on both branches.

Comment 20

[Jay Patel [:jay]]

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

Comment 21

[Daniel Veditz [:dveditz]]

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.