1774489 - Perma leakcheck StringAdopt for tests using Glean events with extra keys and testGetValue

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Chris H-C :chutten]

The following browser-chrome mochitest test will leak 6 StringAdopt objects (if you comment out one of the extra key/value pairs, it will leak 4):

/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

"use strict";

add_task(async () => {
  Assert.equal(
    undefined,
    Glean.testOnlyIpc.eventWithExtra.testGetValue(),
    "Nothing to begin with"
  );  
  Glean.testOnlyIpc.eventWithExtra.record({
    extra1: "Some extra string",
    extra2: 42, 
    extra3_longer_name: false,
  }); 
  Assert.equal(
    1,  
    Glean.testOnlyIpc.eventWithExtra.testGetValue().length,
    "One event? One event."
  );  

  // AND NOW, FOR THE TRUE TEST:
  // Will this leak memory all over the place?
});

This isn't great and should be looked into.

Comment 1

[Chris H-C :chutten]

It leaks on testGetValue not on the record call, and leaks one instance per extra key and per extra value.

Looks like it's coming from how we're moving event extra keys and values across from Rust. I'm gonna have to dig into how we return nsCStrings on those APIs. Maybe I forgot to release them someplace.

 1:16.02 INFO Serial Numbers of Leaked Objects:
 1:16.02 INFO 2921 @0x7f347bcfb5e0 (0 references; 0 from COMPtrs)
 1:16.02 INFO allocation stack:
Initializing stack-fixing for the first stack frame, this may take a while...
 1:31.64 INFO #00: Gecko_IncrementStringAdoptCount (/home/chutten/gecko/xpcom/string/nsSubstring.cpp:321)
 1:31.64 INFO #01: <nsstring::nsCString as core::convert::From<alloc::vec::Vec<u8>>>::from (xpcom/rust/nsstring/src/lib.rs:0)
 1:31.64 INFO 2920 @0x7f34796535c0 (0 references; 0 from COMPtrs)
 1:31.64 INFO allocation stack:
 1:31.64 INFO #00: Gecko_IncrementStringAdoptCount (/home/chutten/gecko/xpcom/string/nsSubstring.cpp:321)
 1:31.64 INFO #01: <nsstring::nsCString as core::convert::From<alloc::vec::Vec<u8>>>::from (xpcom/rust/nsstring/src/lib.rs:0)
 1:31.64 INFO 2923 @0x7f34661c3060 (0 references; 0 from COMPtrs)
 1:31.64 INFO allocation stack:
 1:31.64 INFO #00: Gecko_IncrementStringAdoptCount (/home/chutten/gecko/xpcom/string/nsSubstring.cpp:321)
 1:31.64 INFO #01: <nsstring::nsCString as core::convert::From<alloc::vec::Vec<u8>>>::from (xpcom/rust/nsstring/src/lib.rs:0)
 1:31.64 INFO 2924 @0x7f347bcfb700 (0 references; 0 from COMPtrs)
 1:31.64 INFO allocation stack:
 1:31.64 INFO #00: Gecko_IncrementStringAdoptCount (/home/chutten/gecko/xpcom/string/nsSubstring.cpp:321)
 1:31.64 INFO #01: <nsstring::nsCString as core::convert::From<alloc::vec::Vec<u8>>>::from (xpcom/rust/nsstring/src/lib.rs:0)
 1:31.64 INFO 2922 @0x7f34796534b0 (0 references; 0 from COMPtrs)
 1:31.64 INFO allocation stack:
 1:31.64 INFO #00: Gecko_IncrementStringAdoptCount (/home/chutten/gecko/xpcom/string/nsSubstring.cpp:321)
 1:31.64 INFO #01: <nsstring::nsCString as core::convert::From<alloc::vec::Vec<u8>>>::from (xpcom/rust/nsstring/src/lib.rs:0)
 1:31.64 INFO 2925 @0x7f3479653590 (0 references; 0 from COMPtrs)
 1:31.64 INFO allocation stack:
 1:31.64 INFO #00: Gecko_IncrementStringAdoptCount (/home/chutten/gecko/xpcom/string/nsSubstring.cpp:321)
 1:31.64 INFO #01: <nsstring::nsCString as core::convert::From<alloc::vec::Vec<u8>>>::from (xpcom/rust/nsstring/src/lib.rs:0)

Comment 2

[Chris H-C :chutten]

Hey Jan-Erik, do you happen to remember how exactly we're supposed to clear out the adoption counter of the substrings of the nsCStrings we construct for event extra test_get_value? The counter gets incremented in nsCString::from(Vec<u8>) over here and only gets decremented when we release the data on things like ~nsTSubstring(), which I think should happen when the FfiRecordedEvent falls off the scope at the end of TestGetValue...right?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1774489 - Fix fog event test APIs to not leak. r=chutten

This is also simpler.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4754a2166292
Fix fog event test APIs to not leak. r=chutten

Comment 5

[Chris H-C :chutten]

I think this patch obviates the need for Jan-Erik's info

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/4754a2166292