979993 - Small leak in HTMLInputElement::SetValueInternal

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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

Comment 2

[Andrew McCreight [:mccr8]]

These stacks are pretty useless, so I'll try sprinkling around a bunch of MOZ_NEVER_INLINE and try again.

Comment 3

[Andrew McCreight [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=f7fa3078c4c1

Comment 4

[(no longer active)]

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?

Comment 5

[Andrew McCreight [:mccr8]]

Kyle suggested that, too, but nsTextEditorState does have MOZ_COUNT_CTOR/DTOR, which I think would catch that.

Comment 6

[Andrew McCreight [:mccr8]]

What I mean is, I'd think it would show up with our usual leak detectors.

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Andrew McCreight [:mccr8]]

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)

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

If I were a betting man I would say that there's probably already a pointer in mValue that we're overwriting without freeing.

Comment 10

[Andrew McCreight [:mccr8]]

Yeah, that looks plausible.  I'll try to hack up a fix.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Er, right you are.  Needs FreeData() calls or equivalent!

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: Free the old input data in SetValueInternal.

Comment 13

[Andrew McCreight [:mccr8]]

This reduces the leaks in an M1 run by 140KB, and eliminates all leaks with stacks involving HTMLInputElement.

Comment 14

[Andrew McCreight [:mccr8]]

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().

Comment 15

[Andrew McCreight [:mccr8]]

This whole setup is still fragile, but this fixes the leak for now.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8387118 [details] [diff] [review]
Free the old input data in SetValueInternal.

r=me

Comment 17

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45363136618e

Comment 18

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=5cc305d3c7e7

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45363136618e

Comment 20

[Nicholas Nethercote [inactive]]

\o/

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

I assume we want this on all the branches?

Comment 22

[Andrew McCreight [:mccr8]]

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.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't think we need to worry about this on the phone.

Comment 24

[(no longer active)]

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 25

[Andrew McCreight [:mccr8]]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/004b68d7a880