Closed Bug 593302 Opened 9 years ago Closed 9 years ago

"ASSERTION: Removing id entry that doesn't exist" after moving nodes between documents

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: mounir)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(2 files, 1 obsolete file)

Attached file testcase
###!!! ASSERTION: Removing id entry that doesn't exist: '!aElement->GetOwnerDoc() || !aElement->GetOwnerDoc()->IsHTML() || mIdContentList.IndexOf(aElement) >= 0', file content/base/src/nsDocument.cpp, line 421
We should look into if this could cause crashes.
Assignee: nobody → mounir.lamouri
blocking2.0: --- → final+
Blocks: 594645
I do not think this could cause crash.
The issue is aElement isn't in mIdContentList. However, I'm not sure why we happen to be in this situation...
> I do not think this could cause crash.

Even if someone expects to be able to getElementById() the element and get non-null?
Ok, I found the origin of this assertion. When the frame is removed from the document, the nsDocument::Destroy will be called on the frame's document. nsDocument::Destroy is clearing the id content list making the document forgetting the first added span.

It should be safe to not clear the list when ::Destroy is called because nsDocument destructor will clear the list. However, Jonas think that might cause leaks...
Status: NEW → ASSIGNED
Hardware: x86_64 → All
Yeah, that call is there specifically to prevent leaks, so if we remove it we need to be sure it won't cause leaks.

The good news is that the id-lists no longer hold strong references, so we definitely don't need to clear those.

The name list currently uses strong pointers since it's using nsBaseContentList. That would be a bigger patch to change, but might be worth it to avoid the refcounting?

That would leave the doc-all list and mImageElement.
Thinking some more about it. The name-list and the doc-all list should both be very reliably only hold references to things that are real children of the document, and so only hold references to things that we're already holding references to.

This leaves the mImageElement member. So we can either just clear out that, or add some other mechanism to ensure that that never creates leaks.

Though looking at that API, there really isn't anything to prevent you from adding an element which is in a parent document or otherwise is holding the document alive. Thus it is very prone to causing leak-inducing cycles.

Ideally the cycle collector should catch those, but if we don't want to rely on that we can just clear only those references out here.
fwiw, don't rely on the fuzzer to find leak bugs until bug 593541 and bug 551631 are fixed.
So worked out this plan with peterv:

1. Remove the .clear call
2. Add a testcase which contains an iframe. Let the document *inside* the iframe use MozSetImageElement to create a cycle, using something like document.MozSetImageElement("foo", window.frameElement);
3. Push to try and look for leaks

ideally also:

4. Remove the remove the traverse-call to mImageElement in nsIdentifierMapEntry::Traverse
5. Push to try and make sure there *are* leaks
Actually, in step 1 you should also add such a .clear() call to NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsDocument)
Status: ASSIGNED → NEW
Attached patch Patch v1 (obsolete) — Splinter Review
This is following recommandations from comments 8 and 9.
Locally, just running the crashtests I got no leaks in both situations. Though, patches have been sent to the try server...
Attachment #479695 - Flags: review?(peterv)
Status: NEW → ASSIGNED
Attached patch Patch v1.1Splinter Review
Attachment #479695 - Attachment is obsolete: true
Attachment #479715 - Flags: review?(peterv)
Attachment #479695 - Flags: review?(peterv)
No leak on try with this patch.
Whiteboard: [needs review]
(In reply to comment #8)
> 4. Remove the remove the traverse-call to mImageElement in
> nsIdentifierMapEntry::Traverse

I had a quick look on that again and it looks like the code you asked to be commented is never called, so commenting it doesn't change anything.

I will have a deeper look on that. In the mean time, Peter and Jonas, let me know if you have any idea.
OS: Linux → All
Comment on attachment 479715 [details] [diff] [review]
Patch v1.1

(In reply to comment #13)
> (In reply to comment #8)
> > 4. Remove the remove the traverse-call to mImageElement in
> > nsIdentifierMapEntry::Traverse
> 
> I had a quick look on that again and it looks like the code you asked to be
> commented is never called, so commenting it doesn't change anything.

I ended up debugging this myself :-/. DestroyContent saves us here for content that's in the document. Please replace the iframe in 593302-2.html with

    <script>
      var elem = document.createElement('span');
      document.mozSetImageElement('foo', elem);
      elem.foo = document;
    </script>

That does leak if you remove the traverse call.
Attachment #479715 - Flags: review?(peterv) → review+
(In reply to comment #14)
> Comment on attachment 479715 [details] [diff] [review]
> Patch v1.1
> 
> (In reply to comment #13)
> > (In reply to comment #8)
> > > 4. Remove the remove the traverse-call to mImageElement in
> > > nsIdentifierMapEntry::Traverse
> > 
> > I had a quick look on that again and it looks like the code you asked to be
> > commented is never called, so commenting it doesn't change anything.
> 
> I ended up debugging this myself :-/.

Really sorry about that. I was planning to talk to you about this bug during the All Hands.

> DestroyContent saves us here for content
> that's in the document. Please replace the iframe in 593302-2.html with
> 
>     <script>
>       var elem = document.createElement('span');
>       document.mozSetImageElement('foo', elem);
>       elem.foo = document;
>     </script>
> 
> That does leak if you remove the traverse call.

Thanks!
Whiteboard: [needs review] → [needs-try]
Pushed:
https://hg.mozilla.org/mozilla-central/rev/4772228b45e1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs-try]
Target Milestone: --- → mozilla2.0b9
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.