Closed Bug 780790 Opened 12 years ago Closed 12 years ago

crash in mozGetUserMedia (null mSuccess) [@ mozilla::SuccessCallbackRunnable::Run()][@ mozilla::GetUserMediaStreamRunnable::Run()]

Categories

(Core :: WebRTC, defect)

Product:
Core
Component:
WebRTC
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla19

People

(Reporter: jruderman, Assigned: anant)

References

Details

(Keywords: crash, testcase, Whiteboard: [getUserMedia], [blocking-gum+])

Crash Data

Attachments

(4 files, 4 obsolete files)

stack trace
(deleted), text/plain
Details
Testcase
(deleted), text/html
Details
Check for NULL callbacks explicitely - v1
(deleted), patch
jesup
: review+
Details | Diff | Splinter Review
Crashtest v4
(deleted), patch
jesup
: review+
Details | Diff | Splinter Review
Attached file testcase (requires pref) (crashes Firefox) (obsolete) (deleted) — 
1. Set
     user_pref("media.navigator.enabled", true);
2. Load the testcase.
3. Wait 2 seconds.

Result: crash like bp-7088f8d8-7fab-411e-8ec3-d4d542120807

77	      // XPConnect is a magical unicorn.
78	      mSuccess->OnSuccess(mFile);             <-- crashes here
Attached file stack trace (deleted) — 

    
    

    
  
Confirmed. The attached test case also crashes my browser on the latest Nightly (8/6/2012).
Crash Signature: [@ mozilla::SuccessCallbackRunnable::Run()]
OS: Mac OS X → All
Hardware: x86_64 → All
Summary: mozGetUserMedia crash (null mSuccess) → crash in mozilla::SuccessCallbackRunnable::Run - mozGetUserMedia (null mSuccess)

    
    

    
  
Important code worth noting:

navigator.mozGetUserMedia({picture: {}}, null, null);

    
    

    
  
Narrowed down more:

navigator.mozGetUserMedia({video: true}, null, null);

The issue here probably is that we aren't handling null values too well in the function callbacks. We probably just need to null check carefully and fail gracefully.
Whiteboard: [getUserMedia], [blocking-gum+]
Keywords: qawanted

    
    

    
  
I retested in triage and didn't see it happen with the attached test case. But when I tried my own test case by calling:

function onError(err) {

}

navigator.mozGetUserMedia({video: true}, null, onError);

I still get a crash consistently.
Keywords: qawanted

        Attachment #649502 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase (deleted)
        — 
      

    


  
Flags: in-testsuite?

    
    

    
  
I'm able to reproduce for both cases. Patch incoming.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Crashtest v1 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #672696 -
        Attachment description: Crashtest v1 → Crashtest v1 - Not tested

        Attachment #672696 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Crashtest v2 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #672702 -
        Flags: review?(rjesup)

    
    

    
  
Comment on attachment 672702 [details] [diff] [review]
Crashtest v2

Review of attachment 672702 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/crashtests/crashtests.list
@@ +1,1 @@
> +pref(media.peerconnection.enabled,true) pref(media.navigator.permission.disabled,true) load 780790.html

We do not have to disable permissions for faked streams as given by bug 798966. So please ask for review for version 1 of this patch.

        Attachment #672702 -
        Flags: review?(rjesup) → review-

        Attachment #672702 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 672696 [details] [diff] [review]
Crashtest v1

Ah, you are right. Missed that.

        Attachment #672696 -
        Attachment description: Crashtest v1 - Not tested → Crashtest v1

        Attachment #672696 -
        Attachment is obsolete: false

        Attachment #672696 -
        Flags: review?(hskupin)

    
    

    
  
Comment on attachment 672696 [details] [diff] [review]
Crashtest v1

One sec, let me fix the whitespace nit...

        Attachment #672696 -
        Flags: review?(hskupin)

        Attachment #672696 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Crashtest v3 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #672794 -
        Flags: review?(hskupin)
Crash Signature: [@ mozilla::SuccessCallbackRunnable::Run()] → [@ mozilla::SuccessCallbackRunnable::Run()]
[@ mozilla::GetUserMediaStreamRunnable::Run()]
Summary: crash in mozilla::SuccessCallbackRunnable::Run - mozGetUserMedia (null mSuccess) → crash in mozGetUserMedia (null mSuccess) [@ mozilla::SuccessCallbackRunnable::Run()][@ mozilla::GetUserMediaStreamRunnable::Run()]

    
    

    
  
Comment on attachment 672794 [details] [diff] [review]
Crashtest v3

Review of attachment 672794 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/crashtests/780790.html
@@ +6,5 @@
> +<head>
> +  <meta charset="utf-8">
> +  <title>Simple gUM test - null success callback</title>
> +  <script type="application/javascript">
> +    navigator.mozGetUserMedia({video: true, fake: true}, null, function() {});

So the original testcase specified null for the onError parameter. You are using an empty callback. I don't consider it the same given that 'null != (function() {})'. It might also reproduce the crash and it would be nice to test both paths.

        Attachment #672794 -
        Flags: review?(hskupin) → review-

    
    

    
  
(In reply to Henrik Skupin (:whimboo) from comment #15)
> Comment on attachment 672794 [details] [diff] [review]
> Crashtest v3
> 
> Review of attachment 672794 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/tests/crashtests/780790.html
> @@ +6,5 @@
> > +<head>
> > +  <meta charset="utf-8">
> > +  <title>Simple gUM test - null success callback</title>
> > +  <script type="application/javascript">
> > +    navigator.mozGetUserMedia({video: true, fake: true}, null, function() {});
> 
> So the original testcase specified null for the onError parameter. You are
> using an empty callback. I don't consider it the same given that 'null !=
> (function() {})'. It might also reproduce the crash and it would be nice to
> test both paths.

No. The original test case used null in success and error callbacks. The crash here is dealing with the success callback, not the error callback. I just confirmed it does not occur with the error callback.

        Attachment #672794 -
        Flags: review- → review?(rjesup)

    
    

    
  
(In reply to Jason Smith [:jsmith] from comment #16)
> (In reply to Henrik Skupin (:whimboo) from comment #15)
> > Comment on attachment 672794 [details] [diff] [review]
> > Crashtest v3
> > 
> > Review of attachment 672794 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/media/tests/crashtests/780790.html
> > @@ +6,5 @@
> > > +<head>
> > > +  <meta charset="utf-8">
> > > +  <title>Simple gUM test - null success callback</title>
> > > +  <script type="application/javascript">
> > > +    navigator.mozGetUserMedia({video: true, fake: true}, null, function() {});
> > 
> > So the original testcase specified null for the onError parameter. You are
> > using an empty callback. I don't consider it the same given that 'null !=
> > (function() {})'. It might also reproduce the crash and it would be nice to
> > test both paths.
> 
> No. The original test case used null in success and error callbacks. The
> crash here is dealing with the success callback, not the error callback. I
> just confirmed it does not occur with the error callback.

Now note - that's not saying we shouldn't test this though (null error cases), but let's do that on a separate patch. I'll file a followup bug for this.

    
    

    
  
Filed bug 803164 for the null check automated tests in general.

    
    

    
  


  
Assignee: nobody → anant
Status: NEW → ASSIGNED

        Attachment #672875 -
        Flags: review?(rjesup)

        Attachment #672794 -
        Attachment is obsolete: true

        Attachment #672794 -
        Flags: review?(rjesup)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Crashtest v4 (deleted)
        — 
          — Splinter Review
      

    


  

    
    

    
  
Comment on attachment 672880 [details] [diff] [review]
Crashtest v4

Updated test to reflect Anant's patch.

        Attachment #672880 -
        Flags: review?(rjesup)

    
    

    
  
Comment on attachment 672875 [details] [diff] [review]
Check for NULL callbacks explicitely - v1

Review of attachment 672875 [details] [diff] [review]:
-----------------------------------------------------------------

r+, but consider if (especially for error) it might make sense to allow it to run.  Does the spec say null is allowed?  (Probably not)

It may make sense to leave it as-is - parity with other similar interfaces is probably the dominant factor.

::: dom/media/MediaManager.cpp
@@ +675,5 @@
>  
>    NS_ENSURE_TRUE(aParams, NS_ERROR_NULL_POINTER);
>    NS_ENSURE_TRUE(aWindow, NS_ERROR_NULL_POINTER);
> +  NS_ENSURE_TRUE(aOnError, NS_ERROR_NULL_POINTER);
> +  NS_ENSURE_TRUE(aOnSuccess, NS_ERROR_NULL_POINTER);

Do we want it NS_WARNING if they pass a null error pointer?  Null success is pretty silly, but again does it rise to NS_WARNING?

@@ +866,5 @@
>    NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
>  
> +  NS_ENSURE_TRUE(aOnError, NS_ERROR_NULL_POINTER);
> +  NS_ENSURE_TRUE(aOnSuccess, NS_ERROR_NULL_POINTER);
> +

Ditto

        Attachment #672875 -
        Flags: review?(rjesup) → review+

    
    

    
  
I've proposed making both success/error callbacks mandatory earlier, and surfaced it again recently in the error handling proposal: http://lists.w3.org/Archives/Public/public-webrtc/2012Oct/0030.html

Nobody has agreed (but nobody has disagreed either!), and we can always make it optional later.

        Attachment #672880 -
        Flags: review?(rjesup) → review+

    
    

    
  
(In reply to Anant Narayanan [:anant] from comment #24)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/a441f6e0afc9

Could you land the test with this patch? I don't have commit power.
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/a441f6e0afc9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

    
    

    
  
Verified on 10/21 build.
Status: RESOLVED → VERIFIED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: