EME is throwing DOMExceptions using nsresults
Categories
(Core :: Audio/Video: Playback, defect, P3)
Tracking
()
People
(Reporter: bzbarsky, Unassigned)
References
(Blocks 1 open bug)
Details
It's not like there was a choice before, but now there is. In bug 1612007 and bug 1612213 I am adding APIs to allow throwing DOMExceptions using APIs like ThrowInvalidStateError without nsresults involved at all. This is pretty important because I want to remove the general ability to throw nsresults to web content.
I mostly managed to get rid of existing ThrowDOMException consumers, except for one: DetailedPromise::MaybeReject(nsresult, const nsACString&). The only consumers of that I haven't gotten rid of are MediaKeys::RejectPromise and HTMLMediaElement::SetCDMProxyFailure. Both seem to do with EME stuff.
For the latter, I have a MediaResult and absolutely no guarantees that the nsresult in it is even valid for a DOMException. Auditing suggests it might be, but it's hard to tell.
For MediaKeys::RejectPromise there are tons of callsites. Some of those come through MediaResult and some come through other things.
I'm open to suggestions for how this could work. One option would be to create an ErrorResult at the point where the error is first generated and propagate it through, but that might not play that nice with MozPromise, because you can't copy ErrorResults. If our MozPromise instances are exclusive, we could pass things by rvalue ref and then that would work. Or if we had to we could use CopyableErrorResult.
The other option would be to be able to store a DetailedPromise member method in the MediaResult and then we could just call it on the receiving end. But I'm not sure whether this stuff gets serialized over IPC, say. If it does, CopyableErrorResult might be the way to go.
Thoughts? Unfortunately, it looks like the authors and reviewers of this code are no longer active...
|
||
Comment 1•6 years ago
|
||
Redirecting to Bryce, as my responsibilities have shifted in the past week.
Acking that I've seen this. Holding NI to remind me to look again after I've cleared backlog from all hands.
I think bug 1615035 fixed this in terms of us using a better error type.
|
Reporter | |
Comment 4•5 years ago
|
||
No, this is a different issue. That issue was about using NS_ERROR_DOM_TYPE_ERR instead of ThrowTypeError. This one is about the callsites described in comment 0 that end up landing in ThrowDOMException: DetailedPromise::MaybeReject(nsresult, const nsACString&), called from MediaKeys::RejectPromise and HTMLMediaElement::SetCDMProxyFailure.
Thanks for the clarification. Could you clarify further? Is my understanding correct that bug 1615035 is cleaning up the error types, but that it's still difficult to trace the different paths that end up creating errors near the browser surface so we can't tidy that up (which this bug tracks)? Is MediaKeys::RejectPromise still a concern now that it uses an ErrorResult[0]?
If so, then does that leave the results that go via MediaResult to be handled?
|
|
Reporter | |
Comment 6•5 years ago
|
||
The main point is that we wanted to decouple throwing from nsresult in the API, so that all we need for throwing is the kind of exception to throw and a message.
MediaKeys::RejectPromise is not a concern now that is uses ErrorResult, good catch. Looks like that got fixed in bug 1615035. So all that's left is HTMLMediaElement::SetCDMProxyFailure. If we can switch that to ErrorResult (e.g. by having an ErrorResult in MediaResult), then we should be able to remove the following methods:
DetailedPromise::MaybeReject(nsresult aArg, const nsACString& aReason)Promise::MaybeRejectWithDOMException
and TErrorResult::ThrowDOMException can then become private, I expect. Then the underlying work to decouple exceptions from nsresults can keep happening without any more API changes.
Thanks for the further clarification.
|
||
Comment 8•3 years ago
|
||
Unassigning bugs assigned to Bryce because he no longer works at Mozilla.
Description
•