Closed Bug 973315 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: WebRTC: Networking, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jruderman, Assigned: bwc)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase)

Attachments

(3 files, 2 obsolete files)

Attached file 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
Attached file stack
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Attachment #8377733 - Flags: review?(jib)
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()).
Attachment #8377733 - Flags: review?(jib) → review-
(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.
Attachment #8377733 - Attachment is obsolete: true
Attachment #8378540 - Flags: review?(jib)
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.
Attachment #8378540 - Flags: review?(jib) → review+
(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.
Attachment #8378540 - Attachment is obsolete: true
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
Attachment #8378683 - Flags: review+
Whiteboard: [checkin-needed]
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.
https://hg.mozilla.org/mozilla-central/rev/4526c5815d9d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(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.
You need to log in before you can comment on or make changes to this bug.