Closed Bug 348062 Opened 15 years ago Closed 15 years ago

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

Categories

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

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.8.1

People

(Reporter: martijn.martijn, Assigned: jst)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [sg:critical?][baking until 8/27])

Crash Data

Attachments

(3 files)

Attached patch Debug codeSplinter Review
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.
Attachment #233014 - Flags: superreview?(bugmail)
Attachment #233014 - Flags: review?(bugmail)
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
Attachment #233014 - Flags: superreview?(bugmail)
Attachment #233014 - Flags: superreview+
Attachment #233014 - Flags: review?(bugmail)
Attachment #233014 - Flags: review+
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.
Depends on: 348156
I checked the debug code in.
> +#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?
Flags: blocking1.9?
Flags: blocking1.8.1?
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.7?
Flags: blocking1.8.0.7? → blocking1.8.0.7+
Target Milestone: --- → mozilla1.8.1
If this is assigned to "general@dom.bugs" (i.e. nobody) can we really be blocking 1.8.1 and 1.8.0.7?
(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.
Flags: blocking1.8.0.8?
Flags: blocking1.8.0.7-
Flags: blocking1.8.0.7+
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.
Assignee: general → jst
Status: NEW → ASSIGNED
Attachment #235330 - Flags: superreview?(bzbarsky)
Attachment #235330 - Flags: review?(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.
Attachment #235330 - Flags: superreview?(bzbarsky)
Attachment #235330 - Flags: superreview+
Attachment #235330 - Flags: review?(bzbarsky)
Attachment #235330 - Flags: review+
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.
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.
Duh, yes. I put that at the end of nsDocument::Destroy().
Fix checked in on the trunk. Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #235330 - Flags: approval1.8.1?
Attachment #235330 - Flags: approval1.8.0.8?
Attachment #235330 - Flags: approval1.8.0.7?
Attachment #235330 - Flags: approval1.7.14?
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.
Attachment #235330 - Flags: approval1.8.0.8?
Attachment #235330 - Flags: approval1.8.0.7?
Attachment #235330 - Flags: approval1.8.0.7+
Flags: blocking1.8.0.7-
Flags: blocking1.8.0.7+
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
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.
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.
Whiteboard: [baking until 8/27]
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.
Attachment #235330 - Flags: approval1.8.1? → approval1.8.1+
Fix checked in on both branches.
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
Whiteboard: [baking until 8/27] → [sg:critical?][baking until 8/27]
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.
Depends on: 354711
Flags: blocking1.9?
Group: security
Attachment #235330 - Flags: approval1.7.14?
Flags: in-testsuite?
Crash Signature: [@ nsArraySH::GetProperty]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.