decodeAudioData is returning null without throwing, but claims to never return null

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bzbarsky, Assigned: padenot)

Tracking

({regression})

unspecified
mozilla37
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36+ fixed, firefox37+ fixed)

Details

Attachments

(1 attachment)

I missed this when reviewing bug 1000264.  :(

This code:

463   promise = Promise::Create(parentObject, rv);
464   if (rv.Failed()) {
465     return nullptr;
466   }

should probably be using a passed-in ErrorResult, not one we declared on the stack.  Or the method should be returning a nullable promise or something, but I suspect we want to use the passed-in ErrorResult.
Paul can you confirm whether this affects 37/36 also would you be able to take this and work on it?
Flags: needinfo?(padenot)
Yes and yes.
Flags: needinfo?(padenot)
Assignee: nobody → padenot
Boris, is this what you had in mind?
Attachment #8543949 - Flags: review?(bzbarsky)
Comment on attachment 8543949 [details] [diff] [review]
Make AudioContext.decodeAudioData throw if we can't create a promise. r=

> +    aRv.Throw(NS_ERROR_UNEXPECTED);

Not needed.  aRv is already in a failed state.

r=me with that line removed.
Attachment #8543949 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/c4ea7a2518fc
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Paul, do you think we should uplift that to 36?
Flags: needinfo?(padenot)
I'm actually not sure how important this is, I merely followed Boris' instructions.

Boris, do you think we should uplift this?
Flags: needinfo?(padenot) → needinfo?(bzbarsky)
I think it's a good idea to uplift this, yes.  It's a very simple fix, and fixes an issue that could be a security problem in the wrong circumstances.
Flags: needinfo?(bzbarsky)
Comment on attachment 8543949 [details] [diff] [review]
Make AudioContext.decodeAudioData throw if we can't create a promise. r=

Approval Request Comment
[Feature/regressing bug #]: 1000264
[User impact if declined]: possible security issue under the wrong circumtances
[Describe test coverage new/current, TreeHerder]: no tests, been landed for some time, made the 37 uplift
[Risks and why]: Low risk: this is a pattern that is used everywhere in the tree
[String/UUID change made/needed]: none
Attachment #8543949 - Flags: approval-mozilla-beta?
Attachment #8543949 - Flags: approval-mozilla-aurora?
Comment on attachment 8543949 [details] [diff] [review]
Make AudioContext.decodeAudioData throw if we can't create a promise. r=

Right, it made the uplift, so, no need to have this in aurora :)
Attachment #8543949 - Flags: approval-mozilla-beta?
Attachment #8543949 - Flags: approval-mozilla-beta+
Attachment #8543949 - Flags: approval-mozilla-aurora?
Attachment #8543949 - Flags: approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.