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

VERIFIED FIXED in mozilla19

Status

()

defect
--
critical
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: jruderman, Assigned: anant)

Tracking

(Blocks 1 bug, {crash, testcase})

Trunk
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(4 attachments, 4 obsolete attachments)

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
Posted file stack trace
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
Posted file Testcase
I'm able to reproduce for both cases. Patch incoming.
Posted patch Crashtest v1 (obsolete) — Splinter Review
Attachment #672696 - Attachment description: Crashtest v1 → Crashtest v1 - Not tested
Attachment #672696 - Attachment is obsolete: true
Posted patch Crashtest v2 (obsolete) — 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
Posted patch Crashtest v3 (obsolete) — Splinter Review
Attachment #672794 - Flags: review?(hskupin)
Duplicate of this bug: 803052
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)
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: 7 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.