Open Bug 1162765 Opened 5 years ago Updated 9 months ago

Convert nsGenericHTMLFormElement::mForm to a strong reference

Categories

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

defect

Tracking

()

People

(Reporter: ehsan, Unassigned, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

We have a cycle collector now, so this should be fine!
Attachment #8603157 - Flags: review?(continuation) → review+
Comment on attachment 8603159 [details] [diff] [review]
Part 2: Convert nsGenericHTMLFormElement::mFieldSet to a strong reference

Review of attachment 8603159 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like this mDependentElements could be strong now, too.  Please at least file a bug for that if you think that makes sense.
Attachment #8603159 - Flags: review?(continuation) → review+
Filed bug 1163148 as a follow-up.
The cause of these assertions is this code: <https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#4564>.  Here we look at the element's type and mForm to determine whether to remove it from the radio group.  The problem is that after the node gets unlinked, mForm will be null, and we have no way of knowing whether it was previously assigned to or not.

Andrew, can you think of a clean way to check whether a node has been unlinked during UnbindFromTree?  I can't think of a much cleaner way to fix this except for maintaining a flag in nsGenericHTMLFormElement indicating whether mForm has ever been assigned to, which sucks... :/
Flags: needinfo?(ehsan) → needinfo?(continuation)
Olli, do you have some ideas?  You are more familiar with all of this node unlinking stuff.
Flags: needinfo?(bugs)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> Andrew, can you think of a clean way to check whether a node has been
> unlinked during UnbindFromTree? 
What does this even mean ;)
But no, we don't really have any flags indicating whether something has been unlinked, if that is what the question is about.

> I can't think of a much cleaner way to fix
> this except for maintaining a flag in nsGenericHTMLFormElement indicating
> whether mForm has ever been assigned to, which sucks... :/
Yeah, or just keep mForm around? Don't really see other ways. There needs to be some flag for this case (either bool or pointer to form).
Flags: needinfo?(bugs)
Flags: needinfo?(continuation)
Is it OK to keep mForm around?  I thought that Unlink() is supposed to remove the strong refs that Traverse() discovers.

If that is OK, it's probably an acceptable solution here.
well, I was thinking to keep mForm as a raw reference and rely on the current way to clear it.
(in other words, wontfix this bug)
Flags: needinfo?(ehsan)
https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.
Priority: -- → P5
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.