1296988 - Update libnestegg to add2e904

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Matthew Gregan [:kinetik]]

Update libnestegg to 0d2d07ac to pick up several fuzzer fixes.  Also drags in an updated version of halloc.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93983467542a

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: bug1296988_v0.patch

Individual changes have already been reviewed, so r? is just for a general rubber stamp.

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: bug1296988_v1.patch

Small change: replace malloc.h include (which is non-standard) with stdlib.h to resolve build breakage on OS X and Windows.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bde484ddbdcb

Comment 3

[Ralph Giles (:rillian)]

Comment on attachment 8783395 [details] [diff] [review]
bug1296988_v1.patch

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

looks good. r=me.

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: bug1296988_v2.patch

Sorry to be a pain, but here's another version to review since I decided to rip halloc out as we don't need 99% of the functionality and removing it deals with issues like bug 1293762 (which has been fixed upstream in 1.2.3) and bug 1281295 (which is tricky to fix portably).

The replacement is crude, but it's not used in a performance critical path (e.g. a typical file might only allocate 2-4 items in this pool).

This allows allows us to remove the halloc-specific memory reporter we had.  This was only tracking the handful of metadata allocations and not the majority of the frame allocations made via the nestegg_read_packet path, so it was of very limited use.

Comment 5

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9510a447c761

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: bug1296988_v3.patch

There was a suspicious crash in free() in the last patch (which I don't think I caused; we'll see on the next try push), and while investigating with ASAN I discovered that the libnestegg changes for bug 1261900 and bug 1257726 introduced leaks.  This patch includes leak fixes for those regressions.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf811fd5175b

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8783809 [details] [diff] [review]
bug1296988_v3.patch

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

r=me with the in-tree halloc sources removed.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84b4b7e346b8
Update libnestegg.  r=rillian

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8783809 [details] [diff] [review]
bug1296988_v3.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1261900 and bug 1257726
[User impact if declined]: potential memory leaks with WebM MSE and EME playback
[Describe test coverage new/current, TreeHerder]: covered by existing tests
[Risks and why]: low risk, much of the changes are adding additional error handling/memory freeing on error paths
[String/UUID change made/needed]: none

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: bug1296988_v3.1.patch

Update with actual patch to land - only changes are deleting the halloc source from Gecko.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Wes, did this land on m-c yesterday?

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/84b4b7e346b8

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Good that we're fixing this potential for memory leaks. One regression is from 45 and one is from 49. Do we really need to uplift this to 49 beta 9? The release candidate build for 49 is in one week. I think it is ok for it to ride the trains or maybe uplift to aurora.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8784661 [details] [diff] [review]
bug1296988_v3.1.patch

It's probably ok to take this in 50 as its still in Aurora cycle.

Comment 15

[Matthew Gregan [:kinetik]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #13)
> Good that we're fixing this potential for memory leaks. One regression is
> from 45 and one is from 49. Do we really need to uplift this to 49 beta 9?
> The release candidate build for 49 is in one week. I think it is ok for it
> to ride the trains or maybe uplift to aurora.

Both of the regressions are in 49, so if we don't uplift we'll ship these leaks to release.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8784661 [details] [diff] [review]
bug1296988_v3.1.patch

Regression in 49, I see matthew's point that it is a regression we haven't shipped yet. Let's try this in beta 9 to prevent possible new memory leaks.

Comment 17

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/0721c01c5e73

Comment 18

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

https://hg.mozilla.org/releases/mozilla-beta/rev/8188fba7325b