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

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

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

We should look into if this could cause crashes.

Comment 2

[Mounir Lamouri (:mounir)]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I do not think this could cause crash.

Even if someone expects to be able to getElementById() the element and get non-null?

Comment 4

[Mounir Lamouri (:mounir)]

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

Comment 5

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

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.

Comment 6

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

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.

Comment 7

[Jesse Ruderman]

fwiw, don't rely on the fuzzer to find leak bugs until bug 593541 and bug 551631 are fixed.

Comment 8

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

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

Comment 9

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

Actually, in step 1 you should also add such a .clear() call to NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsDocument)

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

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

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Comment 12

[Mounir Lamouri (:mounir)]

No leak on try with this patch.

Comment 13

[Mounir Lamouri (:mounir)]

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

Comment 14

[Peter Van der Beken [:peterv]]

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.

Comment 15

[Mounir Lamouri (:mounir)]

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

Comment 16

[Mounir Lamouri (:mounir)]

Pushed:
https://hg.mozilla.org/mozilla-central/rev/4772228b45e1