Last Comment Bug 348062 - Crash [@ nsArraySH::GetProperty] with stirtable and removestyles stuff and removing iframe
: Crash [@ nsArraySH::GetProperty] with stirtable and removestyles stuff and re...
Status: RESOLVED FIXED
[sg:critical?][baking until 8/27]
: crash, testcase, verified1.8.0.7, verified1.8.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: mozilla1.8.1
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
: Hixie (not reading bugmail)
Mentors:
Depends on: 348156 354711
Blocks: stirtable
  Show dependency treegraph
 
Reported: 2006-08-09 10:44 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2009-04-02 22:15 PDT (History)
9 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
mtschrep: blocking1.8.1+
dveditz: blocking1.8.0.7+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Debug code (7.56 KB, patch)
2006-08-09 18:02 PDT, Blake Kaplan (:mrbkap)
jonas: review+
jonas: superreview+
Details | Diff | Splinter Review
Make nsContentList hold strong references to the nodes in the list. (6.89 KB, patch)
2006-08-24 16:53 PDT, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.0.7+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review
Branch version of the above. (6.83 KB, patch)
2006-08-25 16:33 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review

Comment 2 Blake Kaplan (:mrbkap) 2006-08-09 18:02:45 PDT
Created attachment 233014 [details] [diff] [review]
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 2006-08-09 20:39:47 PDT
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 2006-08-09 20:58:28 PDT
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) 2006-08-10 11:58:48 PDT
I checked the debug code in.
Comment 6 Boris Zbarsky [:bz] 2006-08-10 19:45:12 PDT
> +#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] 2006-08-16 15:06:55 PDT
If this is assigned to "general@dom.bugs" (i.e. nobody) can we really be blocking 1.8.1 and 1.8.0.7?
Comment 8 Blake Kaplan (:mrbkap) 2006-08-18 16:25:29 PDT
(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, jst@mozilla.com) 2006-08-24 16:53:17 PDT
Created attachment 235330 [details] [diff] [review]
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 [:bz] 2006-08-24 20:08:20 PDT
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 2006-08-24 20:10:17 PDT
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 [:bz] 2006-08-24 20:14:58 PDT
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, jst@mozilla.com) 2006-08-25 10:06:46 PDT
Duh, yes. I put that at the end of nsDocument::Destroy().
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2006-08-25 10:47:16 PDT
Fix checked in on the trunk. Marking FIXED.
Comment 15 Daniel Veditz [:dveditz] 2006-08-25 11:41:44 PDT
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, jst@mozilla.com) 2006-08-25 16:33:59 PDT
Created attachment 235493 [details] [diff] [review]
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, jst@mozilla.com) 2006-08-25 16:37:00 PDT
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 2006-08-27 09:59:03 PDT
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, jst@mozilla.com) 2006-08-28 15:08:53 PDT
Fix checked in on both branches.
Comment 20 Jay Patel [:jay] 2006-08-30 17:02:21 PDT
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] 2006-09-19 15:47:26 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.