Bug 14569 - Repro crash due to saved state not being cleared when select and textarea elements are adopted by another document
Summary: Repro crash due to saved state not being cleared when select and textarea ele...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P1 Major
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-07-10 00:16 PDT by mitz
Modified: 2007-07-17 10:51 PDT (History)
0 users

See Also:


Attachments
[DO NOT LAND YET] Unregister and reregister when moving from one document to another (5.65 KB, patch)
2007-07-10 00:28 PDT, mitz
mjs: review+
Details | Formatted Diff | Diff
Unregister and reregister when moving from one document to another, using a registersWithDocument predicate (9.11 KB, patch)
2007-07-11 09:35 PDT, mitz
no flags Details | Formatted Diff | Diff
unregister/register when moving between documents with class to factor out state logic (36.96 KB, patch)
2007-07-16 23:42 PDT, Darin Adler
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2007-07-10 00:16:56 PDT
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.
Comment 1 mitz 2007-07-10 00:28:45 PDT
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?
Comment 2 Brady Eidson 2007-07-10 00:47:49 PDT
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
Comment 3 Brady Eidson 2007-07-10 00:49:51 PDT
"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"
Comment 4 Maciej Stachowiak 2007-07-10 02:21:47 PDT
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
Comment 5 mitz 2007-07-11 09:35:48 PDT
Created attachment 15474 [details]
Unregister and reregister when moving from one document to another, using a registersWithDocument predicate
Comment 6 Darin Adler 2007-07-16 22:40:09 PDT
Since there are tons of bits available in HTMLGenericFormElement, I grabbed a bit and have what I think is an even better fix.
Comment 7 Darin Adler 2007-07-16 23:42:05 PDT
Created attachment 15540 [details]
unregister/register when moving between documents with class to factor out state logic
Comment 8 mitz 2007-07-17 00:04:50 PDT
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!
Comment 9 Darin Adler 2007-07-17 08:21:28 PDT
(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.
Comment 10 Darin Adler 2007-07-17 10:51:10 PDT
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.