The peerIdentity constraint causes the constrained streams to enter a mode where they can't be sent unless the identity of the remote peer matches the constraint. This is far better understood currently than the trickier (and in jeopardy) noaccess constraint.

This adds support for a peerIdentity attribute on the RTCConfiguration object and a matching attribute on the getUserMedia argument. Both cause the ownership of the corresponding object to change, from the current page origin to a new construct (a PeerIdentityPrincipal, backed by a PeerIdentityUri). This causes any attempt by the site to access the content to fail. I've made it so that streams cannot be added to peerconnection when there is an ownership mismatch (this currently throws, which is bad, we need a callback here...). Also, any stream generated by peerconnection will inherit its ownership. The net effect is that a site will be able to create a call where the content is not accessible to the site on either end. There are some minor deviations from the design described in the specs, but I'll be sending emails and pull requests to fix those up shortly. I don't expect any of those to be controversial.

OK, this is good: https://tbpl.mozilla.org/?tree=Try&rev=9ec45713f1cf https://tbpl.mozilla.org/?tree=Try&rev=67c17606325d

IDL changes, which are just now merged into the main spec: https://github.com/fluffy/webrtc-w3c/pull/10

Creation of a new principal for peer identity.

Media pipeline needs to be turned off when principals do not align.

Here's the final ball of wax that relies on the principal.

(Adding :bz and :bholley at :jst's recommendation for principal-related review. Also :roc.) This patch adds a new type of nsIPrincipal that is specifically used for isolating media streams in peer-to-peer scenarios. The intent here is to mark DOMMediaStream instances with a principal that is not subsumed by the document principal. These can be displayed, but not examined (i.e., drawn to a canvas and sampled, or piped to web audio). There's some dynamic stuff here that isn't complete, since principals on DOMMediaStream change. That is being completed in Bug 966066 (which I have completed part of).

Comment on attachment 8401460 [details] [diff] [review] 0001-Bug-942367-IDL-changes-for-peerIdentity-constraint.patch Review of attachment 8401460 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nit. ::: dom/webidl/MediaStreamTrack.webidl @@ +34,4 @@ > (boolean or object) video = false; // (boolean or MediaTrackConstraints) > boolean picture = false; > boolean fake = false; > + DOMString? peerIdentity = null; Why distinguish between null and ""? I think DOMString peerIdentity = "" would be preferable.

Comment on attachment 8401463 [details] [diff] [review] 0003-Bug-942367-Adding-ability-to-disable-on-the-MediaPip.patch Review of attachment 8401463 [details] [diff] [review]: ----------------------------------------------------------------- lgtm. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp @@ +662,5 @@ > + if (NS_FAILED(rv)) { > + enableStream = false; > + MOZ_MTLOG(ML_NOTICE, "Unable to subsume with " > + << static_cast<void*>(principal) << " over " > + << static_cast<void*>(domstream_->GetPrincipal())); I see this is prevalent, but isn't part of the charm of << supposed to be that we don't have to chokehold data anymore? @@ -661,4 @@ > // Should not be set for a transmitter > MOZ_ASSERT(!possible_bundle_rtp_); > if (&info == &rtp_) { > - // TODO(ekr@rtfm.com): Move onto MSG thread. Gave up on MSG thread?

(In reply to Jan-Ivar Bruaroey [:jib] from comment #14) > ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp > @@ +662,5 @@ > > + if (NS_FAILED(rv)) { > > + enableStream = false; > > + MOZ_MTLOG(ML_NOTICE, "Unable to subsume with " > > + << static_cast<void*>(principal) << " over " > > + << static_cast<void*>(domstream_->GetPrincipal())); > > I see this is prevalent, but isn't part of the charm of << supposed to be > that we don't have to chokehold data anymore? You got me, I'm just blindly copying here. Didn't think it through. I originally thought it was an explicit coercion of nsCOMPtr or nsRefPtr, but that's not right, because all the uses I see of static_cast<void*> are from bare pointers. > @@ -661,4 @@ > > // Should not be set for a transmitter > > MOZ_ASSERT(!possible_bundle_rtp_); > > if (&info == &rtp_) { > > - // TODO(ekr@rtfm.com): Move onto MSG thread. > > Gave up on MSG thread? Look at the declaration of the type used below, I'm using Atomic<bool> to cover the thread crossing issue.

> I think DOMString peerIdentity = "" would be preferable. That would treat null as "null". I doubt that's what you want if you want "" and null to be treated the same.

(In reply to Boris Zbarsky [:bz] from comment #16) > > I think DOMString peerIdentity = "" would be preferable. > > That would treat null as "null". I doubt that's what you want if you want > "" and null to be treated the same. Indeed, and while "" might be invalid, there's a difference between invalid and not set that I want to maintain in this case.

I'm confused. "Not set" in JavaScript would be undefined, not null, no? DOMString peerIdentity = ""; will set the value to "" if not set or if explicitly set to undefined.

> The intent here is to mark DOMMediaStream instances with a principal that is not subsumed > by the document principal. Is there a summary somewhere of why expanded principals and null principals do not fit the bill, just to make sure I understand the problem space?

(In reply to Martin Thomson [:mt] from comment #15) > > Gave up on MSG thread? > > Look at the declaration of the type used below, I'm using Atomic<bool> to > cover the thread crossing issue. Makes sense. Good to know that was the lone reason a move to MSG was considered. (In reply to Boris Zbarsky [:bz] from comment #18) > I'm confused. "Not set" in JavaScript would be undefined, not null, no? > > DOMString peerIdentity = ""; > > will set the value to "" if not set or if explicitly set to undefined. Thanks, that's my point that I failed to express cogently. Us c++ guys reach for null to mean "not passed", when this seems superfluous in js. I don't know what best practice is on defending against unexpected null on string inputs, and I yield to bz on anything js anyways. ;-)

(In reply to Martin Thomson [:mt] from comment #17) > Indeed, and while "" might be invalid, there's a difference between invalid > and not set that I want to maintain in this case. What's the harm in treating "" as unset?

(In reply to Boris Zbarsky [:bz] from comment #19) > > The intent here is to mark DOMMediaStream instances with a principal that is not subsumed > > by the document principal. > > Is there a summary somewhere of why expanded principals and null principals > do not fit the bill, just to make sure I understand the problem space? Yes, please. In general, it's good practice to discuss changes like this with the module owner beforehand. I'm not really wild about having a special principal here - the rest of the machinery is quite generic and reusable.

(In reply to Bobby Holley (:bholley) from comment #22) > (In reply to Boris Zbarsky [:bz] from comment #19) > > > The intent here is to mark DOMMediaStream instances with a principal that is not subsumed > > > by the document principal. > > > > Is there a summary somewhere of why expanded principals and null principals > > do not fit the bill, just to make sure I understand the problem space? > > Yes, please. In general, it's good practice to discuss changes like this > with the module owner beforehand. I'm not really wild about having a special > principal here - the rest of the machinery is quite generic and reusable. I won't lie, this is largely the result of me flailing around looking for something that works. This works, but I'm not wed to it, hence the request for discussion (yeah, the patches compile and run, but I can change code). In theory at least a nsPrincipal carrying a custom URI, in combination with the doc principal, both housed in an nsExtendedPrincipal would do the job. The only concern I have now is this line here: http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1940 That means that the custom URI would have to be an instance of nsIStandardURL rather than just nsIURI. I don't know if nsStandardURL would work here, but I'm willing to give it a try. Would that work better?

Could we take another step back for one second? What security properties do we actually need here? Nothing in this bug explains that; I'd love such an explanation or link to such an explanation. Basically, before we try to figure out whether this patch is the right solution or what the right solution is I need to know what problem we're trying to solve.

Happy to do so. The intent of this is to protect streams from applications so that they can only a) be displayed, and b) be sent to a specific peer using RTCPeerConnection See http://dev.w3.org/2011/webrtc/editor/getusermedia.html#isolated-media-streams My intent with using principals was to provide this protection.

So, in the interests of an experiment, I went ahead and tried to remove the principal and just use a combination of nsPrincipal and nsExtendedPrincipal. Seems to be OK, see attached (this builds on previous for my benefit, but I'll clean things up if this makes sense to pursue). Looking into the URI reuse situation, I'm still a little nervous about that. It's probably OK, but nsStandardURL does some things like adding "://" that don't fill me with confidence.

Martin, thanks for the link. That link sounds like just giving the MediaStream a nonce origin (a nsNullPrincipal in Gecko) would give the desired behavior: it would not be subsumed by the document. Can you explain why this is not sufficient? In particular, why do we need the PeerIdentityURI bits, which are presumably why we can't use a nullprincipal here?

That would require a custom attribute on the stream so that we could actually send it over RTCPeerConnection. That's possible.

Is the point that deciding whether to allow a MediaStream to be sent over an RTCPeerConnection needs the URI in question?

That's correct, though it's more the case that if a MediaStream is bound to "x@example.com", then only an RTCPeerConnection bound to "x@example.com" can be used. (A mismatch causes blackness/silence to be sent in place of the actual content.)

I see. So we were at least considering encapsulating the information about what a MediaStream is bound to in its principal?

That's right. It seemed like that is the function of a principal. I would have said the primary function, but then I've actually looked at the code.

The primary function is hard to pin down, since it shifted a lot. Usually the idea is to encapsulate the origin something is from (conceptually, at least), plus some other bits. I take it that in this case the "who you are bound to" bit is a URI but has no authority section?

Comment on attachment 8401467 [details] [diff] [review] 0004-Bug-942367-Adding-support-for-peerIdentity-constrain.patch Review of attachment 8401467 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits and a few questions. A DOM reviewer should probably look over the JS as well. I also don't know the Dtls stuff very well. ::: dom/media/MediaManager.cpp @@ +169,5 @@ > // Only run if the window is still active. > NS_ASSERTION(NS_IsMainThread(), "Only call on main thread"); > > + nsCOMPtr<nsIDOMGetUserMediaSuccessCallback> successCb = mSuccess.forget(); > + nsCOMPtr<nsIDOMGetUserMediaErrorCallback> errorCb = mError.forget(); I think we generally try to avoid touching code just for cleanup and renaming reasons, to preserve blame. ::: dom/media/PeerConnection.js @@ +687,5 @@ > + let good = !!message; > + // This might be a valid assertion, but if we are constrained to a single peer > + // identity, then we also need to make sure that the assertion matches > + if (good && this._impl.peerIdentity) { > + good = message.identity === this._impl.peerIdentity; Some parentheses would make me feel more comfortable here. @@ +705,5 @@ > + let idpError = null; > + > + // we can run the IdP validation in parallel with setRemoteDescription this > + // complicates much more than would be ideal, but it ensures that the IdP > + // doesn't hold things up too much when it's not on the critical path Pity we don't have promises for this. @@ +720,5 @@ > + > + let setRemoteDone = function() { > + setRemoteComplete = true; > + allDone(); > + }.bind(this); bind is not needed on this one. @@ +732,5 @@ > + } else { > + idpDone = function(message) { > + let idpGood = this._processIdpResult(message); > + if (!idpGood) { > + // iff we are waiting for a very specific peerIdentity typo ::: dom/media/tests/identity/idp-proxy.js @@ +3,5 @@ > > function IDPJS() { > this.domain = window.location.host; > + var p = window.location.pathname; > + this.protocol = p.substring(p.lastIndexOf('/') + 1) + window.location.hash; Manual url parsing is usually fraught with issues, but in this case I trust the window.location is not a content window or otherwise modifiable from content? ::: dom/media/tests/identity/test_peerConnection_peerIdentity.html @@ +45,5 @@ > + peerIdentity: id1 > + }]); > + test.setIdentityProvider(test.pcLocal, 'test1.example.com', 'idp.html'); > + test.setIdentityProvider(test.pcRemote, 'test2.example.com', 'idp.html'); > + test.chain.append([ Doesn't this run all the other tests first? Are they all necessary to run again for this, or could some be skipped? ::: dom/media/tests/mochitest/blacksilence.js @@ +30,5 @@ > + clearInterval(interval); > + interval = null; > + done(); > + } > + }, 50); Maybe something divisible by 15 ms for Windows' sake? I also worry 50 ms is too tight for some of the builders, and might cause intermittents ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp @@ +173,5 @@ > CC_CallFeature_FoundICECandidate(vcm_opaque->call_handle_, > + candidate_tmp, > + nullptr, > + vcm_opaque->level_, > + nullptr); Splinter says there are lots of whitespace-only changes in this file, yet I see no difference. This should be undone to preserve blame. @@ +2227,5 @@ > + */ > +static int vcmTxCreateAudioConduit(int level, > + const vcm_payload_info_t *payload, > + sipcc::PeerConnectionWrapper &pc, > + vcm_mediaAttrs_t *attrs, maybe const? @@ +2344,5 @@ > + short tos, > + sdp_setup_type_e setup_type, > + const char *fingerprint_alg, > + const char *fingerprint, > + vcm_mediaAttrs_t *attrs) I would refrain from whitespace only changes to preserve blame. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +554,4 @@ > #ifdef MOZILLA_INTERNAL_API > + nsCOMPtr<nsIPrincipal> systemPrincipal; > + nsCOMPtr<nsIScriptSecurityManager> securityManager = > + do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID); Should check NS_SUCCEEDED(rv) && securityManager I think @@ +560,5 @@ > + stream->CombineWithPrincipal(systemPrincipal); > + > + // Make the stream data (audio/video samples) accessible to the receiving page, > + // or bind it to the peer identity, if that is enabled. > + if (PrivacyRequested()) { if (mPrivacyRequested) ? This may be a style thing. I'm generally not a fan of classes "APIing" themselves just for the sake of it e.g. indirecting their own member access. I prefer stark parent-child encapsulation. That's why we have private. Just my 2 cents. @@ +754,5 @@ > mWindow = aWindow; > NS_ENSURE_STATE(mWindow); > > + mPeerIdentity = aRTCConfiguration->mPeerIdentity; > + mPrivacyRequested = !mPeerIdentity.IsVoid(); // null check Why not use IsEmpty() instead of IsVoid() throughout here? What happens if empty string is passed in? @@ +755,5 @@ > NS_ENSURE_STATE(mWindow); > > + mPeerIdentity = aRTCConfiguration->mPeerIdentity; > + mPrivacyRequested = !mPeerIdentity.IsVoid(); // null check > + nsCOMPtr<nsPIDOMWindow> window(do_QueryInterface(mWindow)); window != nullptr? Some code checks some doesn't... @@ +757,5 @@ > + mPeerIdentity = aRTCConfiguration->mPeerIdentity; > + mPrivacyRequested = !mPeerIdentity.IsVoid(); // null check > + nsCOMPtr<nsPIDOMWindow> window(do_QueryInterface(mWindow)); > + > + mPrincipal = window->GetExtantDoc()->NodePrincipal(); Check doc? @@ +1280,5 @@ > mTimeCard = nullptr; > STAMP_TIMECARD(tc, "Set Local Description"); > > +#ifdef MOZILLA_INTERNAL_API > + nsIPrincipal* scriptPrincipal = GetWindow()->GetExtantDoc()->NodePrincipal(); Can any of these fail? You check doc against nullptr elsewhere. @@ +1404,5 @@ > return NS_OK; > } > > +nsresult > +PeerConnectionImpl::SetPeerIdentity(const nsAString& peerIdentity) aPeerIdentity @@ +1409,5 @@ > +{ > + PC_AUTO_ENTER_API_CALL(true); > + > + // once set, this can't be changed > + if (!mPeerIdentity.IsVoid()) { What happens if passed-in aPeerIdentity.IsVoid()? Then the comment is wrong if nothing else. @@ +1435,5 @@ > + // Besides, this is only used to say if we have been connected ever. > + mDtlsConnected = true; > +#ifdef MOZILLA_INTERNAL_API > + if (!PrivacyRequested()) { > + if (aPrivacyRequested && !mPeerIdentity.IsVoid()) { So empty string is a valid identity? @@ +1440,5 @@ > + // if we previously didn't know, time to update stream principals > + mMedia->UpdateRemoteStreamPrincipals_m(mPrincipal); > + } > + } else { > + // now we know that privacy isn't needed for sure I don't understand this comment. Privacy isn't needed when PrivacyRequested()? @@ +1449,5 @@ > + CSFLogInfo(logTag, "Can't update principal on streams; window gone"); > + } > + } > +#endif > + mPrivacyRequested = mPrivacyRequested || aPrivacyRequested; When mPrivacyRequested is false and doc is null above then mPrivacyRequested is set here without UpdateRemoteStreamPrincipals_m being called. Is that expected? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ +367,5 @@ > NS_DISPATCH_NORMAL); > } > > +void > +PeerConnectionMedia::SelfDestruct_m() Seems like this function moved for no reason. Maybe restore to preserve blame? @@ +541,5 @@ > + dtlsLayer->SignalStateChange.disconnect(this); > + > + bool privacyRequested = false; > + // TODO (Bug 952678) set privacy mode, ask the DTLS layer about that > + GetMainThread()->Dispatch( mMainThread? @@ +555,5 @@ > + > + MOZ_ASSERT(!mTransportFlows[index_inner]); > + mTransportFlows[index_inner] = aFlow; > + > + GetSTSThread()->Dispatch( mSTSThread? @@ +581,5 @@ > + * @param scriptPrincipal the principal > + * @returns true if any stream is isolated > + */ > +bool > +PeerConnectionMedia::LocalStreamsIsolated(nsIPrincipal *scriptPrincipal) const Maybe AnyLocalStreamsIsolated? @@ +585,5 @@ > +PeerConnectionMedia::LocalStreamsIsolated(nsIPrincipal *scriptPrincipal) const > +{ > + ASSERT_ON_THREAD(mMainThread); > + > + for (uint32_t u = 0; u < mLocalSourceStreams.Length(); u++) { Rest of file uses for (size_t i @@ +610,5 @@ > +LocalSourceStreamInfo::UpdateSinkPrincipal_m(nsIPrincipal* aPrincipal) > +{ > + for (auto it = mPipelines.begin(); it != mPipelines.end(); ++it) { > + mozilla::RefPtr<mozilla::MediaPipelineTransmit> pipeline( > + static_cast<mozilla::MediaPipelineTransmit*>((*it).second.get())); Why bypass virtual? Also, I don't think you need to addref here. ::: media/webrtc/signaling/test/FakeMediaStreams.h @@ +90,4 @@ > protected: > std::set<Fake_MediaStreamListener *> mListeners; > mozilla::Mutex mMutex; // Lock to prevent the listener list from being modified while > + // executing Periodic(). whitespace only change? ::: media/webrtc/signaling/test/mediapipeline_unittest.cpp @@ -925,5 @@ > return rv; > } > - > - > - eof-space.

(In reply to Martin Thomson [:mt] from comment #28) > That would require a custom attribute on the stream so that we could > actually send it over RTCPeerConnection. That's possible. Yeah, I think this should live on the stream rather than the principal. Principals are a very generic and fundamental mechanism, and we're trying to keep extraneous stuff out of them as much as possible. For example, CSP currently lives on the principal, but we're finally fixing that. Are there any downsides to doing it that way?

(In reply to Boris Zbarsky [:bz] from comment #33) > The primary function is hard to pin down, since it shifted a lot. Usually > the idea is to encapsulate the origin something is from (conceptually, at > least), plus some other bits. > > I take it that in this case the "who you are bound to" bit is a URI but has > no authority section? Yes, but only in the strictest interpretation of RFC 3986. The URI takes the same form as a "mailto" or "sip" URI, e.g., "some-scheme:user@host.example.com". Technically, the domain in those URIs is part of the scheme-specific path component. I always find the rationalization in RFC 3986 amusing. (In reply to Bobby Holley (:bholley) from comment #35) > (In reply to Martin Thomson [:mt] from comment #28) > > That would require a custom attribute on the stream so that we could > > actually send it over RTCPeerConnection. That's possible. > > Yeah, I think this should live on the stream rather than the principal. > Principals are a very generic and fundamental mechanism, and we're trying to > keep extraneous stuff out of them as much as possible. For example, CSP > currently lives on the principal, but we're finally fixing that. > > Are there any downsides to doing it that way? It significantly changes the way I've been approaching the problem, but that's mostly just typing. And testing. If that's the guidance, then that's what I'll do. It will probably be less code in the end. Implementing nsIURI generated a fair bit of dead code.

Yeah, those URIs really don't match what the rest of the web thinks of as an "origin". I think we should probably have two separate pieces of information on the stream. A principal which is used by most of the system and that we can set to nullprincipal to make it taint canvases and whatnot, and a separate string or URI for the thing it's bound to that's used in that one spot.

(In reply to Boris Zbarsky [:bz] from comment #37) > I think we should probably have two separate pieces of information on the > stream. A principal which is used by most of the system and that we can set > to nullprincipal to make it taint canvases and whatnot, and a separate > string or URI for the thing it's bound to that's used in that one spot. Awesome. I'll do that. It shouldn't be too hard to cobble something together.

Good feedback. I've addresses these on my branch. Now I need to deal with the bigger issue of principal use. (In reply to Jan-Ivar Bruaroey [:jib] from comment #34) > > + nsCOMPtr<nsIDOMGetUserMediaSuccessCallback> successCb = mSuccess.forget(); > > + nsCOMPtr<nsIDOMGetUserMediaErrorCallback> errorCb = mError.forget(); > > I think we generally try to avoid touching code just for cleanup and > renaming reasons, to preserve blame. I should have reverted this; there's an unused variable there, but I'll leave that alone. > ::: dom/media/PeerConnection.js > @@ +687,5 @@ > > + let good = !!message; > > + // This might be a valid assertion, but if we are constrained to a single peer > > + // identity, then we also need to make sure that the assertion matches > > + if (good && this._impl.peerIdentity) { > > + good = message.identity === this._impl.peerIdentity; > > Some parentheses would make me feel more comfortable here. It's fine... Comfort parentheses coming up. > @@ +705,5 @@ > > + let idpError = null; > > + > > + // we can run the IdP validation in parallel with setRemoteDescription this > > + // complicates much more than would be ideal, but it ensures that the IdP > > + // doesn't hold things up too much when it's not on the critical path > > Pity we don't have promises for this. I considered pulling in Promise.jsm, then reconsidered. It's just not ready enough yet. > @@ +720,5 @@ > > + > > + let setRemoteDone = function() { > > + setRemoteComplete = true; > > + allDone(); > > + }.bind(this); > > bind is not needed on this one. Yeah, this should be a () => {} closure. I need to use those more. > @@ +732,5 @@ > > + } else { > > + idpDone = function(message) { > > + let idpGood = this._processIdpResult(message); > > + if (!idpGood) { > > + // iff we are waiting for a very specific peerIdentity > > typo iff = "if and only if", not typo :) > ::: dom/media/tests/identity/idp-proxy.js > @@ +3,5 @@ > > > > function IDPJS() { > > this.domain = window.location.host; > > + var p = window.location.pathname; > > + this.protocol = p.substring(p.lastIndexOf('/') + 1) + window.location.hash; > > Manual url parsing is usually fraught with issues, but in this case I trust > the window.location is not a content window or otherwise modifiable from > content? This is effectively content code - it's what runs in the IdP sandbox - and I know that it doesn't navigate. Yes, I would ordinarily avoid this sort of mess, but I don't have (or want) access to the real URL parsing code here. > ::: dom/media/tests/identity/test_peerConnection_peerIdentity.html > @@ +45,5 @@ > > + peerIdentity: id1 > > + }]); > > + test.setIdentityProvider(test.pcLocal, 'test1.example.com', 'idp.html'); > > + test.setIdentityProvider(test.pcRemote, 'test2.example.com', 'idp.html'); > > + test.chain.append([ > > Doesn't this run all the other tests first? Are they all necessary to run > again for this, or could some be skipped? In this case, I want to have an active connection between the two peers. The easiest way to get that is to run all the other tests. Looking at template.js, the only tests that I really want to drop here are the stats tests. That's not a big enough saving to worry about. > ::: dom/media/tests/mochitest/blacksilence.js > @@ +30,5 @@ > > + clearInterval(interval); > > + interval = null; > > + done(); > > + } > > + }, 50); > > Maybe something divisible by 15 ms for Windows' sake? I also worry 50 ms is > too tight for some of the builders, and might cause intermittents It's 15.6 or 1/64 seconds actually. Though I'm not sure if that makes any difference, since we probably switch to running real-time timers once media is going. 15.6 is basically only a power saving measure. I'm effectively running this in a tight loop. There is no lower bound on the time it takes to complete. The risk here is that the check is costly enough to push the CPU over the edge, not only taxing the machine enough that it takes 50ms to complete. In that case, it seems unlikely that it's the checking that is costing so much. > ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp > @@ +173,5 @@ > > CC_CallFeature_FoundICECandidate(vcm_opaque->call_handle_, > > + candidate_tmp, > > + nullptr, > > + vcm_opaque->level_, > > + nullptr); > > Splinter says there are lots of whitespace-only changes in this file, yet I > see no difference. This should be undone to preserve blame. It's tabs to spaces. This file was a royal mess. I've reverted most, if not all of these. Incidentally, blame can be queried in whitespace-intolerant mode, which ensures that whitespace cleanup doesn't cause misattribution. Better that than to leave things messed up. > @@ +2227,5 @@ > > + */ > > +static int vcmTxCreateAudioConduit(int level, > > + const vcm_payload_info_t *payload, > > + sipcc::PeerConnectionWrapper &pc, > > + vcm_mediaAttrs_t *attrs, > > maybe const? Sure. > @@ +2344,5 @@ > > + short tos, > > + sdp_setup_type_e setup_type, > > + const char *fingerprint_alg, > > + const char *fingerprint, > > + vcm_mediaAttrs_t *attrs) > > I would refrain from whitespace only changes to preserve blame. See above. > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp > @@ +554,4 @@ > > #ifdef MOZILLA_INTERNAL_API > > + nsCOMPtr<nsIPrincipal> systemPrincipal; > > + nsCOMPtr<nsIScriptSecurityManager> securityManager = > > + do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID); > > Should check NS_SUCCEEDED(rv) && securityManager I think Well, this part is definitely changing. > @@ +560,5 @@ > > + stream->CombineWithPrincipal(systemPrincipal); > > + > > + // Make the stream data (audio/video samples) accessible to the receiving page, > > + // or bind it to the peer identity, if that is enabled. > > + if (PrivacyRequested()) { > > if (mPrivacyRequested) ? > > This may be a style thing. I'm generally not a fan of classes "APIing" > themselves just for the sake of it e.g. indirecting their own member access. > I prefer stark parent-child encapsulation. That's why we have private. Just > my 2 cents. I'm not a fan of getters either. But where the indirection exists, it should be used consistently. Internally and externally. Otherwise if there is a need to change the getter, you don't get inconsistent behaviour. > @@ +754,5 @@ > > mWindow = aWindow; > > NS_ENSURE_STATE(mWindow); > > > > + mPeerIdentity = aRTCConfiguration->mPeerIdentity; > > + mPrivacyRequested = !mPeerIdentity.IsVoid(); // null check > > Why not use IsEmpty() instead of IsVoid() throughout here? What happens if > empty string is passed in? The empty string is invalid, true. But that failure gets caught at other levels (we don't allow an IdP to make sweeping assertions like that). After checking, it looks like IsEmpty() encompasses IsVoid(). That means that we get behaviour consistent with the JavaScript boolean coercion. I'm probably going to go with the change. > @@ +755,5 @@ > > NS_ENSURE_STATE(mWindow); > > > > + mPeerIdentity = aRTCConfiguration->mPeerIdentity; > > + mPrivacyRequested = !mPeerIdentity.IsVoid(); // null check > > + nsCOMPtr<nsPIDOMWindow> window(do_QueryInterface(mWindow)); > > window != nullptr? Some code checks some doesn't... The MOZ_ASSERT() and NS_ENSURE_STATE() immediately above should sort that out. I'm fairly sure that it can't go null off the main thread. > @@ +757,5 @@ > > + mPeerIdentity = aRTCConfiguration->mPeerIdentity; > > + mPrivacyRequested = !mPeerIdentity.IsVoid(); // null check > > + nsCOMPtr<nsPIDOMWindow> window(do_QueryInterface(mWindow)); > > + > > + mPrincipal = window->GetExtantDoc()->NodePrincipal(); > > Check doc? Probably, but this is going away shortly. > @@ +1280,5 @@ > > mTimeCard = nullptr; > > STAMP_TIMECARD(tc, "Set Local Description"); > > > > +#ifdef MOZILLA_INTERNAL_API > > + nsIPrincipal* scriptPrincipal = GetWindow()->GetExtantDoc()->NodePrincipal(); > > Can any of these fail? You check doc against nullptr elsewhere. I think that I should be using nsScriptSecurityManager here anyway. That has a different set of failure modes. > @@ +1409,5 @@ > > +{ > > + PC_AUTO_ENTER_API_CALL(true); > > + > > + // once set, this can't be changed > > + if (!mPeerIdentity.IsVoid()) { > > What happens if passed-in aPeerIdentity.IsVoid()? Then the comment is wrong > if nothing else. Ahh, I should assert that. Thanks. There are plenty of other guards on this, but no harm in making another, particularly since it provides just-in-time documentation. > @@ +1440,5 @@ > > + // if we previously didn't know, time to update stream principals > > + mMedia->UpdateRemoteStreamPrincipals_m(mPrincipal); > > + } > > + } else { > > + // now we know that privacy isn't needed for sure > > I don't understand this comment. Privacy isn't needed when > PrivacyRequested()? Oh crap. I lost the if here. It should have been: } else if (!aPrivacyRequested) { > @@ +1449,5 @@ > > + CSFLogInfo(logTag, "Can't update principal on streams; window gone"); > > + } > > + } > > +#endif > > + mPrivacyRequested = mPrivacyRequested || aPrivacyRequested; > > When mPrivacyRequested is false and doc is null above then mPrivacyRequested > is set here without UpdateRemoteStreamPrincipals_m being called. Is that > expected? That's a failure mode that is likely associated with navigation/tab close/that sort of thing. What you get in that case is isolated streams that can only be displayed. Again, this is a case where I should be using nsScriptSecurityManager to retrieve the principal, which can fail as well in theory, but not due to navigation. > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp > @@ +367,5 @@ > > NS_DISPATCH_NORMAL); > > } > > > > +void > > +PeerConnectionMedia::SelfDestruct_m() > > Seems like this function moved for no reason. Maybe restore to preserve > blame? Yeah, my bad. > @@ +585,5 @@ > > +PeerConnectionMedia::LocalStreamsIsolated(nsIPrincipal *scriptPrincipal) const > > +{ > > + ASSERT_ON_THREAD(mMainThread); > > + > > + for (uint32_t u = 0; u < mLocalSourceStreams.Length(); u++) { > > Rest of file uses for (size_t i http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#364 says uint32_t I'm sure that we'd all like it to be size_t, because that's the way STL would have it. But this is the type it is. I'm sure that there's no harm in using size_t. There's a good chance that it's at least 32 bits wide. > @@ +610,5 @@ > > +LocalSourceStreamInfo::UpdateSinkPrincipal_m(nsIPrincipal* aPrincipal) > > +{ > > + for (auto it = mPipelines.begin(); it != mPipelines.end(); ++it) { > > + mozilla::RefPtr<mozilla::MediaPipelineTransmit> pipeline( > > + static_cast<mozilla::MediaPipelineTransmit*>((*it).second.get())); > > Why bypass virtual? Also, I don't think you need to addref here. I didn't think that bypassed virtual. It's true, (*it).second->DoTheThing() would be smaller and easier to read. Shame that it doesn't work that way. Avoiding the cast would require templating the crap out of SourceStreamInfo. I don't think that's strictly better; it seems like this is the lesser evil. The AddRef, sure, that's probably overkill.

(In reply to Martin Thomson [:mt] from comment #39) > This is effectively content code - it's what runs in the IdP sandbox - and I > know that it doesn't navigate. Yes, I would ordinarily avoid this sort of > mess, but I don't have (or want) access to the real URL parsing code here. In that case I would advice using as much existing URI parsing code as possible. bz was able to trip up most of my early URI parsing attempts for the stun/turn uris. I ended up using nsURI and ParseAuthority() to minimize novel parsing - http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#658 I'll yield to bholley on this. And if you end up redoing this to not use URLs then maybe it'll be moot. > Looking at > template.js, the only tests that I really want to drop here are the stats > tests. That's not a big enough saving to worry about. That's the one I was thinking about. > I'm effectively running this in a tight loop. There is no lower bound on > the time it takes to complete. The risk here is that the check is costly > enough to push the CPU over the edge, not only taxing the machine enough > that it takes 50ms to complete. In that case, it seems unlikely that it's > the checking that is costing so much. Make sure to repeat the mochis on tbpl a couple of times on try, especially on Windows XP. It may be fine, I just know I've been plagued by intermittents every time I add timing assumptions to a mochitest. > I've reverted most, if not all of these. Incidentally, blame can be queried > in whitespace-intolerant mode, which ensures that whitespace cleanup doesn't > cause misattribution. Better that than to leave things messed up. I don't think annotate on http://hg.mozilla.org has that. > I'm not a fan of getters either. But where the indirection exists, it > should be used consistently. Internally and externally. Otherwise if there > is a need to change the getter, you don't get inconsistent behaviour. I think that's a design decision. I think of getters are exporters. In any case, not a big deal here. > > Rest of file uses for (size_t i > > http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#364 says > uint32_t So it does! TIL > > > + for (auto it = mPipelines.begin(); it != mPipelines.end(); ++it) { > > > + mozilla::RefPtr<mozilla::MediaPipelineTransmit> pipeline( > > > + static_cast<mozilla::MediaPipelineTransmit*>((*it).second.get())); > > > > Why bypass virtual? Also, I don't think you need to addref here. > > I didn't think that bypassed virtual. It's true, (*it).second->DoTheThing() > would be smaller and easier to read. Shame that it doesn't work that way. Sorry I didn't mean bypass, I meant, since all the methods are virtual, why bother to downcast? e.g. what's wrong with: for (auto it = mPipelines.begin(); it != mPipelines.end(); ++it) { MediaPipeline* pipeline = (*it).second.get(); Also, no need to add mozilla:: (in new code) since the namespace is open in these .cpp files.

Comment on attachment 8401462 [details] [diff] [review] 0002-Bug-942367-New-PeerIdentityPrincipal-and-supporting-.patch Per previous discussion, r-ing this and cancelling review on the other one.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #40) > (In reply to Martin Thomson [:mt] from comment #39) > > This is effectively content code - it's what runs in the IdP sandbox - and I > > know that it doesn't navigate. Yes, I would ordinarily avoid this sort of > > mess, but I don't have (or want) access to the real URL parsing code here. > > In that case I would advice using as much existing URI parsing code as > possible. bz was able to trip up most of my early URI parsing attempts for > the stun/turn uris. I ended up using nsURI and ParseAuthority() to minimize > novel parsing - > http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/ > peerconnection/PeerConnectionImpl.cpp#658 Sorry, I wasn't clear enough. This *is* content code. It has extra restrictions, not extra capabilities, so all that wonderful URI parsing code we have in gecko is completely inaccessible. > > Looking at > > template.js, the only tests that I really want to drop here are the stats > > tests. That's not a big enough saving to worry about. > > That's the one I was thinking about. I'd rather run the check once more and ensure that identity being enabled doesn't alter stats than save a few 100ms. > > I'm effectively running this in a tight loop. There is no lower bound on > > the time it takes to complete. The risk here is that the check is costly > > enough to push the CPU over the edge, not only taxing the machine enough > > that it takes 50ms to complete. In that case, it seems unlikely that it's > > the checking that is costing so much. > > Make sure to repeat the mochis on tbpl a couple of times on try, especially > on Windows XP. It may be fine, I just know I've been plagued by > intermittents every time I add timing assumptions to a mochitest. They have been run quite a few times already, but I'll do that. > > > > + for (auto it = mPipelines.begin(); it != mPipelines.end(); ++it) { > > > > + mozilla::RefPtr<mozilla::MediaPipelineTransmit> pipeline( > > > > + static_cast<mozilla::MediaPipelineTransmit*>((*it).second.get())); > > > > > > Why bypass virtual? Also, I don't think you need to addref here. > > > > I didn't think that bypassed virtual. It's true, (*it).second->DoTheThing() > > would be smaller and easier to read. Shame that it doesn't work that way. > > Sorry I didn't mean bypass, I meant, since all the methods are virtual, why > bother to downcast? e.g. what's wrong with: > > for (auto it = mPipelines.begin(); it != mPipelines.end(); ++it) { > MediaPipeline* pipeline = (*it).second.get(); > > Also, no need to add mozilla:: (in new code) since the namespace is open in > these .cpp files. I've dropped the ns prefix, but I still need to cast (or add templating). The method I'm calling isn't on MediaPipeline.

> so all that wonderful URI parsing code we have in gecko is completely inaccessible Wait, why? Are we talking about untrusted JS needing to parse URIs or something?

Yes, it's untrusted JS, in a mochitest support file. Currently, it does a little URI parsing: > var p = window.location.pathname; > this.protocol = p.substring(p.lastIndexOf('/') + 1) + window.location.hash;

That seems perfectly reasonable to me.

Carrying r+ from jib. (Addresses comments)

Adding custom attribute for tracking peerIdentity-based media isolation on DOMMediaStream.

As much as possible, I've tried to keep this the same as what was reviewed before, but that's hardly easy when everything is changed. bholley, can you review the use of principals in this to ensure I'm not screwing things up? (Searching for "Principal" should hit all the relevant parts.)

Can you upload an interdiff? Splinter cried uncle.

I had an interdiff at some point, but it wasn't that much use. Too many small changes. This is 964+/277-, the interdiff was >600+/>400-. And then I had to rebase. Let me see if I can find it.

This is not exactly the same code that is in the patch, because several small changes were necessary, but it should help jib identify what is (and isn't) needing review.

Comment on attachment 8405540 [details] [diff] [review] 0003-Bug-942367-Stream-isolation-for-WebRTC.patch Review of attachment 8405540 [details] [diff] [review]: ----------------------------------------------------------------- I don't have a great sense of the intricacies of the spec here, but the usage of principals here all seems very reasonable, and IIUC along the lines of the suggestion in comment 37. ::: dom/media/MediaManager.cpp @@ +566,5 @@ > reinterpret_cast<int64_t>(trackunion->GetStream())); > > + nsCOMPtr<nsIPrincipal> principal; > + if (mPeerIdentity) { > + principal = do_CreateInstance(NS_NULLPRINCIPAL_CONTRACTID); At the point where you're including nsNullPrincipal.h, you can just |new nsNullPrincipal()|, right? Otherwise, drop the include, and just inline "@mozilla.org/nullprincipal;1" ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp @@ +662,5 @@ > + const PeerIdentity* sinkIdentity) { > + ASSERT_ON_THREAD(main_thread_); > + bool enableStream; > + > + nsresult rv = principal->Subsumes(domstream_->GetPrincipal(), &enableStream); Just use the simple overload that's inlined in nsIPrincipal.idl: bool enableStream = principal->Subsumes(domstream_->GetPrincipal());

Comment on attachment 8405537 [details] [diff] [review] 0002-Bug-942367-DOMMediaStream-supports-PeerIdentity.patch Review of attachment 8405537 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webrtc/PeerIdentity.cpp @@ +50,5 @@ > + > +/* static */ void > +PeerIdentity::GetUser(const nsAString& aPeerIdentity, nsAString& aUser) > +{ > + int32_t at = aPeerIdentity.FindChar('@'); Is it specified somewhere that everything after the first @ is the host? If so please cite it here.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #53) > Is it specified somewhere that everything after the first @ is the host? If > so please cite it here. Here specifically? I've added a reference to the definition in the header file.

Removing dependency on bug 907770, in favour of bug 995328.

Comment on attachment 8405540 [details] [diff] [review] 0003-Bug-942367-Stream-isolation-for-WebRTC.patch Review of attachment 8405540 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits ::: dom/media/MediaManager.cpp @@ +1637,4 @@ > // Notify the UI that this window no longer has gUM active > char windowBuffer[32]; > PR_snprintf(windowBuffer, sizeof(windowBuffer), "%llu", outerID); > + nsString data = NS_ConvertUTF8toUTF16(windowBuffer); NS_ConvertUTF8toUTF16 data(windowBuffer); ::: dom/media/PeerConnection.js @@ +348,5 @@ > return this._pc; > }, > > + callCB: function(callback, arg) { > + if (typeof callback === "function") { (Missed on first review) While moving this function you changed this line from: if (callback) { Why? The old function silently accepted only falsy values, whereas now it silently accepts a broader range of values instead of throwing a type-error, hence is more permissive. @@ +721,5 @@ > + onSuccess = null; > + this._executeNext(); > + }; > + > + let setRemoteDone = () => { Indent by one more. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +553,5 @@ > #ifdef MOZILLA_INTERNAL_API > + // Make the stream data (audio/video samples) accessible to the receiving page, > + // or bind it to the peer identity, if that is enabled. > + // we're only certain that privacy hasn't been requested if we're not yet connected > + if (mDtlsConnected && !PrivacyRequested()) { Comment doesn't match logic. @@ +1417,5 @@ > + nsIDocument* doc = GetWindow()->GetExtantDoc(); > + if (doc) { > + mMedia->UpdateSinkIdentity_m(doc->NodePrincipal(), mPeerIdentity); > + } else { > + CSFLogInfo(logTag, "Can't update principal on streams; document gone"); I'm curious, why not fail? @@ +1441,5 @@ > + nsIDocument* doc = GetWindow()->GetExtantDoc(); > + if (doc) { > + mMedia->UpdateRemoteStreamPrincipals_m(doc->NodePrincipal()); > + } else { > + CSFLogInfo(logTag, "Can't update principal on streams; document gone"); Same question. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h @@ +393,5 @@ > + return NS_OK; > + } > +#endif > + > + peerIdentity.SetIsVoid(true); Is SetIsVoid frozen? I notice you're not calling this function (GetPeerIdentity) anywhere in this patch. Is it needed? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ +632,5 @@ > +{ > + // this blasts away the existing principal, which, if we're here > + // will be the null principal > + // we only do this when we become certain that the stream is safe to make > + // accessible to the script principal Are these assertable assumptions?

(In reply to Jan-Ivar Bruaroey [:jib] from comment #56) > ::: dom/media/MediaManager.cpp > @@ +1637,4 @@ > > // Notify the UI that this window no longer has gUM active > > char windowBuffer[32]; > > PR_snprintf(windowBuffer, sizeof(windowBuffer), "%llu", outerID); > > + nsString data = NS_ConvertUTF8toUTF16(windowBuffer); > > NS_ConvertUTF8toUTF16 data(windowBuffer); I honestly didn't think that would work. I couldn't find the definition of ::get(). But it compiles and runs, so I guess it's OK. > ::: dom/media/PeerConnection.js > @@ +348,5 @@ > > return this._pc; > > }, > > > > + callCB: function(callback, arg) { > > + if (typeof callback === "function") { > > (Missed on first review) While moving this function you changed this line > from: > > if (callback) { > > Why? The old function silently accepted only falsy values, whereas now it > silently accepts a broader range of values instead of throwing a type-error, > hence is more permissive. You're concerned about hiding failures. Sounds reasonable. The only errors this surfaces - assuming our WebIDL interface is properly type-checking content-provided callbacks - is errors in our code. But that's valuable too I guess. I have a habit, honed from years of being stung, of checking that the thing that I'm calling can actually be called. Suppressing that habit takes conscious effort. > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp > @@ +553,5 @@ > > #ifdef MOZILLA_INTERNAL_API > > + // Make the stream data (audio/video samples) accessible to the receiving page, > > + // or bind it to the peer identity, if that is enabled. > > + // we're only certain that privacy hasn't been requested if we're not yet connected > > + if (mDtlsConnected && !PrivacyRequested()) { > > Comment doesn't match logic. It does now. Thanks. > @@ +1417,5 @@ > > + nsIDocument* doc = GetWindow()->GetExtantDoc(); > > + if (doc) { > > + mMedia->UpdateSinkIdentity_m(doc->NodePrincipal(), mPeerIdentity); > > + } else { > > + CSFLogInfo(logTag, "Can't update principal on streams; document gone"); > > I'm curious, why not fail? I wasn't sure that there was a point to it. If the document is gone, we're in the tail end of a page cleanup and this probably isn't going to have any material effect on anything. Still, no harm in it. I'll add a return statement here. > @@ +1441,5 @@ > > + nsIDocument* doc = GetWindow()->GetExtantDoc(); > > + if (doc) { > > + mMedia->UpdateRemoteStreamPrincipals_m(doc->NodePrincipal()); > > + } else { > > + CSFLogInfo(logTag, "Can't update principal on streams; document gone"); > > Same question. Same answer :) > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h > @@ +393,5 @@ > > + return NS_OK; > > + } > > +#endif > > + > > + peerIdentity.SetIsVoid(true); > > Is SetIsVoid frozen? In what way? It's the way I know to get a null over into JS. > I notice you're not calling this function (GetPeerIdentity) anywhere in this > patch. Is it needed? In PeerConnection.js, see RTCPeerConnection::_processIdpResult(message). > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp > @@ +632,5 @@ > > +{ > > + // this blasts away the existing principal, which, if we're here > > + // will be the null principal > > + // we only do this when we become certain that the stream is safe to make > > + // accessible to the script principal > > Are these assertable assumptions? Not really. It requires knowledge from the calling context, and that would break encapsulation. This is just extra documentation for a call that isn't ordinarily a good idea.

(In reply to Martin Thomson [:mt] from comment #57) > > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h > > @@ +393,5 @@ > > > + return NS_OK; > > > + } > > > +#endif > > > + > > > + peerIdentity.SetIsVoid(true); > > > > Is SetIsVoid frozen? > > In what way? It's the way I know to get a null over into JS. This call is outside MOZ_INTERNAL_API so I was surprised it even compiled. I thought only the frozen string api was available outside it, and I read that the void-functionality wasn't included in that (from years ago). But it compiles and isn't somehow ignored/optimized away, so ignore me. > > I notice you're not calling this function (GetPeerIdentity) anywhere in this > > patch. Is it needed? > > In PeerConnection.js, see RTCPeerConnection::_processIdpResult(message). Thanks. > > Are these assertable assumptions? > > Not really. It requires knowledge from the calling context, and that would > break encapsulation. This is just extra documentation for a call that isn't > ordinarily a good idea. What about the existing principal being the null principal?

(In reply to Jan-Ivar Bruaroey [:jib] from comment #58) > > > Are these assertable assumptions? > > > > Not really. It requires knowledge from the calling context, and that would > > break encapsulation. This is just extra documentation for a call that isn't > > ordinarily a good idea. > > What about the existing principal being the null principal? There are other reasons that it might be null. I don't think that a partial assert is a good idea.

OK.

(In reply to Martin Thomson [:mt] from comment #59) > > What about the existing principal being the null principal? > > There are other reasons that it might be null. I don't think that a partial > assert is a good idea. The wording of these two comments makes me nervous. There is no such thing as "the null principal". There are nsNullPrincipal instances, each of which is unique. And there is nullptr, which is not a valid principal and should not be given any meaning from a security perspective.

(In reply to Bobby Holley (:bholley) from comment #61) > (In reply to Martin Thomson [:mt] from comment #59) > > > What about the existing principal being the null principal? > > > > There are other reasons that it might be null. I don't think that a partial > > assert is a good idea. > > The wording of these two comments makes me nervous. There is no such thing > as "the null principal". There are nsNullPrincipal instances, each of which > is unique. And there is nullptr, which is not a valid principal and should > not be given any meaning from a security perspective. Sorry, I was being imprecise. You are right. I don't believe that we ever want a stream to have a nullptr reference for its principal; that would crash the code. My response should have been: I could check that the principal is an instance of nsNullPrincipal, but that would not be sufficient, because there are other cases where a stream might legitimately have nsNullPrincipal instance as principal AND this call is invalid. Asserting here on a necessary, but not sufficient condition might imply that the condition is sufficient. The comment is good enough in my opinion.

If you have an invariant then you should test it IHMO. It doesn't have to catch ALL bugs to be worthwhile.

I got what I needed from try: https://tbpl.mozilla.org/?tree=Try&rev=c5d0d073f7c8

(In reply to Jan-Ivar Bruaroey [:jib] from comment #63) > If you have an invariant then you should test it IHMO. It doesn't have to > catch ALL bugs to be worthwhile. Adding a DebugOnly check for isNullPrincipal(). I realized that this could could have been called twice, so I added an extra guard. I'll need to double check my code, since I've not used DebugOnly before, but this is looking good to go.

Carrying r+ from jib.

r=roc

r=jib,bholley

Fixing comment

r=jib,bholley. Uploaded an old version by accident: made changes in the repo rather than editing patch files by hand.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1e04ec8d283 https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0426ac98f1 https://hg.mozilla.org/integration/mozilla-inbound/rev/e6c72bcaa64c

sorry had to backout for test failure like https://tbpl.mozilla.org/php/getParsedLog.php?id=37907649&tree=Mozilla-Inbound

I shouldn't have added that assertion. I'm going to remove it. Turns out that we do call this a few times, and that's perfectly OK.

Backing out assertion for breakage.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cc66aee4341 https://hg.mozilla.org/integration/mozilla-inbound/rev/7d2138e87ca0 https://hg.mozilla.org/integration/mozilla-inbound/rev/1e581e74878d

Backed out for B2G mochitest failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/c2094871202f https://tbpl.mozilla.org/php/getParsedLog.php?id=38049475&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=38050896&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=38055770&tree=Mozilla-Inbound All permafails after this landed ^

The webaudio failure in the above logs was someone else's.

My testing here shows that jib was entirely correct in his concern regarding the timing. I've cranked up the times and I'm yet to see a failure on my rig[1]. Next step: try. [1] My macbook running Linux in a virtualbox VM, which is allocated only 40% of the CPU, which runs the mochitests in an emulator. It's super-painful. I'm running at 20% now, despite warnings that this might cause delays. No kidding.

OK, so the warnings were right, I never managed to get a successful run at 20% CPU in the VM. But I think that's OK, because it runs orders of magnitudes slower than what I've seen from TBPL (5 minutes just to launch the emulator, 5 more before tests start. TBPL is better. Bug 963244 is being quite annoying here, but it looks good so far. I'll do a couple more rounds and call this a (qualified) success. https://tbpl.mozilla.org/?tree=Try&rev=513b9f197dc4

Carrying r+

Carrying r+, unrotting.

Carrying r+, adding looser timers for b2g emulator.

These patches are bitrotten. Please rebase.

unrot r=jib

unrot, r=jib

unrot, r=jib,bholley

Did the webidl changes here ever get reviewed by a DOM peer? *** WebIDL file dom/webidl/MediaStream.webidl altered in changeset 77917bf9583e without DOM peer review WebIDL file dom/webidl/PeerConnectionImpl.webidl altered in changeset 77917bf9583e without DOM peer review WebIDL file dom/webidl/RTCConfiguration.webidl altered in changeset 77917bf9583e without DOM peer review*** remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=... This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..

Comment on attachment 8416066 [details] [diff] [review] 0002-Bug-942367-WebIDL-changes-for-peerIdentity-constrain.patch As RyanVM notes, the rules are now more stringent here. I should have asked for review. The spec for the constraints is here: http://dev.w3.org/2011/webrtc/editor/getusermedia.html#mediastreamconstraints And the RTCConfiguration is missing from the spec, but there's an open issue to fix the omission: https://github.com/fluffy/webrtc-w3c/pull/20 All of this remains behind a pref for now. The other stuff is internal supporting stuff (PeerConnectionImpl is ChromeOnly).

Comment on attachment 8416066 [details] [diff] [review] 0002-Bug-942367-WebIDL-changes-for-peerIdentity-constrain.patch Looks good, r=jst

Can you please send an intent to implement/ship email to dev-platform about this?

Sent; wasn't sure if this wasn't already covered by similar commitments on the larger WebRTC API.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1dabddb5afe https://hg.mozilla.org/integration/mozilla-inbound/rev/8d893585ff9a https://hg.mozilla.org/integration/mozilla-inbound/rev/50116088c244

https://hg.mozilla.org/mozilla-central/rev/d1dabddb5afe https://hg.mozilla.org/mozilla-central/rev/8d893585ff9a https://hg.mozilla.org/mozilla-central/rev/50116088c244