Closed
Bug 424780
Opened 16 years ago
Closed 15 years ago
Typing into <select> dropdown can cause nsStringBuffer leak
Categories
(Core :: Layout: Form Controls, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: cbook, Assigned: jruderman)
References
()
Details
(Keywords: memory-leak, testcase)
Attachments
(5 files, 1 obsolete file)
640 bytes,
text/plain
|
Details | |
15.71 KB,
text/plain
|
Details | |
203 bytes,
text/html
|
Details | |
3.24 KB,
patch
|
Details | Diff | Splinter Review | |
4.01 KB,
patch
|
jruderman
:
review+
|
Details | Diff | Splinter Review |
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032320 Firefox/3.0b5pre ID:2008032320 found this leak during running the Surf's up Test in Litmus. Steps to reproduce: -> Load https://litmus.mozilla.org/show_test.cgi?id=4390 -> Go to http://www.alabe.com/freechart/ -> Enter some Dates and -> on the following result site, move the Star Chart around -> Close Firefox --> Leaks nsStringBuffer Only small leak and so not requesting blocking. 0 TOTAL 25 8 10134557 1 ( 7427.45 +/- 5704.58) 21652820 1 ( 5026.63 +/- 7590.61)
Assignee | ||
Comment 1•16 years ago
|
||
Simpler steps: 1. Go to http://www.alabe.com/freechart/ 2. Enter "Los Angeles" and "California", leaving the dates alone. 3. Close Firefox.
Assignee | ||
Comment 2•16 years ago
|
||
Err, missed a "submit the form" step in there ;)
Assignee | ||
Comment 3•16 years ago
|
||
Actually, you don't need to submit the form. The leak involves the incremental typing feature of the <select> dropdown. Typing 'C' while the dropdown has focus is enough to trigger the leak. (I figured this out after looking at the leak log, which pointed at nsListControlFrame::KeyPress.)
Summary: Firefox leaks nsStringBuffer when i move the chart on http://www.alabe.com around → Typing into <select> dropdown can cause nsStringBuffer leak
Assignee | ||
Updated•16 years ago
|
Component: General → Layout: Form Controls
QA Contact: general → layout.form-controls
Assignee | ||
Comment 4•16 years ago
|
||
The leak log seems to implicate the "static nsString incrementalString;" that lives in nsListControlFrame::GetIncrementalString. I think this means you can only leak one of these at a time. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsListControlFrame.cpp&rev=1.436&mark=2496#2492 Is it a bug that this is a static variable rather than a member variable? Or would it be sufficient to add a "GetIncrementalString().Truncate();" somewhere?
Assignee | ||
Comment 5•16 years ago
|
||
To reproduce the leak: click the textbox, press Tab, press 'c', quit.
Comment 6•16 years ago
|
||
This should be a static nsString*, allocated and destroyed when we init/shutdown layout. imo.
yeah, static strings can crash XR apps rather easily too. We have an assertion that catches most of them, but apparently not this one...
Yeah, we're not going to catch that as the string isn't going to be initialized until the first time that function is called.
Assignee | ||
Comment 9•16 years ago
|
||
Sicking recommended allocating the nsString in nsListControlFrame::GetIncrementalString rather than when initing layout.
Assignee: nobody → jruderman
Status: NEW → ASSIGNED
Attachment #311490 -
Flags: superreview?(jonas)
Attachment #311490 -
Flags: review?(jonas)
Attachment #311490 -
Flags: superreview?(jonas)
Attachment #311490 -
Flags: superreview+
Attachment #311490 -
Flags: review?(jonas)
Attachment #311490 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Attachment #311490 -
Flags: approval1.9?
Assignee | ||
Comment 10•16 years ago
|
||
Bug 425068 prevents me from running mochitests locally, so I don't know whether the code that cares about sIncrementalString is covered by existing mochitests.
Comment 11•16 years ago
|
||
Jesse you should probably update the comment in the patch since those strings are not static..
Comment 12•16 years ago
|
||
Need to address the question raised in Comment #10 about whether or not we've test coverage for sIncrementalString. When requesting approvals, please: * Include an assessment of the risk associated with this patch. * Give a sentence or two why it's so important to take your patch this late in the release process. * Included tests so we don't have to ask for them.
Comment 13•16 years ago
|
||
Comment on attachment 311490 [details] [diff] [review] patch Please re-request approval once the question of test coverage is addressed.
Attachment #311490 -
Flags: approval1.9?
So we don't really have tests for leaks yet. But I think what we should do is to add a mochitest that sends key events to a dropdown and checks that that results in the correct being selected. Once we do leak tests on mochitest that will catch this issue and in the meantime we're checking that the patch won't regress the functionality.
Comment 15•16 years ago
|
||
Assuming this is cross-platform code, once we use runtests.py on the OS X box and add a --leak-threshold=0 to the command to invoke Mochitests, we'll get orange if this were to break. Other platforms (including the recently added bm-pgo box, which uses runtests.py and would only need the addition of --leak-threshold) have different sets of leaks which prevent doing this there immediately, unfortunately.
Assignee | ||
Comment 16•16 years ago
|
||
> Jesse you should probably update the comment in the patch since those
> strings are not static..
They're static members of the class (rather than per-instance members), but their values can change.
Assignee | ||
Comment 17•16 years ago
|
||
Assignee | ||
Comment 18•16 years ago
|
||
The mochitest currently fails due to script-generated events having timestamps in microseconds instead of milliseconds. See bug 238041. With my patch in that bug, the mochitest passes.
Depends on: 238041
Assignee | ||
Comment 19•15 years ago
|
||
Attachment #311490 -
Attachment is obsolete: true
Attachment #393383 -
Flags: review+
Assignee | ||
Comment 20•15 years ago
|
||
I'm going to land this fix without the test, and file a new bug for landing the test (depends on bug 238041).
Assignee | ||
Comment 21•15 years ago
|
||
Fix landed: http://hg.mozilla.org/mozilla-central/rev/13d567fb0899
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 22•15 years ago
|
||
Filed bug 509254 for landing the test.
No longer depends on: 238041
You need to log in
before you can comment on or make changes to this bug.
Description
•