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)

defect
Not set
normal

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)

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.
Attached patch bug1271866_aurora.patch (obsolete) — Splinter Review
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)
Attached patch bug1271866_beta.patch (obsolete) — Splinter Review
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)
Blocks: 1257699
Attachment #8751063 - Flags: review?(giles) → review+
Attachment #8751065 - Flags: review?(giles) → review+
Attached patch nightly patchSplinter Review
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+
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
Attached patch aurora patchSplinter Review
Patch for aurora.
Attachment #8751065 - Attachment is obsolete: true
Attachment #8751076 - Flags: review+
Attachment #8751076 - Attachment description: bug1271866_aurora.patch → aurora patch
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?
https://hg.mozilla.org/mozilla-central/rev/2b936bb98746
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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+
Is 47 affected?
(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.
The checkin create a file media/libnestegg/bug1271866.patch in both m-c and m-a. This seems to be wrong.
Flags: needinfo?(kinetik)
(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)
Matthew,

m-c is now ok but the patch file did make it to m-a also and is still there.
(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.