973315 - "Assertion failure: location" (sipcc::PeerConnectionImpl::Initialize)

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Jesse Ruderman]

Attachment: testcase

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

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Byron Campen [:bwc]]

Attachment: Make code that sets PeerConnectionImpl::mName more tolerant of weird scenarios.

Here's a fix.

Comment 3

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=735931e96239

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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()).

Comment 5

[Byron Campen [:bwc]]

(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.

Comment 6

[Byron Campen [:bwc]]

Attachment: Make code that sets PeerConnectionImpl::mName more tolerant of weird scenarios.

Incorporate feedback.

Comment 7

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=33a317734fc8

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 9

[Byron Campen [:bwc]]

(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.

Comment 10

[Byron Campen [:bwc]]

(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.

Comment 11

[Byron Campen [:bwc]]

Attachment: Make code that sets PeerConnectionImpl::mName more tolerant of weird scenarios.

Fix a nit.

Comment 12

[Byron Campen [:bwc]]

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 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4526c5815d9d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4526c5815d9d

Comment 16

[Byron Campen [:bwc]]

(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.