Closed Bug 1296988 Opened 4 years ago Closed 4 years ago

Update libnestegg to add2e904

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: kinetik, Assigned: kinetik)

References

Details

Attachments

(1 file, 4 obsolete files)

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
Attached patch bug1296988_v0.patch (obsolete) — Splinter Review
Individual changes have already been reviewed, so r? is just for a general rubber stamp.
Attachment #8783382 - Flags: review?(giles)
Summary: Update libnestegg to 0d2d07ac → Update libnestegg to f522e4f4
Attached patch bug1296988_v1.patch (obsolete) — Splinter Review
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
Attachment #8783382 - Attachment is obsolete: true
Attachment #8783382 - Flags: review?(giles)
Attachment #8783395 - Flags: review?(giles)
Comment on attachment 8783395 [details] [diff] [review]
bug1296988_v1.patch

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

looks good. r=me.
Attachment #8783395 - Flags: review?(giles) → review+
Summary: Update libnestegg to f522e4f4 → Update libnestegg to add2e904
Attached patch bug1296988_v2.patch (obsolete) — Splinter Review
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.
Attachment #8783395 - Attachment is obsolete: true
Attachment #8783763 - Flags: review?(giles)
Blocks: 1281295
Attached patch bug1296988_v3.patch (obsolete) — Splinter Review
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
Attachment #8783763 - Attachment is obsolete: true
Attachment #8783763 - Flags: review?(giles)
Attachment #8783809 - Flags: review?(giles)
Blocks: 1261900, 1257726
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.
Attachment #8783809 - Flags: review?(giles) → review+
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
Attachment #8783809 - Flags: approval-mozilla-beta?
Attachment #8783809 - Flags: approval-mozilla-aurora?
Update with actual patch to land - only changes are deleting the halloc source from Gecko.
Attachment #8783809 - Attachment is obsolete: true
Attachment #8783809 - Flags: approval-mozilla-beta?
Attachment #8783809 - Flags: approval-mozilla-aurora?
Attachment #8784661 - Flags: approval-mozilla-beta?
Attachment #8784661 - Flags: approval-mozilla-aurora?
Wes, did this land on m-c yesterday?
Flags: needinfo?(wkocher)
https://hg.mozilla.org/mozilla-central/rev/84b4b7e346b8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
Flags: needinfo?(kinetik)
Attachment #8784661 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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.
Attachment #8784661 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Flags: needinfo?(kinetik) → needinfo?(lhenry)
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.
Flags: needinfo?(lhenry)
Attachment #8784661 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.