309467 - Memory keeps growing when script repeately sets textfield's .value

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

(I really had no idea in which component to put this bug in)

This is a page that changes the content of a file and opens that file in a frame
and then refreshes. This is repeated every 5 seconds.

You have to test this locally.
Download the 3 files on your computer. 
Download "mozdata.htm" to c:\. 
If you use Linux, then you can download it anywhere you like, but then you need
to adjust var savefile = "c:\\mozdata.htm"; in "filewrite.js" to the correct
directory.

So when I let filewrite.xul run for a while, I keep getting increased memory
usage in Mozilla.
When I let filewrite.xul run for a whole night, Mozilla ate all my memory, and
my system became unusable.
So my question is really, is this normal for this testcase? (I suspect not)

Comment 1

[Jo Hermans]

Does this also occur in Firefox 1.5 beta 1 ?

Comment 2

[Boris Zbarsky [:bzbarsky]]

So.. I can reproduce the issue, and this is definitely not normal.  The bug is
present even in a TOO_MUCH_GC build.  However, I don't see any refcount leaks on
shutdown.

Given that, my guesses are that we're either keeping stuff alive through some
sort of gc/xpcom cycles that are broken at shutdown, or are leaking non-xpcom stuff.

Unfortunately, building with --enable-trace-malloc doesn't start if I actually
set --trace-malloc to anything.  :(

Comment 3

[Martijn Wargers (dead)]

(In reply to comment #1)
> Does this also occur in Firefox 1.5 beta 1 ?
Yes, I've made this some time ago, so I'm quite sure this is also an issue in
1.8b builds.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So... It seems that the user can undo scripted sets of .value on text inputs. 
So the memory growth is due to us storing all the past values in the editor
transaction list.

Should we perhaps clear the undo list when .value is set?  What do other
browsers do?

mconnor also suggested that a single undo should undo all scripted changes (that
is, they should all be considered one transaction).  Not sure we have useful
support for that in editor....

Comment 5

[Martijn Wargers (dead)]

Oops!
I didn't know that changing the input value was causing the memory growth
(otherwise I probably would not file the bug).
This was in fact my first bug fix, if I remember correctly, see bug 158071.
In IE6, you can undo scripted setting of input values.
Maybe there should be a limit on the amount of undo's or something?

Comment 6

[Bob Clary [:bc] (inactive)]

(In reply to comment #4)
> 
> Should we perhaps clear the undo list when .value is set?  What do other
> browsers do?

MS(IE) has two different kinds of edit controls. 1) A simple edit control which
can only undo 1 change and 2) A Rich edit control which can undo either 1 change
or multiple changes depending on how it is initialized. The multilevel undo is
programmatically configurable on how many undos can be done, the default is 100.
MS recommends keeping the level as low as possible to limit memory usage. It
appears that for html inputs, MSIE treats them as multilevel undos. I didn't
check to see what the maximum level is though.

Comment 7

[Hixie (not reading bugmail)]

(bob, do you have a reference for that? i'd like to read more about it since i
may have to spec some of this stuff out for WHATWG)

Comment 8

[Bob Clary [:bc] (inactive)]

(In reply to comment #7)

heh. ;-) I thought about leaving a trail of urls, but they were pretty long and
all to framesets. I found it googling for |internet explorer undo edit control
site:microsoft.com| and looking around. You can find more details about the rich
edit controls by adding |rich| to the query. 

Comment 9

[Boris Zbarsky [:bzbarsky]]

I think it would be reasonable to cap text inputs in particular (as opposed to
editor in general) in terms of the length of the undo list.

A cap of several hundred or even a thousand would prevent infinite resource
growth without unduly hindering actual use, I would think...

Comment 10

[Martijn Wargers (dead)]

Attachment: testcase

Quick and dirty testcase that shows the memory growing issue.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Brade, Simon, do you know whether editor already provides an API for limiting
the undo depth?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ah, nevermind.  Transaction manager has something.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible patch

Martijn, does that help?

Comment 14

[Martijn Wargers (dead)]

Yes, I think so, (although to be absolutely sure I should let the testcase run
for a night or so).

Comment 15

[[e-mail]]

Comment on attachment 197853 [details] [diff] [review]
Possible patch

>+  nsCOMPtr<nsITransactionManager> transMgr;
>+  mEditor->GetTransactionManager(getter_AddRefs(transMgr));
>+  NS_ENSURE_TRUE(transMgr, NS_ERROR_FAILURE);
>+
>+  transMgr->SetMaxTransactionCount(DEFAULT_UNDO_CAP);
>+
>   if (IsPasswordTextControl()) {
>     // Disable undo for password textfields.  Note that we want to do this at
>     // the very end of InitEditor, so the calls to EnableUndo when setting the
>     // default value don't screw us up.
>     // Since changing the control type does a reframe, we don't have to worry
>     // about dynamic type changes here.
>     mEditor->EnableUndo(PR_FALSE);
>   }
Is it me or could you have limited the undo in an else clause?

Comment 16

[Boris Zbarsky [:bzbarsky]]

I could have, but I'd like to do it this way in case some extension messes with
password inputs....

Comment 17

[Martijn Wargers (dead)]

I just noticed that this is also noticable with the url bar. Switching tabs and
so is also stored in the url bar textbox's transaction manager.
I think I like it, but maybe this is not desirable?

Comment 18

[Boris Zbarsky [:bzbarsky]]

*** Bug 312779 has been marked as a duplicate of this bug. ***

Comment 19

[Asa Dotzler [:asa]]

Not going to block on this but when this lands and is verified on the trunk,
please request approval to land on the branch and we'll evaluate it there. Thanks.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 197853 [details] [diff] [review]
Possible patch

r=jst, though I would've picked a much smaller number than 1000 for the default
cap. How many levels of undo do we need, really? 100 even seems like a lot. I'm
fine with this change either way though.

Comment 21

[Boris Zbarsky [:bzbarsky]]

The 1000 is mostly to avoid breaking things that set the textfield over and over
again and that might want programmatic undo.  It prevents unbounded memory
growth, for now.  In the 1.9 cycle, we might want to pick a lower bound.

Comment 22

[Darin Fisher]

What undo limit does IE use?  If it has a limit of 10 or 20, then there's little
reason for us to go over that limit, right?

Comment 23

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #22)
> What undo limit does IE use?  If it has a limit of 10 or 20, then there's little
> reason for us to go over that limit, right?

Having a full-depth undo stack is one of the plusses reported by users about
our editor. So if you want to limit that depth for textfields, you'll have to
think about a way to discriminate between textfields and other usage of the
editor...

Comment 24

[Boris Zbarsky [:bzbarsky]]

Daniel, we're limiting for only nsTextControlFrame -- that means <input
type="text"> and <textarea>.  And 1000 should be high enough that users won't
really run into the limit; that's why I picked it.

Darin, IE defaults to 100 but has an API that allows the page to change the
value.  We could expose a similar API if desired, I guess....

Comment 25

[Darin Fisher]

> Darin, IE defaults to 100 but has an API that allows the page to change the
> value.  We could expose a similar API if desired, I guess....

In that case, something larger than 100 sounds good to me too since there's
surely no chance of introducing those APIs for 1.5 ;-)

Comment 26

[Boris Zbarsky [:bzbarsky]]

That was sorta the thinking.  ;)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 197853 [details] [diff] [review]
Possible patch

Requesting branch approval.  This limits undo in textareas and text inputs to
the last 1000 transactions.  Impact is only on people who try to undo more than
that...  I think this should be pretty safe.

Comment 29

[Alex Vincent [:WeirdAl]]

Interesting that you guys mention html inputs and textareas.  I was just playing
with <xul:textbox/>, in adding a reset() functionality (bug 312867).

Do you want an API exposed for xul:textbox that allows people to change the
max-txmgr history directly as well?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Eventually, yes.  Built on top of the same API for text inputs...

Comment 31

[u81239]

Should setting an input value dynamically through .value trigger an addition to
the undo history at all?

I’d say that should only happen for actual user input...


~Grauw

Comment 32

[James Ross]

(In reply to comment #31)
> Should setting an input value dynamically through .value trigger an addition to
> the undo history at all?

I believe it should (as was the point of bug 158071), not just because IE6 does
it, but consider the case that the webpage it helping the user edit the text,
such as a forum post with buttons for adding bold, italic, links, etc.. Losing
your undo history just because you asked the page for help instead of typing it
yourself seems a pretty bad experience to me.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Fixed on branch.