Bug 12938 – Google calendar settings page crashes

• Component: DOM
• Status: RESOLVED FIXED

[Brett Wilson (Google)]

Go to Google Calendar and click "Settings". Click back and forth between the first two tabs ("General" and "Calendars") as fast as possible. After a while (a few seconds to 20 seconds, usually), it will crash.

The problem is updating the check state of the radio buttons on the "General" tab. At some point, the hash table mapping checkbox names to objects becomes corrupted or out of sync.

[Brett Wilson (Google)]

Created attachment 13447 [details]
Reduced testcase

This patch seems to crash recent WebKit nightlies (although looks like not shipping Safari). There is a small chance that this is actually a different crash than this bug, but is very likely related.

It seems to help if you click on the text, but this isn't required. It will crash eventually if you leave it alone.

It seems wrong that HTMLFormElement::~HTMLFormElement() does not tell the document that radio buttons have been removed like HTMLFormElement::removeFormElement() does.

[Brett Wilson (Google)]

HTMLFormElement::removeFormElement is not always called. In HTMLGenericFormElement::removedFromTree, it does this check:

    // If the form and element are both in the same tree, preserve the
    // connection to the form. Otherwise, null out our form and remove ourselves
    // from the form's list of elements.
    if (m_form && !m_form->preserveAcrossRemove() &&
            findRoot(this) != findRoot(m_form)) {
        m_form->removeFormElement(this);
        m_form = 0;
    }

I don't know what this check is for. When the parent form is being removed, though, this check fails and the input element is never able to notify its document that it's gone. The Document's radio state is never updated and you have a stale pointer.

A related problem is that the form never tells the document that it is going away. The document maintains a map of form addresses that are used to index into another map of radio group names -> checked element pointers.

When the form is removed, the document's map keeps the form element's pointer in its map. In addition, the destroyed radio buttons are never removed from the form map that this points to. If you then create a new form quickly so that it has the exact same address as the removed form, you'll end up using the old form's hash table, with the old pointers.

I think there's two things that need to be done. HTMLInputElements should be fixed so they remove themselves at the right time. I'm not sure the correct time to do this because I don't understand the condition in the code I wrote above. It could be that this condition should be modified such that it succeeds when the form is being removed.

The other thing that needs to be fixed is that the form element needs to get its hashtable removed from the document when it is being removed. Even if the above bug is fixed, you'll accumulate (empty) hash tables for every form element that has ever existing in a document.

(In reply to comment #3)
> I don't know what this check is for.

It is precisely for the case of the form control being removed along with the form (rather then being yanked out of the form).

> When the parent form is being removed,
> though, this check fails and the input element is never able to notify its
> document that it's gone. The Document's radio state is never updated and you
> have a stale pointer.

I do not think you have a stale pointer at that point -- nothing has been destroyed.

> A related problem is that the form never tells the document that it is going
> away.

I think that's the only problem.

> When the form is removed, the document's map keeps the form element's pointer
> in its map.

That's okay, you only need to remove the form from the map when it's destroyed.

> In addition, the destroyed radio buttons are never removed from the
> form map that this points to.

Seems to me that the radio buttons in questions are destroyed under the HTMLFormElement destructor. If you make the destructor tell the document to remove the form from its map at that point, then you'll have removed any pointers to those buttons as well.

> The other thing that needs to be fixed is that the form element needs to get
> its hashtable removed from the document when it is being removed.

I think you only need to do it when the form element is destroyed, and I think that alone will fix the bug.

[Maciej Stachowiak]

I think the analysis from Mitz is sound.

[Brett Wilson (Google)]

Mitz says on IRC that all we need to do is in HTMLFormElement's destructor, notify the document that the form is gone. A new function on Document would then remove that form from the document's hashtable of forms.

[Brett Wilson (Google)]

Created attachment 13458 [details]
Patch

This is a patch implemented as discussed above.

[Darin Adler]

Comment on attachment 13458 [details]
Patch

+void Document::removeForm(HTMLFormElement* form) {

Brace goes on the line after the function declaration.

+    void removeForm(HTMLFormElement* form);

Neither this function, nor the old functions just above it, should give the parameter a name. In WebKit code, when the type makes the parameter clear we omit the name.

+    // The remove function will handle the case if the form is not found.

The above comment doesn't make any sense in this version of the patch. Maybe there was an earlier version where it made sense.

You need to delete formRadioButtons in Document::removeForm; I presume that's the reason you called get.

There's an uglier but more efficient way to both get and remove an element from a HashMap without doing two hash table lookups:

    FormToGroupMap::iterator it = m_selectedRadioButtons.find(form);
    if (it == m_selectedRadioButtons.end())
        return;
    delete it->second;
    m_selectedRadioButtons.remove(it);

When the form is removed, is there a possibility that radio buttons that remain behind could now be inside a new outer form? If so, then we probably need to put the form elements in the appropriate map; simply discarding them might not be right. Would you be willing to write a test case for that?

review- because of the storage leak

[Brett Wilson (Google)]

(In reply to comment #8)
> When the form is removed, is there a possibility that radio buttons that remain
> behind could now be inside a new outer form? If so, then we probably need to
> put the form elements in the appropriate map; simply discarding them might not
> be right. Would you be willing to write a test case for that?

I'm not sure what you mean. Do you mean
  <form><form><input type=radio>...</form></form>
and we remove the inner form? Won't that remove the radio buttons and eliminate the problem? 

[Mark Rowe (bdash)]

<rdar://problem/5045711>

[Maciej Stachowiak]

(In reply to comment #9)
> 
> I'm not sure what you mean. Do you mean
>   <form><form><input type=radio>...</form></form>
> and we remove the inner form? Won't that remove the radio buttons and eliminate
> the problem? 
> 

I think you  are right on that, although I believe Darin's other review comments are still applicable.

I think there's one other case that needs to be considered, which is when a (subtree containing a) form is adopted by a different document: Document::adoptNode() calls setDocument() on the form, but does not tell the old document to forget about it.

[Brett Wilson (Google)]

Created attachment 13725 [details]
Revised patch

[Darin Adler]

Comment on attachment 13725 [details]
Revised patch

Thanks. Looks like a good fix!

A style comment that I think I already made last time:

+void Document::removeForm(HTMLFormElement* form) {

Brace goes on next line.

We need a regression test that demonstrates the bug.

Should remove the conflict markers in the change log.

review- because of the lack of a regression test.

[Darin Adler]

(In reply to comment #12)
> I think there's one other case that needs to be considered, which is when a
> (subtree containing a) form is adopted by a different document:
> Document::adoptNode() calls setDocument() on the form, but does not tell the
> old document to forget about it.

Did you address this comment?

[Brett Wilson (Google)]

(In reply to comment #14)
> (From update of attachment 13725 [details] [edit])
> Thanks. Looks like a good fix!
> 
> A style comment that I think I already made last time:
> 
> +void Document::removeForm(HTMLFormElement* form) {
> 
> Brace goes on next line.

Sorry, I've got too many coding styles floating around my head.


> We need a regression test that demonstrates the bug.

I don't know how to write a reliable regression test for this bug. The attached testcase is the best I can do, and the instructions are to run it for "a while" and see if it crashes. Sometimes it takes a long time to crash, sometimes it doesn't, depending on how the memory manager is feeling today. I can run it for 200 times or so and we can say it passes if it doesn't crash, if that sounds good to you.


> Did you address this comment?

No, we posted at basically the same time.

[Brett Wilson (Google)]

Created attachment 13739 [details]
Not working adoptNode

I'm trying to generate an adoptNode testcase. This is what I came up with, but it doesn't seem to work. It seems like the form should move from the outer document to the inner document, but I get some kind of exception in the adoptNode. Does anybody know?

[Brett Wilson (Google)]

Created attachment 13774 [details]
Better not working adoptNode test

This is a more complete test using adoptNode, although I was still unable to get it to work. What I was going for is moving the form to another document, deleting the checked radio button, and moving it back to the original document. With this bug, the pointer will still be in the original document to the old radio button, causing a crash.

But I was unable to get the HTMLInputElement object to get deleted when I wanted despite some suggestions from Mitz. Maybe there are more ideas or ideas where my test case went wrong?

[Brett Wilson (Google)]

Created attachment 13783 [details]
Revised patch again

This is a revised patch that also fixes the potential crash on removing forms in the document. This is indeed a problem: the pointer to the form is left in the old document's hash table when it is moved.

As discussed above, I was having problems generating a test for the move between documents bug. After spending several hours on this, I've never actually seen this one cause a crash, although it's definitely possible (and also a memory leak).

I can convert the testcase I attached at the top (first attachment) into a regression test if you would like, but it would not be totally reliable since it depends on the memory manager's feeling of where to place things. My PowerBook seems to never trip on this page, although a colleague's Intel-based iMac does after a bit. Requesting review to get official opinion on this.

[Darin Adler]

Comment on attachment 13783 [details]
Revised patch again

I think a test is valuable even if it doesn't always fail. So I'd like to see the test too.

r=me on this

Comment on attachment 13783 [details]
Revised patch again

I'm confused by this. Why is it OK to call removeForm() when removing and re-inserting the form into the same document? Are the child input elements going to re-register themselves with the form in that case?

Created attachment 13788 [details]
A test case that the latest patch (13783) breaks

Attachment 13783 [details] breaks this test case: after clicking "jump", clicking the unselected radio button does not deselect the other one.

Please reconsider if you want to land the current patch!

[Darin Adler]

Comment on attachment 13783 [details]
Revised patch again

Oops. Mitz is right. And he mentioned this before. I just forgot. review- until we deal with that issue.

Playing around with attachment 13774 [details], I got a crash under Document::formElementsState() when I closed the browser window or navigated away from the page.

[Brady Eidson]

I have a clear view on how to fix this one, and plan to after mother's day festivities

[Brett Wilson (Google)]

Thanks, Brady. This bug kind of fell off my radar as I don't really understand how to fix the issue. Let me know if you need help on this.

[Anders Carlsson]

Committed revision 24541.