794354 - Valgrind on tbpl detects leak with mozilla::safebrowsing::Classifier::ApplyUpdates on the stack

Status: RESOLVED WORKSFORME

(Toolkit :: Safe Browsing, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: m-c changeset 08d435dedc7f Valgrind stack with line numbers

Valgrind detects a leak of 192 bytes (direct) with mozilla::safebrowsing::Classifier::ApplyTableUpdates on the stack, see attached snippet which comes from:

https://tbpl.mozilla.org/php/getParsedLog.php?id=15521837&tree=Firefox&full=1

Guessing Toolkit: General, please change component if necessary.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

mozilla::safebrowsing::HashStore::WriteFile is also on the stack, and mozilla::safebrowsing::Classifier::ApplyUpdates is lower down on the stack.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Is this with any specific workload or test? Or does it just always happen for regular runs?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #2)
> Is this with any specific workload or test? Or does it just always happen
> for regular runs?

It happens for PGO tests I think:

python _profile/pgo/profileserver.py --debugger=valgrind --debugger-args="$debugger_args" || exit 1

from http://hg.mozilla.org/build/tools/file/default/scripts/valgrind/valgrind.sh#l82

Comment 4

[Gian-Carlo Pascutto [:gcp]]

ApplyTableUpdates is supposed to delete all the TableUpdate* entries it consumes, but it looks reasonably obvious to me that that won't happen if there are failures while processing the update.

The comments in the code make it obvious this bug isn't unexpected. But for judging the urgency it would be nice to know if this happens during normal operation, or during failure cases.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

>It happens for PGO tests I think

I dunno what those are but if I read that profileserver.py correctly, it just loads index.html, which loads 4 webpages and some JS code?

That shouldn't be a failure case, in which case we're leaking in normal operation.

:-(

Comment 6

[Jesse Ruderman]

The PGO build might shut down before the download finishes, or it might be completely unable to contact the server.  Would either of those scenarios trigger the leak?

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Perhaps yes. I ran valgrind with --leak-check=all on current m-c and didn't get any warnings or leaks inside the safebrowsing stuff, during a normal run that received updates.

Given that we only seem to trigger this when things are already hosed, reducing the severity. We'll want to fix this but it looks like it wont leak during normal operation.

Comment 8

[Nicholas Nethercote [inactive]]

This is no longer occurring in Valgrind-on-TBPL runs.