Extreme memory growth on DOM Hanoi test

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

DOM Hanoi test with the default recursion level of 3 takes several hundred megabytes, but almost nothing in Firefox.

Steps to reproduce: run <https://safari.apple.com/groups/safariteam/wiki/bb053/attachments/febd2/DOM-Hanoi%20v0.2.html>.

In Safari, RPRVT steadily grows - up to about 800 Mb on x86-64, and perhaps even more on PowerPC. In Firefox, it oscillates between 40 and 80 Mb.

leaks tool says there are no leaks.

<rdar://problem/8009738>

[Alexey Proskuryakov]

One thing I'm seeing is the same as <https://bugs.webkit.org/show_bug.cgi?id=9880> - we just added a notification that's sent to the client when form controls change:
HTMLTextAreaElement::HTMLTextAreaElement(const QualifiedName& tagName, Document* document, HTMLFormElement* form)
<...>
{
    ASSERT(hasTagName(textareaTag));
    setFormControlValueMatchesRenderer(true);
    notifyFormStateChanged(this);  // <-- here an ObjC wrapper for the node is created and autoreleased
}

It's more tricky to fix, because HTML elements start with refcount zero, so draining the autorelease pool would destroy HTMLTextAreaElement - during its own constructor!

The offending notifyFormStateChanged() call was added in <http://trac.webkit.org/changeset/39152>. It's only present for HTMLTextArea, which makes me wonder if it's actually needed.

[Alexey Proskuryakov]

Without ObjC-style autorelease, destroying a temporary wrapper made for this call will just destroy the element, like it happens when forcing autorelease pool draining in ObjC case.

So, this can't really work on other platforms. Note that WebKit/win has an empty implementation for formStateDidChange(), and WebKit/chromium all but ignores the element argument.

[Alexey Proskuryakov]

This call was added by Brett Wilson in <http://trac.webkit.org/changeset/39152>, and Mac implementation was added by Greg Bolsinga in <http://trac.webkit.org/changeset/45723>.

Brett and Greg, can we just remove the notifyFormStateChanged() call from HTMLTextAreaElement constructor? Greg, is formStateDidChange delegate call needed at all? Bug 27153 only mentioned focus and blur as needed calls.

[Greg Bolsinga]

I can confirm that WebChromeClient::formStateDidChange() is not needed on iPhone right now.

[Greg Bolsinga]

In other words, that can be made an empty inline method, and not be implemented to call CallUIDelegate.

[Brett Wilson (Google)]

Chrome this to implement session saving without polling. When the user changes a form field, we set a timer (to avoid saving too often) and then serialize the current form state to disk. Then we can restore the form state when recovering from a crash.

The other form elements do the same thing, but they derive from HTMLInputElement which is where the code lives for those controls.

It's not immediately clear why this call has to be in the constructor. My guess is that I was thinking if a form control was added with some initial value, we would want to count that as a change to the form state of the page. However, it appears that the initial set is coming from setNonDirtyValue. So I think the call to notifyFormStateChanged in the constructor can be safely removed.

[Brett Wilson (Google)]

(To be clear, though, the other calls should stay, we can't remove all of them without breaking this feature.)

[Alexey Proskuryakov]

Created attachment 57024 [details]
proposed fix

[Darin Adler]

Comment on attachment 57024 [details]
proposed fix

> +        * html/HTMLTextAreaElement.cpp: (WebCore::HTMLTextAreaElement::HTMLTextAreaElement):
> +        Don't call notifyFormStateChanged() - since the element starts with refcount 0, it's not
> +        safe to call functions that are likely to create temporary wrappers (wrapper destructor
> +        would bring refcount back to 0, and destroy HTMLTextAreaElement from within its constructor).

This could also be resolved by making HTMLTextAreaElement start with a reference count of 1. We do plan to do this eventually for all DOM classes, and we could do this one ahead of time.

I assume that the real reason it's OK not to call this is that people don't need the call. Your change log entry talks about why the call caused problems, but does not address why it's OK to remove the call.

> +        Need a short description and bug URL (OOPS!)

I assume that your comment here was going to say something about his private WebUIDelegate method being no longer needed since it's no longer used by Safari nor any other client of the Mac OS X version of WebKit.

Perhaps we could remove formStateDidChange entirely. Does any platform use it in WebKit?

Generally speaking the issue of autoreleased objects could be addressed by adding autorelease pools as well.

I'm going to say review- on this, but really the code change might be OK if you put clearer comments in

[Alexey Proskuryakov]

Created attachment 57027 [details]
now with a saved ChangeLog

[Darin Adler]

Comment on attachment 57027 [details]
now with a saved ChangeLog

>  {
>      ASSERT(hasTagName(textareaTag));
>      setFormControlValueMatchesRenderer(true);
> -    notifyFormStateChanged(this);
>  }

This patch still does not explain why this change is OK. The WebKit part of the patch removes the Mac OS X code entirely, which does two things:

    1) Makes it clear this change is OK on Mac OS X.
    2) Does some of the cleanup of this unused feature.

But this does not prove that this change is OK for non-Mac-OS-X versions of WebKit, nor does it do all the cleanup that's possible. I think it's fine to remove this unused function call, but I don't see why you'd want to do some but not all of the cleanup at the same time.

And this code change seems to have an effect on Chromium. ChromeClientImpl::formStateDidChange for chromium calls didUpdateCurrentHistoryItem. That code is probably incorrect, but this code change will mean it doesn't get called any more.

I'm going to say r=me if you keep the code in inside a PLATFORM(CHROMIUM) ifdef, leaving a known but in the Chromium case that can be addressed next. Then we should clean all this out entirely as quickly as possible.

[Darin Adler]

OK, I see the Chromium comments in the change log now.

Sorry I should have read this before.

[Darin Adler]

The change log comments could say:

    This call is only needed on Chromium, ignored on all other platforms, and on Chromium it's not needed at creation time for new textarea elements.

If it said that, then I would not have been confused.

[Alexey Proskuryakov]

> The change log comments could say:

Oops, landed before I saw this. Sorry, I see how this could be confusing without looking at bug comments.

Committed <http://trac.webkit.org/changeset/60175>. There is some memory growth remaining, unlike in Firefox, I'll file a new bug for that.