Closed Bug 979993 Opened 6 years ago Closed 6 years ago

Small leak in HTMLInputElement::SetValueInternal

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox28 --- wontfix
firefox29 --- fixed
firefox30 --- fixed
firefox-esr24 --- wontfix

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lsan])

Attachments

(1 file)

I see these in LSAN logs:

reftest:
Direct leak of 392 byte(s) in 94 object(s) allocated from:
    #0 0x446395 in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7fd5f1307b18 in moz_xmalloc /somepath/memory/mozalloc/mozalloc.cpp:52
    #2 0x7fd5e987eb26 in mozilla::dom::HTMLInputElement::HandleTypeChange(unsigned char) /somepath/content/html/content/src/HTMLInputElement.cpp:4455
    #3 0x7fd5e989fe04 in mozilla::dom::HTMLInputElement::ParseAttribute(int, nsIAtom*, nsAString_internal const&, nsAttrValue&) /somepath/content/html/content/src/HTMLInputElement.cpp:4856
    #4 0x7fd5e9506acc in mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) /somepath/content/base/src/Element.cpp:1776

crashtest:
Direct leak of 6 byte(s) in 3 object(s) allocated from:
    #0 0x446395 in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7f1d94635b18 in moz_xmalloc /somepath/memory/mozalloc/mozalloc.cpp:52
    #2 0x7f1d8deb2b26 in mozilla::dom::HTMLInputElement::HandleTypeChange(unsigned char) /somepath/content/html/content/src/HTMLInputElement.cpp:4455
    #3 0x7f1d8ded3e04 in mozilla::dom::HTMLInputElement::ParseAttribute(int, nsIAtom*, nsAString_internal const&, nsAttrValue&) /somepath/content/html/content/src/HTMLInputElement.cpp:4856
    #4 0x7f1d8db3aacc in mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) /somepath/content/base/src/Element.cpp:1776

I'm not sure what is happening here, but I'm guessing it is legit, given that it occurs across multiple tests.  If looking at the code doesn't turn anything up, perhaps bisecting the test suite, or trying just the test that deals with whatever this is, could provide some insight.
Some other stacks involving HTMLInputElement, from reftest:
Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x446395 in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7fd5f1307b18 in moz_xmalloc /somepath/memory/mozalloc/mozalloc.cpp:52
    #2 0x7fd5e98859c9 in mozilla::dom::HTMLInputElement::SetValue(nsAString_internal const&, mozilla::ErrorResult&) /somepath/content/html/content/src/HTMLInputElement.cpp:1806
    #3 0x7fd5e7d2e340 in mozilla::dom::HTMLInputElementBinding::set_value(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLInputElement*, JSJitSetterCallArgs) /somepath/obj-firefox/dom/bindings/./HTMLInputElementBinding.cpp:1803
    #4 0x7fd5e864e9dc in mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*) /somepath/dom/bindings/BindingUtils.cpp:2235
Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x446395 in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7fd5f1307b18 in moz_xmalloc /somepath/memory/mozalloc/mozalloc.cpp:52
    #2 0x7fd5e987aefe in SetDefaultValueAsValue /somepath/content/html/content/src/HTMLInputElement.cpp:5476
    #3 0x7fd5e987aefe in mozilla::dom::HTMLInputElement::AfterSetAttr(int, nsIAtom*, nsAttrValue const*, bool) /somepath/content/html/content/src/HTMLInputElement.cpp:1355
    #4 0x7fd5e9507d95 in mozilla::dom::Element::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, unsigned char, bool, bool, bool) /somepath/content/base/src/Element.cpp:1898
    #5 0x7fd5e9506b7c in mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) /somepath/content/base/src/Element.cpp:1780
Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x446395 in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7fd5f1307b18 in moz_xmalloc /somepath/memory/mozalloc/mozalloc.cpp:52
    #2 0x7fd5e98a638a in SetDefaultValueAsValue /somepath/content/html/content/src/HTMLInputElement.cpp:5476
    #3 0x7fd5e98a638a in mozilla::dom::HTMLInputElement::Reset() /somepath/content/html/content/src/HTMLInputElement.cpp:5503
    #4 0x7fd5e983a5c5 in DoReset /somepath/content/html/content/src/HTMLFormElement.cpp:614
    #5 0x7fd5e983a5c5 in mozilla::dom::HTMLFormElement::DoSubmitOrReset(mozilla::WidgetEvent*, int) /somepath/content/html/content/src/HTMLFormElement.cpp:589
    #6 0x7fd5e983c023 in mozilla::dom::HTMLFormElement::PostHandleEvent(nsEventChainPostVisitor&) /somepath/content/html/content/src/HTMLFormElement.cpp:551
Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x446395 in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7fd5f1307b18 in moz_xmalloc /somepath/memory/mozalloc/mozalloc.cpp:52
    #2 0x7fd5e988582b in mozilla::dom::HTMLInputElement::SetValue(nsAString_internal const&, mozilla::ErrorResult&) /somepath/content/html/content/src/HTMLInputElement.cpp:1793
    #3 0x7fd5e7d2e340 in mozilla::dom::HTMLInputElementBinding::set_value(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLInputElement*, JSJitSetterCallArgs) /somepath/obj-firefox/dom/bindings/./HTMLInputElementBinding.cpp:1803
    #4 0x7fd5e864e9dc in mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*) /somepath/dom/bindings/BindingUtils.cpp:2235
These stacks are pretty useless, so I'll try sprinkling around a bunch of MOZ_NEVER_INLINE and try again.
Could it be:

mInputData.mState = new nsTextEditorState(this);

?

I worked really hard to get this lifetime right iirc, but maybe not all corner cases are covered?
Kyle suggested that, too, but nsTextEditorState does have MOZ_COUNT_CTOR/DTOR, which I think would catch that.
What I mean is, I'd think it would show up with our usual leak detectors.
Well, that didn't really help that much, as it just ends up pointing to a bunch of lines that call SetValueInternal.  I have no idea why it isn't showing where inside that we are actually allocating, because I set to to be MOZ_NEVER_INLINE, and it is called many places, so it seems weird that it would inline it.
Running it locally seems to give better line numbers, for whatever reason.  This appears to be the line that is allocating the thing that leaks:
  mInputData.mValue = ToNewUnicode(value);
(line 2801 in SetValueInternal)
Summary: Small leak in HTMLInputElement::HandleTypeChange → Small leak in HTMLInputElement::SetValueInternal
If I were a betting man I would say that there's probably already a pointer in mValue that we're overwriting without freeing.
Yeah, that looks plausible.  I'll try to hack up a fix.
Assignee: nobody → continuation
Er, right you are.  Needs FreeData() calls or equivalent!
This reduces the leaks in an M1 run by 140KB, and eliminates all leaks with stacks involving HTMLInputElement.
Comment on attachment 8387118 [details] [diff] [review]
Free the old input data in SetValueInternal.

Review of attachment 8387118 [details] [diff] [review]:
-----------------------------------------------------------------

I just inlined and specialized FreeData().
Attachment #8387118 - Flags: review?(bzbarsky)
This whole setup is still fragile, but this fixes the leak for now.
Comment on attachment 8387118 [details] [diff] [review]
Free the old input data in SetValueInternal.

r=me
Attachment #8387118 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/45363136618e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
I assume we want this on all the branches?
It seems low risk, but I don't know how bad the leak really is.  It might be useful for Terako, though I don't know how many HTMLInputElements there are going to be in the main process.
I don't think we need to worry about this on the phone.
The size of this leak is controlled by the value that the content is storing in our input element.  Given that, and that the patch is very low risk, I think we should take it on branches.
Comment on attachment 8387118 [details] [diff] [review]
Free the old input data in SetValueInternal.

The ship has sailed on beta, but aurora is still fair game.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): very old
User impact if declined: possibly large memory leaks
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none
Attachment #8387118 - Flags: approval-mozilla-aurora?
Attachment #8387118 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.