A typo snuck in with bug 1438134, [1], OverConstrainedError should be OverconstrainedError. This is js observable. We should probably be raising errors by type instead of string, or at least assert that they're as expected at runtime. I also wonder how it's possible we don't have tests for this in automation. [1] https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/dom/media/MediaManager.cpp#1302

Comment on attachment 8965628 [details] Bug 1452031 - Fix OverConstrainedError typo. https://reviewboard.mozilla.org/r/234470/#review240122

Comment on attachment 8965629 [details] Bug 1452031 - Add assert for current values of BaseMediaMgrError. https://reviewboard.mozilla.org/r/234472/#review240124 This is not a good idea. These asserts will only happen rarely when something else goes wrong, probably in the field rather than in testing. The real answer is to add mochitests for specific APIs and their errors, rather than this catch-all. If we want a catch-all, we should probably use c++ enums in the api here to catch it at compile time instead or adding runtime asserts. Note also we're not to spec with the type of any of these MediaStreamError errors. We'd need to this in mind in tests.

s/to this in mind/to keep this in mind/

There are some constraints tests in bug 1088621 that never landed, that would have caught this [1]. Could never get them green enough. Maybe things are better now? Unfortunately, they are severely bitrotted. They relied on hardcoded capabilities and altering the inheritance tree of the fake devices, which probably won't work anymore, since we removed most of that inheritance recently (otherwise a good thing). Once we implement resizeMode (bug 1433480) we might be able to test constraints a bit more, like in the known two-camera downscale case, though this still won't test the constraints algorithm. If we end up writing tests specifically for machines with real devices, as I suggest in bug 1441585 comment 7 then perhaps the above tests can be a template for tests of real devices (which is a bit harder since device capabilities are not a fixed input, but still seems doable, especially if we implement getCapabilities() (bug 1179084). [1] https://reviewboard.mozilla.org/r/67364/diff/9#index_header

> though this still won't test the constraints algorithm Because only cameras use the full-fledged constraints algorithm.

Comment on attachment 8965629 [details] Bug 1452031 - Add assert for current values of BaseMediaMgrError. I'm not attempting to solve the testing situation for applyConstraints. That should have been done before the feature landed. This does add a signal that something is wrong because it will give us crash stats in beta. A fairly simple investigation will reveal what is wrong. This is a distinct improvement over the status quo. If your prefer I can of course also land only the fix and skip the assert, but then we will still have _zero_ coverage of any similar future regressions, and that doesn't make sense to me.

I was responding to your slack message that applyConstraints doesn't seem adequately tested. "In Shutdown" is not a valid error code. I'd rather see us use enum here and have the compiler ensure we didn't miss anything than rely on reviews to do the same thing.

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #9) > I was responding to your slack message that applyConstraints doesn't seem > adequately tested. And I was responding to the r-. > "In Shutdown" is not a valid error code. I'd rather see us use enum here and > have the compiler ensure we didn't miss anything than rely on reviews to do > the same thing. "In Shutdown" simply reflects what's currently in the tree. Feel free to file a follow up to remove it.

Comment on attachment 8965629 [details] Bug 1452031 - Add assert for current values of BaseMediaMgrError. Added alternative patch as discussed in side channel. PTAL.

Comment on attachment 8965810 [details] Bug 1452031 - Use strong types for Media error names, fixing OverconstrainedError typo. https://reviewboard.mozilla.org/r/234648/#review240464 I like the approach. Some details to straighten out before r+ however. ::: dom/media/MediaManager.cpp:1405 (Diff revision 1) > if (!domStream || !stream || sHasShutdown) { > LOG(("Returning error for getUserMedia() - no stream")); > > if (auto* window = nsGlobalWindowInner::GetInnerWindowWithId(mWindowID)) { > RefPtr<MediaStreamError> error = new MediaStreamError(window->AsInner(), > - NS_LITERAL_STRING("InternalError"), > + MediaStreamError::Name::InternalError, It's somewhat confusing that in som places this is `MediaStreamError::Name`, and in some cases `MediaMgrError::Name`, while it is in fact declared and defined in `BaseMediaMgrError::Name`. Is there anything keeping us from consolidating these into one class? ::: dom/media/MediaManager.cpp:3673 (Diff revision 1) > - nsString errorMessage(NS_LITERAL_STRING("NotAllowedError")); > + MediaMgrError::Name errorName = MediaMgrError::Name::NotAllowedError; > > if (aSubject) { > nsCOMPtr<nsISupportsString> msg(do_QueryInterface(aSubject)); > MOZ_ASSERT(msg); > + nsString errorMessage; `errorMessage` is a confusing name since MediaStreamError contains a `mMessage` member. `errorMessage` seems to instead be the desired name of the error. ::: dom/media/MediaManager.cpp:3675 (Diff revision 1) > - if (errorMessage.IsEmpty()) > - errorMessage.AssignLiteral(u"InternalError"); > + errorName = (errorMessage.EqualsLiteral("NotFoundError")) > + ? MediaMgrError::Name::NotFoundError > + : MediaMgrError::Name::InternalError; Not sure how well used this path is but it looks to me like `errorMessage` could be of a lot more values than "NotFoundError", [1]. I think we need a proper mapping. [1] https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/browser/modules/ContentWebRTC.jsm#139,153 ::: dom/media/MediaStreamError.h:57 (Diff revision 1) > +private: > + const Name mName; Why private? If you could make mMessage const too, we don't need any protected or private members at all. It'd just be an immutable class, which by itself has many wins. ::: dom/media/MediaStreamError.cpp:26 (Diff revision 1) > - if (mName.EqualsLiteral("NotFoundError")) { > - mMessage.AssignLiteral("The object can not be found here."); > - } else if (mName.EqualsLiteral("NotAllowedError")) { > - mMessage.AssignLiteral("The request is not allowed by the user agent " > - "or the platform in the current context."); > - } else if (mName.EqualsLiteral("SecurityError")) { > - mMessage.AssignLiteral("The operation is insecure."); > - } else if (mName.EqualsLiteral("NotReadableError")) { > - mMessage.AssignLiteral("The I/O read operation failed."); > - } else if (mName.EqualsLiteral("InternalError")) { > - mMessage.AssignLiteral("Internal error."); > + CASE_MEDIAERR(AbortError, "The operation was aborted.") > + CASE_MEDIAERR(InternalError, "Internal error.") > + CASE_MEDIAERR(InvalidStateError, "The object is in an invalid state.") > + CASE_MEDIAERR(NotAllowedError, "The request is not allowed by the user agent " > + "or the platform in the current context.") > + CASE_MEDIAERR(NotFoundError, "The object can not be found here.") > + CASE_MEDIAERR(NotReadableError, "The I/O read operation failed.") > + CASE_MEDIAERR(NotSupportedError, "The operation is not supported.") > + CASE_MEDIAERR(OverconstrainedError, "Constraints could be not satisfied.") > + CASE_MEDIAERR(SecurityError, "The operation is insecure.") > + CASE_MEDIAERR(TypeError, "") I'm not a fan of the indirection these cases bring. Especially as they rely on an external like `aMessage`. IMO the overhead of using `mMessage.AssignLiteral` and `break;` is fine. Mapping it to a string can be moved to where it's needed.

Comment on attachment 8965811 [details] Bug 1452031 - InternalError is not a valid gUM error per spec. Use AbortError instead. https://reviewboard.mozilla.org/r/234650/#review240476 FWIW peer connection also has a bunch of InternalError uses: https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/dom/media/PeerConnection.js#1630-1646

(In reply to Andreas Pehrson [:pehrsons] from comment #15) > Comment on attachment 8965811 [details] > Bug 1452031 - InternalError is not a valid gUM error per spec. Use > AbortError instead. > > https://reviewboard.mozilla.org/r/234650/#review240476 > > FWIW peer connection also has a bunch of InternalError uses: > https://searchfox.org/mozilla-central/rev/ > 7ccb618f45a1398e31a086a009f87c8fd3a790b6/dom/media/PeerConnection.js#1630- > 1646 Wanna file a followup for this?

Comment on attachment 8965810 [details] Bug 1452031 - Use strong types for Media error names, fixing OverconstrainedError typo. https://reviewboard.mozilla.org/r/234648/#review240464 > It's somewhat confusing that in som places this is `MediaStreamError::Name`, and in some cases `MediaMgrError::Name`, while it is in fact declared and defined in `BaseMediaMgrError::Name`. > > Is there anything keeping us from consolidating these into one class? This seems like classic inheritance to me. The name resolutions used for constants matches the class being used in each instance, which seems predictable and correct (i.e. regardless of how or whether inheritance is used in the internal makeup of the class). > Is there anything keeping us from consolidating these into one class? Yes, MediaStreamError is cycle-collected, wrapper-cached, and main-thread only, for use by JS. MediaMgrError is none of those things and thread-safe, for use on the media thread. Mozilla c++ classes cannot be both thread-safe and cycle-collected AFAIK. > Not sure how well used this path is but it looks to me like `errorMessage` could be of a lot more values than "NotFoundError", [1]. I think we need a proper mapping. > > [1] https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/browser/modules/ContentWebRTC.jsm#139,153 The getUserMedia spec algorithm [2] does not allow returning arbitrary errors to JS. I'll added a comment to clarify. The mozGetUserMediaDevices() call in [1] is vestigial. It used to be where devices where enumerated, but nowadays this happens earlier, and this is just a cache lookup of that earlier work [3]. As such it cannot fail for any procedural reason other than internal error (AbortError). Even the "NotFoundError" happens ahead of UI now [4] and not here. I left it in, thinking maybe it would be within scope for UI code to further limit selection (though I don't believe any UI code does this today), thinking this would be benign. But comparing it to [4] I see we're missing a fingerprinting check here. This is making me change my mind about this being benign, so I think I'll rip out this "feature" and treat all UI errors here as internal (in a new patch). [2] https://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevices-getusermedia [3] https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/dom/media/MediaManager.cpp#3222 [4] https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/dom/media/MediaManager.cpp#2862-2863 > Why private? > > If you could make mMessage const too, we don't need any protected or private members at all. It'd just be an immutable class, which by itself has many wins. I like private by default, and even the constructor is protected, so I don't see the point of loosening encapsulation here. Making mMessage const would be doable, but would mean creating a helper function just for the constructor. Also, regardless of merit, cleaning up this class is not a goal imho, since it's non-spec, and therefore likely on the chopping block soon.

(In reply to Andreas Pehrson [:pehrsons] from comment #16) > Wanna file a followup for this? Filed bug 1452861.

Comment on attachment 8965810 [details] Bug 1452031 - Use strong types for Media error names, fixing OverconstrainedError typo. https://reviewboard.mozilla.org/r/234648/#review240464 > I like private by default, and even the constructor is protected, so I don't see the point of loosening encapsulation here. Making mMessage const would be doable, but would mean creating a helper function just for the constructor. Also, regardless of merit, cleaning up this class is not a goal imho, since it's non-spec, and therefore likely on the chopping block soon. It's `const` so there's nothing nasty an external user can do with it anyway. For the record I think immutability is great enough that if all it takes is a helper function we should go for it. Fair enough about the chopping block. Please file a followup for this, or link it. (re-opening this issue in mozreview for this)

Comment on attachment 8965810 [details] Bug 1452031 - Use strong types for Media error names, fixing OverconstrainedError typo. https://reviewboard.mozilla.org/r/234648/#review240926 ::: dom/media/MediaManager.cpp:3675 (Diff revisions 1 - 2) > - errorName = (errorMessage.EqualsLiteral("NotFoundError")) > + // The only errors other than NotAllowedError allowed by the getUserMedia > + // spec related to selection are NotFoundError for no valid options and > + // the catch-all AbortError for everything else (NotReadableError is > + // reserved for device startup errors later). > + errorName = (msgData.EqualsLiteral("NotFoundError")) > ? MediaMgrError::Name::NotFoundError > : MediaMgrError::Name::InternalError; It sounds like we should assert that `msgData` is "NotFoundError" or "InternalError" (to be "AbortError") here, so we don't hide errors from frontend code by silently masking them as InternalError. ::: dom/media/MediaStreamError.cpp:26 (Diff revisions 1 - 2) > - CASE_MEDIAERR(AbortError, "The operation was aborted.") > - CASE_MEDIAERR(InternalError, "Internal error.") > - CASE_MEDIAERR(InvalidStateError, "The object is in an invalid state.") > - CASE_MEDIAERR(NotAllowedError, "The request is not allowed by the user agent " > - "or the platform in the current context.") > - CASE_MEDIAERR(NotFoundError, "The object can not be found here.") > + Name mName; > + const char* mNameString; > + const char* mMessage; > + } map[] = { > + > +#define MAP_MEDIAERR(name, msg) { Name::name, #name, msg } Can you lift the #define outside of the array contents?

Comment on attachment 8966473 [details] Bug 1452031 - Close fingerprint concern by removing code no longer needed to handle NotFoundError spec case. https://reviewboard.mozilla.org/r/235156/#review240946 r- as I don't see how this helps against fingerprinting. ::: dom/media/MediaManager.cpp:3680 (Diff revision 1) > - ? MediaMgrError::Name::NotFoundError > - : MediaMgrError::Name::AbortError; > + // Treat all errors - even NotFoundError - from UI prompt code as an > + // internal error. The actual NotFoundError test happens before the UI > + // prompt code, and letting the UI prompt further limit selection is not > + // supported. We include the UI error in message for diagnostic purposes. Reiterating -- this needs an assert, so UI code can be fixed if it's misbehaving. ::: dom/media/MediaManager.cpp:3691 (Diff revision 1) > > nsString key(aData); > RefPtr<GetUserMediaTask> task; > mActiveCallbacks.Remove(key, getter_AddRefs(task)); > if (task) { > - task->Denied(errorName); > + task->Denied(errorName, errorMessage); If you add the original name as a message the fingerprinting surface remains. What's the point then?

Comment on attachment 8965810 [details] Bug 1452031 - Use strong types for Media error names, fixing OverconstrainedError typo. https://reviewboard.mozilla.org/r/234648/#review240464 > It's `const` so there's nothing nasty an external user can do with it anyway. > For the record I think immutability is great enough that if all it takes is a helper function we should go for it. > Fair enough about the chopping block. Please file a followup for this, or link it. (re-opening this issue in mozreview for this) Filed bug 1453013.

Comment on attachment 8965810 [details] Bug 1452031 - Use strong types for Media error names, fixing OverconstrainedError typo. https://reviewboard.mozilla.org/r/234648/#review240926 > It sounds like we should assert that `msgData` is "NotFoundError" or "InternalError" (to be "AbortError") here, so we don't hide errors from frontend code by silently masking them as InternalError. Removed code in third patch instead. PTAL.

Comment on attachment 8966473 [details] Bug 1452031 - Close fingerprint concern by removing code no longer needed to handle NotFoundError spec case. https://reviewboard.mozilla.org/r/235156/#review241180 try: -b do -p all -u mochitest-bc,mochitest-media,mochitest-e10s-bc,mochitest-media-e10s,web-platform-tests -t none

Comment on attachment 8966473 [details] Bug 1452031 - Close fingerprint concern by removing code no longer needed to handle NotFoundError spec case. https://reviewboard.mozilla.org/r/235156/#review241268 ::: dom/media/MediaManager.cpp (Diff revisions 1 - 2) > - MediaMgrError::Name errorName = MediaMgrError::Name::NotAllowedError; > - nsString errorMessage; > - > - if (aSubject) { > - nsCOMPtr<nsISupportsString> msg(do_QueryInterface(aSubject)); > - MOZ_ASSERT(msg); > - msg->GetData(errorMessage); > - // The only errors other than NotAllowedError allowed by the getUserMedia > - // spec related to selection are NotFoundError for no valid options and > - // the catch-all AbortError for everything else (NotReadableError is > - // reserved for device startup errors later). > - // > - // Treat all errors - even NotFoundError - from UI prompt code as an > - // internal error. The actual NotFoundError test happens before the UI > - // prompt code, and letting the UI prompt further limit selection is not > - // supported. We include the UI error in message for diagnostic purposes. > - errorName = MediaMgrError::Name::AbortError; > - } > - LGTM. But we should also fix `denyGUMRequest()` [1] in the frontend code so we don't fool ourselves into thinking that passing `aError` down has any effect. You already fixed the callers so it should be a simple task. Clearing the r? until such a fix is up. [1] https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/browser/modules/ContentWebRTC.jsm#238,240-243 ::: browser/modules/ContentWebRTC.jsm:151 (Diff revision 2) > > prompt(contentWindow, aSubject.windowID, aSubject.callID, > constraints, devices, secure, isHandlingUserInput); > }, > function(error) { > - // bug 827146 -- In the future, the UI should catch NotFoundError > + // These days, device enumeration is done ahead of handleGUMRequest, so "These days" will have no meaning in the future. I think you can leave that phrase out as it's implied that a comment is about the current state of affairs. ::: browser/modules/ContentWebRTC.jsm:206 (Diff revision 2) > requestTypes.push(sharingScreen ? "Screen" : "Camera"); > if (audioDevices.length) > requestTypes.push(sharingAudio ? "AudioCapture" : "Microphone"); > > if (!requestTypes.length) { > - denyGUMRequest({callID: aCallID}, "NotFoundError"); > + // These days, device enumeration is done ahead of handleGUMRequest, so Same here re "These days"

Comment on attachment 8966473 [details] Bug 1452031 - Close fingerprint concern by removing code no longer needed to handle NotFoundError spec case. https://reviewboard.mozilla.org/r/235156/#review241408

Pushed by jbruaroey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b12325c786ba Use strong types for Media error names, fixing OverconstrainedError typo. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/043493cdf0d5 InternalError is not a valid gUM error per spec. Use AbortError instead. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/abc6e8447dc0 Close fingerprint concern by removing code no longer needed to handle NotFoundError spec case. r=pehrsons

https://hg.mozilla.org/mozilla-central/rev/b12325c786ba https://hg.mozilla.org/mozilla-central/rev/043493cdf0d5 https://hg.mozilla.org/mozilla-central/rev/abc6e8447dc0