Assertion failure: location, at /Users/jruderman/trees/mozilla-central/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:733 This assertion was added in bug 906990: http://hg.mozilla.org/mozilla-central/rev/26bd374d3513

Here's a fix.

Comment on attachment 8377733 [details] [diff] [review] Make code that sets PeerConnectionImpl::mName more tolerant of weird scenarios. Review of attachment 8377733 [details] [diff] [review]: ----------------------------------------------------------------- r- for int windowID and incorrect error handling. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +732,2 @@ > nsIDOMLocation* location = nullptr; > mWindow->GetLocation(&location); You should check the error result of GetLocation() here rather than rely on secondary results from a failing function. A failing function is not required to leave its args alone AFAIK. @@ +734,3 @@ > > + if (location) { > + nsString locationAStr; I think nsAutoString is preferred for on-the-stack use like this. @@ +736,5 @@ > + nsString locationAStr; > + location->ToString(locationAStr); > + location->Release(); > + > + CopyUTF16toUTF8(locationAStr, locationCStr); Just fyi, you might like the shorter NS_ConvertUTF16toUTF8 newFoo(aFoo); pattern if it can be worked in here. @@ +749,3 @@ > PR_snprintf(temp, > sizeof(temp), > "%llu (id=%u url=%s)", WindowID is 64-bit so you should use "%llu (id=%llu url=%s)" here. @@ +749,5 @@ > PR_snprintf(temp, > sizeof(temp), > "%llu (id=%u url=%s)", > (unsigned long long)timestamp, > + (unsigned int)windowID, I prefer fewer temporary stack variables, e.g. (mWindow? mWindow->WindowID() : 0). @@ +754,1 @@ > locationCStr.get() ? locationCStr.get() : "NULL"); I doubt .get() ever returns nullptr, though I'm not sure. The preferred pattern seems to be to test on .IsEmpty() (or .IsVoid()).

(In reply to Jan-Ivar Bruaroey [:jib] from comment #4) > Comment on attachment 8377733 [details] [diff] [review] > Make code that sets PeerConnectionImpl::mName more tolerant of weird > scenarios. > > Review of attachment 8377733 [details] [diff] [review]: > @@ +734,3 @@ > > > > + if (location) { > > + nsString locationAStr; > > I think nsAutoString is preferred for on-the-stack use like this. > Ah, this does local storage. Sure, why not. > @@ +736,5 @@ > > + nsString locationAStr; > > + location->ToString(locationAStr); > > + location->Release(); > > + > > + CopyUTF16toUTF8(locationAStr, locationCStr); > > Just fyi, you might like the shorter NS_ConvertUTF16toUTF8 newFoo(aFoo); > pattern if it can be worked in here. > I don't think this will help, since locationCStr needs to exist regardless of whether location is set or not. > @@ +754,1 @@ > > locationCStr.get() ? locationCStr.get() : "NULL"); > > I doubt .get() ever returns nullptr, though I'm not sure. > I actually looked, and it seems to be possible.

Incorporate feedback.

Comment on attachment 8378540 [details] [diff] [review] Make code that sets PeerConnectionImpl::mName more tolerant of weird scenarios. Review of attachment 8378540 [details] [diff] [review]: ----------------------------------------------------------------- lgtm with nits. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +732,1 @@ > nsIDOMLocation* location = nullptr; Redundant initalization. @@ +734,2 @@ > > + if (location && NS_SUCCEEDED(res)) { if (NS_SUCCEEDED(res) && location) { @@ +744,5 @@ > + temp, > + sizeof(temp), > + "%llu (id=%llu url=%s)", > + static_cast<unsigned long long>(timestamp), > + static_cast<unsigned long long>(mWindow ? mWindow->WindowID() : 0), mWindow->WindowID() is already uint64_t.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #8) > Comment on attachment 8378540 [details] [diff] [review] > Make code that sets PeerConnectionImpl::mName more tolerant of weird > scenarios. > > Review of attachment 8378540 [details] [diff] [review]: > ----------------------------------------------------------------- > > lgtm with nits. > > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp > @@ +732,1 @@ > > nsIDOMLocation* location = nullptr; > > Redundant initalization. > > @@ +734,2 @@ > > > > + if (location && NS_SUCCEEDED(res)) { > > if (NS_SUCCEEDED(res) && location) { > I'll just remove both the null check and initialization entirely, and assume that location is valid iff NS_SUCCEEDED(res). > @@ +744,5 @@ > > + temp, > > + sizeof(temp), > > + "%llu (id=%llu url=%s)", > > + static_cast<unsigned long long>(timestamp), > > + static_cast<unsigned long long>(mWindow ? mWindow->WindowID() : 0), > > mWindow->WindowID() is already uint64_t. I have been bitten enough times by integer width changing on me in unexpected ways that I've adopted a policy of doing an explicit cast when using printf-like functions.

(In reply to Byron Campen [:bwc] from comment #9) > (In reply to Jan-Ivar Bruaroey [:jib] from comment #8) > > Comment on attachment 8378540 [details] [diff] [review] > > Make code that sets PeerConnectionImpl::mName more tolerant of weird > > scenarios. > > > > Review of attachment 8378540 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > lgtm with nits. > > > > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp > > @@ +732,1 @@ > > > nsIDOMLocation* location = nullptr; > > > > Redundant initalization. > > > > @@ +734,2 @@ > > > > > > + if (location && NS_SUCCEEDED(res)) { > > > > if (NS_SUCCEEDED(res) && location) { > > > > I'll just remove both the null check and initialization entirely, and > assume that location is valid iff NS_SUCCEEDED(res). Actually, this is no good; it can still be set to nullptr if the call succeeds.

Fix a nit.

Comment on attachment 8378683 [details] [diff] [review] Make code that sets PeerConnectionImpl::mName more tolerant of weird scenarios. Review of attachment 8378683 [details] [diff] [review]: ----------------------------------------------------------------- Carry forward r=jib

Comment on attachment 8378683 [details] [diff] [review] Make code that sets PeerConnectionImpl::mName more tolerant of weird scenarios. Review of attachment 8378683 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +734,2 @@ > > + if (location && NS_SUCCEEDED(res)) { if (NS_SUCCEEDED(res) && location) { ? Not a big deal, but accessing a potentially uninitialized variable first before knowing it is valid still creeps me out, even though it is safe in all cases here.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #13) > Comment on attachment 8378683 [details] [diff] [review] > Make code that sets PeerConnectionImpl::mName more tolerant of weird > scenarios. > > Review of attachment 8378683 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp > @@ +734,2 @@ > > > > + if (location && NS_SUCCEEDED(res)) { > > if (NS_SUCCEEDED(res) && location) { ? > > Not a big deal, but accessing a potentially uninitialized variable first > before knowing it is valid still creeps me out, even though it is safe in > all cases here. Actually, come to think of it, valgrind is a good reason to switch this around. It doesn't merit its own ticket I think, so I'll go ahead and tack it onto one of the half-dozen patches to PeerConnectionImpl I'm working on.