Bug 14569 – Repro crash due to saved state not being cleared when select and textarea elements are adopted by another document

• Component: Forms
• Status: RESOLVED FIXED

This bug is very similar to bug 13701. The patch for that bug left out textarea and select elements. I am going to attach a patch that fixes that and includes a test for both bugs.

Created attachment 15461 [details]
[DO NOT LAND YET] Unregister and reregister when moving from one document to another

Another way to do this is to implement HTMLGenericFormElement::{will,did}MoveToNewOwnerDocument, but I am afraid that that will require either a registersWithDocument() function like I had in the first patch for bug 14400 or for HTMLInputElement::didMove... not to call the base class implementation.

What do you think?

[Brady Eidson]

I'm okay with this patch - but I just had a discussion with mitz on IRC.  I think now that we have these new cases that have to patch in to the will/did move document calls that the original patch in 14400 is more appropriate.  It would be very easy to adapt here.

I look at it this way.  If we land this new patch, we'll be maintaining 3 distinct implementations of the will/did move document calls.  One in TextArea, one in Select, and one in Input.  However, they are largely duplicates of each other.

If we go back to Mitz's original patch in 14400, there will be yet another new call to override in genericform and inputelement, and we'll have to maintain it in these multiple places, but it will be much more targeted code will be duplicated.

Mitz said he was starting to lean towards that solution as well, but I'm only slightly in favor of reverting back to the original 14400 patch  =D

[Brady Eidson]

"but it will be much more targeted code will be duplicated."

should read 

"but it will be much more targeted code that makes sense, and not as much code will be duplicated"

[Maciej Stachowiak]

Comment on attachment 15461 [details]
[DO NOT LAND YET] Unregister and reregister when moving from one document to another

This looks like a fine fix to the bug in any case. r=me

Created attachment 15474 [details]
Unregister and reregister when moving from one document to another, using a registersWithDocument predicate

[Darin Adler]

Since there are tons of bits available in HTMLGenericFormElement, I grabbed a bit and have what I think is an even better fix.

[Darin Adler]

Created attachment 15540 [details]
unregister/register when moving between documents with class to factor out state logic

Comment on attachment 15540 [details]
unregister/register when moving between documents with class to factor out state logic

Cool! This is the "third option" that I told Brady is was considering.

+        (WebCore::HTMLSelectElement::~HTMLSelectElement): Removed the call to
+        unregisterFormElementWithState.

 HTMLSelectElement::~HTMLSelectElement()
 {
-    document()->unregisterFormElementWithState(this);
 }
 
I don't understand why you left the empty destructor in this case but not in the text area case.

r=me!

[Darin Adler]

(In reply to comment #8)
> I don't understand why you left the empty destructor in this case but not in
> the text area case.

Because HTMLSelectElement had members with non-trivial destructors I decided not to rely on the compiler-generated destructor. Probably not a good reason.

[Darin Adler]

Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/forms/saved-state-adoptNode-crash-expected.txt
Adding         LayoutTests/fast/forms/saved-state-adoptNode-crash.html
Sending        WebCore/ChangeLog
Sending        WebCore/dom/Document.cpp
Sending        WebCore/dom/Document.h
Sending        WebCore/html/HTMLGenericFormElement.cpp
Sending        WebCore/html/HTMLGenericFormElement.h
Sending        WebCore/html/HTMLInputElement.cpp
Sending        WebCore/html/HTMLInputElement.h
Sending        WebCore/html/HTMLSelectElement.cpp
Sending        WebCore/html/HTMLSelectElement.h
Sending        WebCore/html/HTMLTextAreaElement.cpp
Sending        WebCore/html/HTMLTextAreaElement.h
Transmitting file data ..............
Committed revision 24363.