Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dangling pointer in Element->GetOwnerDocument() #285

Open
menne386 opened this issue Jun 24, 2016 · 4 comments
Open

dangling pointer in Element->GetOwnerDocument() #285

menne386 opened this issue Jun 24, 2016 · 4 comments

Comments

@menne386
Copy link

menne386 commented Jun 24, 2016

Experienced crash when clicking mouse and holding mouse button, then unloading the document then releasing mouse.

Traced it to Element->GetOwnerDocument() owner_document is cached for child elements, but when that document is gone, you have a dangling pointer in the child.
(When you click mouse element is placed in list inside Context.cpp, when doc unloads element survives as refcount in increased, but owner_document is not reset, while document itself is deleted.)

My version of Element->GetOwnerDocument():

// Gets the document this element belongs to.
ElementDocument* Element::GetOwnerDocument()
{
if (owner_document) {
return owner_document;
}
return parent? parent->GetOwnerDocument():nullptr;
}

Another solution could be to reset owner_document variable of all children when a document unloads.

@viciious
Copy link
Contributor

viciious commented Aug 26, 2016

Doesn't that actually mean that something's holding a pointer to your DOM element? It should have been garbage collected by the time the owner document was about to get destroyed..

@viciious
Copy link
Contributor

Sorry, missed the original explanation in the parenthesis. Imo the proper solution would be to also destroy child elements placed in the context when/if the parent document gets destroyed.

@menne386
Copy link
Author

Works too..

In essence owner_document pointer is like a non-owning reference.
The reference_countable class does not provide an implementation/protection for that...

@viciious
Copy link
Contributor

This is probably related too #276

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants