1373250 - Intermittent LeakSanitizer | leak at AllocateStringCopy, ToNewCString, pref_savePrefs, mozilla::Preferences::WritePrefFile

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Treeherder Bug Filer]

Filed by: philringnalda [at] gmail.com

https://treeherder.mozilla.org/logviewer.html#?job_id=107220299&repo=autoland

https://queue.taskcluster.net/v1/task/ExwKM7oaQuqM5WcbjCWFYg/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Right, not properly freeing the memory when a new write request comes in before the old one is handled.  That would account for the intermittency.  Patch coming.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1373250: encapsulate all of the memory management in a single structure so that we don't have to do any manual freeing.

Review commit: https://reviewboard.mozilla.org/r/149464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149464/

Comment 3

[Benjamin Smedberg]

Attachment: Bug 1373250 alternate version: encapsulate all of the memory management in a single structure so that we don't have to do any manual freeing.

Review commit: https://reviewboard.mozilla.org/r/149494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149494/

Comment 4

[Benjamin Smedberg]

Comment on attachment 8878046 [details]
Bug 1373250: encapsulate all of the memory management in a single structure so that we don't have to do any manual freeing.

https://reviewboard.mozilla.org/r/149464/#review154054

Ugh, I didn't realize the weird memory management here between the UniquePtr and the contents. I've put up an alternate version which unifies the memory and I think is easier to read. Please let me know if you agree!

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8878083 [details]
Bug 1373250 alternate version: encapsulate all of the memory management in a single structure so that we don't have to do any manual freeing.

https://reviewboard.mozilla.org/r/149494/#review154078

::: modules/libpref/Preferences.cpp:289
(Diff revision 1)
> -      char*& pref = aPrefs[valueIdx];
> +      char* pref = prefptr.get();
>        MOZ_ASSERT(pref);
>        outStream->Write(pref, strlen(pref), &writeAmount);
>        outStream->Write(NS_LINEBREAK, NS_LINEBREAK_LEN, &writeAmount);
> -      free(pref);
>        pref = nullptr;

We don't need this line now, right?

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> Comment on attachment 8878046 [details]
> Bug 1373250: Properly free the strings from abandoned pref write.
> 
> https://reviewboard.mozilla.org/r/149464/#review154054
> 
> Ugh, I didn't realize the weird memory management here between the UniquePtr
> and the contents. I've put up an alternate version which unifies the memory
> and I think is easier to read. Please let me know if you agree!

Pushed to try.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8878046 [details]
Bug 1373250: encapsulate all of the memory management in a single structure so that we don't have to do any manual freeing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149464/diff/1-2/

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8878046 [details]
Bug 1373250: encapsulate all of the memory management in a single structure so that we don't have to do any manual freeing.

https://reviewboard.mozilla.org/r/149464/#review154150

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

The patches really got confusing at this point; the I just pushed is Benjamin's version with a build problem fixed, and a review comment addressed.  It's on try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cc67dba7d31

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8878046 [details]
Bug 1373250: encapsulate all of the memory management in a single structure so that we don't have to do any manual freeing.

https://reviewboard.mozilla.org/r/149464/#review154152

Comment 11

[Intermittent Failures Robot]

16 failures in 131 pushes (0.122 failures/push) were associated with this bug yesterday.   

Repository breakdown:
* autoland: 11
* mozilla-central: 3
* mozilla-inbound: 2

Platform breakdown:
* linux64: 16

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1373250&startday=2017-06-15&endday=2017-06-15&tree=all

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04baace29a6c
encapsulate all of the memory management in a single structure so that we don't have to do any manual freeing. r=milan

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Pulsebot from comment #12)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/04baace29a6c
> encapsulate all of the memory management in a single structure so that we
> don't have to do any manual freeing. r=milan

Just to be clear, this is Benjamin's patch, with a build failure fixed and the try above.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/04baace29a6c

Comment 15

[Intermittent Failures Robot]

24 failures in 814 pushes (0.029 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 15
* mozilla-central: 4
* mozilla-inbound: 3
* try: 2

Platform breakdown:
* linux64: 24

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1373250&startday=2017-06-12&endday=2017-06-18&tree=all