The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.8.1

Status

()

Core
DOM
--
critical
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: jst)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla1.8.1
x86
Windows XP
crash, testcase, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][baking until 8/27], crash signature)

Attachments

(3 attachments)

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

Updated

11 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.7?
Flags: blocking1.8.0.7? → blocking1.8.0.7+

Updated

11 years ago
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+
(Assignee)

Comment 9

11 years ago
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.
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.
(Assignee)

Comment 13

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

Comment 14

11 years ago
Fix checked in on the trunk. Marking FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
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?
(Assignee)

Comment 16

11 years ago
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.
(Assignee)

Comment 17

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

Updated

11 years ago
Whiteboard: [baking until 8/27]

Comment 18

11 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.
Attachment #235330 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 19

11 years ago
Fix checked in on both branches.
Keywords: fixed1.8.0.7, fixed1.8.1

Comment 20

11 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
Keywords: fixed1.8.0.7, fixed1.8.1 → verified1.8.0.7, verified1.8.1
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
(Assignee)

Updated

9 years ago
Attachment #235330 - Flags: approval1.7.14?
Flags: in-testsuite?
Crash Signature: [@ nsArraySH::GetProperty]
You need to log in before you can comment on or make changes to this bug.