Closed
Bug 1271866
Opened 8 years ago
Closed 8 years ago
Rework the fix in bug 1257699 to avoid making bug 1261900 worse
Categories
(Core :: Audio/Video: Playback, defect)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox47 | --- | unaffected |
firefox48 | --- | fixed |
firefox49 | --- | fixed |
People
(Reporter: kinetik, Assigned: kinetik)
References
Details
Attachments
(2 files, 2 obsolete files)
1.97 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
5.03 KB,
patch
|
kinetik
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The original fix in bug 1257699 revealed (by prohibiting) a misuse of libnestegg's API that our MSE implementation relies on and which happens to work in some circumstances. Bug 1261900 covers fixing this case properly by altering libnestegg and the calling code to support the required usecase. This bug covers altering the original fix in bug 1257699 so that it fixes the NULL-deref discovered by the parser but continues to allow the API misuse that MSE was relying on. These patches are intended to be uplifted to aurora and beta.
Assignee | ||
Comment 1•8 years ago
|
||
This applies to nightly and aurora, and simply removes the NULL check, because the changes merged in from the blockgroup branch (764a4ca95f1f0b7713c42516970dd62177ec3c22 upstream) removed the dependence on ctx->ancestor from ne_read_block_additions and the other blockgroup handling functions.
Attachment #8751063 -
Flags: review?(giles)
Assignee | ||
Comment 2•8 years ago
|
||
This applies to beta and moves the NULL check from nestegg_read_packet down into the three functions that directly depend on ctx->ancestor. I'll apply these to the trees as a standalone patch rather than upstreaming them directly.
Attachment #8751065 -
Flags: review?(giles)
Updated•8 years ago
|
Attachment #8751063 -
Flags: review?(giles) → review+
Updated•8 years ago
|
Attachment #8751065 -
Flags: review?(giles) → review+
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20adb964d176
Assignee | ||
Comment 4•8 years ago
|
||
Updated update.sh and added .patch file; as pushed to try. I've confirmed that this applies to aurora as-is.
Attachment #8751063 -
Attachment is obsolete: true
Attachment #8751071 -
Flags: review+
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8751071 [details] [diff] [review] nightly patch Brainfart. This patch is for nightly only. The one I said was for beta is for aurora. beta hasn't got this change yet, so it doesn't need to be reverted there.
Attachment #8751071 -
Attachment description: bug1271866_aurora_v2.patch → nightly patch
Assignee | ||
Comment 6•8 years ago
|
||
Patch for aurora.
Attachment #8751065 -
Attachment is obsolete: true
Attachment #8751076 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8751076 -
Attachment description: bug1271866_aurora.patch → aurora patch
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8751076 [details] [diff] [review] aurora patch Approval Request Comment [Feature/regressing bug #]: bug 1257699 [User impact if declined]: potential playback stalls or errors with MSE WebM content (YouTube) [Describe test coverage new/current, TreeHerder]: tested manually [Risks and why]: very low risk, moves a NULL check closer to the place the pointer is used [String/UUID change made/needed]: none
Attachment #8751076 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b936bb987467f4ec7c8d7820dacf7096714da59 Bug 1271866 - Drop NULL check from nestegg_read_packet. r=giles
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2b936bb98746
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 10•8 years ago
|
||
Comment on attachment 8751076 [details] [diff] [review] aurora patch Additional fix aimed at 48. Can you verify the fix on aurora once it lands?
Attachment #8751076 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 11•8 years ago
|
||
Is 47 affected?
Updated•8 years ago
|
status-firefox47:
--- → ?
status-firefox48:
--- → affected
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10) > Comment on attachment 8751076 [details] [diff] [review] > aurora patch > > Additional fix aimed at 48. Can you verify the fix on aurora once it lands? Yep, I can verify. (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11) > Is 47 affected? No, bug 1257699 hadn't made it to 47 yet.
Assignee | ||
Updated•8 years ago
|
Comment 13•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/64c29da64cb8
Comment 14•8 years ago
|
||
The checkin create a file media/libnestegg/bug1271866.patch in both m-c and m-a. This seems to be wrong.
Flags: needinfo?(kinetik)
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to Frank-Rainer Grahl from comment #14) > The checkin create a file media/libnestegg/bug1271866.patch in both m-c and > m-a. This seems to be wrong. The fix landed on m-c also, so that's correct. But bug 1261900 reverted the update.sh change and forgot to remove the (now unused) patch file. I've pushed a removal now; thanks for spotting this.
Flags: needinfo?(kinetik)
Comment 16•8 years ago
|
||
Matthew, m-c is now ok but the patch file did make it to m-a also and is still there.
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Frank-Rainer Grahl from comment #16) > m-c is now ok but the patch file did make it to m-a also and is still there. That's fine, it's supposed to be there. It was just obsoleted on m-c because of bug 1261900 landing.
You need to log in
before you can comment on or make changes to this bug.
Description
•