829691 - WebRTC crash [@sipcc::PeerConnectionImpl::CheckThreadInt]

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Tested with m-c changeset: 118260:f60b87eed1ac with the following patches applied:

https://bugzilla.mozilla.org/attachment.cgi?id=698334
https://bugzilla.mozilla.org/attachment.cgi?id=699413

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

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

The crash was real, but only in an unfinished patch in Bug 825703. I've uploaded a new patch in that bug that doesn't have the problem. Please update.

Thanks for testing it. I'm closing it as Invalid though for clarity, since, technically, the crash never made it to the tree.

Comment 3

[Christoph Diehl [:posidron]]

Yep, fixed but let's mark it as fixed. INVALID means there was no bug at all, which we usually set for some wired bug filings. We have a testcase here and your patch get's checked in later anyways, so it will be valid in the near term. ;)

Comment 4

[Jason Smith [:jsmith]]

No crash with testcase = verified.

Comment 5

[Jason Smith [:jsmith]]

Attachment: Crashtest

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 706801 [details] [diff] [review]
Crashtest

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

r+, but do we need a crashtest for a crash that only happened when a set of unfinished patches were applied?

Comment 7

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #6)
> Comment on attachment 706801 [details] [diff] [review]
> Crashtest
> 
> Review of attachment 706801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+, but do we need a crashtest for a crash that only happened when a set of
> unfinished patches were applied?

I don't think it necessarily hurts to have it. Is there any maintenance cost to keeping it around?

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Sure, in that it's testing for something very unlikely to be re-introduced (as it was an unfinished patch).  All tests have a cost, especially crashtests as they each run in a new instance IIRC - you have to spin up a browser and run a testcase (or at least a new window or load a new page).  Multiply by N checkins and M platforms and X try pushes....  Each one doesn't eat a lot, but building up testcases for extremely unlikely recurrences doesn't help.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 706801 [details] [diff] [review]
Crashtest

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

::: dom/media/tests/crashtests/829691.html
@@ +9,5 @@
> +  <script type="application/javascript">
> +    function start() {
> +      var v0 = new window.mozRTCPeerConnection({});
> +      var v0 = new window.mozRTCPeerConnection();
> +      navigator.mozGetUserMedia({"muted":false,"fake":false}, function(v5) {

Beside the discussion if we need it or not, this might not do what it is intended to do. While this will work with a manual test on the users machine it will not find any device on our testing machines. Was fake=false explicitely necessary here? If not it should be removed before its possible landing.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Comment on attachment 706801 [details] [diff] [review]
> Crashtest
> 
> Review of attachment 706801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/tests/crashtests/829691.html
> @@ +9,5 @@
> > +  <script type="application/javascript">
> > +    function start() {
> > +      var v0 = new window.mozRTCPeerConnection({});
> > +      var v0 = new window.mozRTCPeerConnection();
> > +      navigator.mozGetUserMedia({"muted":false,"fake":false}, function(v5) {
> 
> Beside the discussion if we need it or not, this might not do what it is
> intended to do. While this will work with a manual test on the users machine
> it will not find any device on our testing machines. Was fake=false
> explicitely necessary here? If not it should be removed before its possible
> landing.

There's no impact on that actually. The call made here would be equivalent of calling gUM with no parameters. Which from what I see actually doesn't do anything (which is probably a bug in itself).

Comment 11

[Jason Smith [:jsmith]]

The bug in relation to what should happen actually is bug 773986.

Comment 12

[Jason Smith [:jsmith]]

Actually, let's drop this all-together. I think Randell's point is right that there's no point to checking this in.