If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED

Status

()

Core
WebRTC
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: posidron, Unassigned)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
x86_64
Mac OS X
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 701202 [details]
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
(Reporter)

Comment 1

5 years ago
Created attachment 701203 [details]
callstack
(Reporter)

Updated

5 years ago
Blocks: 786236
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
See Also: → bug 825703
(Reporter)

Comment 3

5 years ago
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. ;)
Resolution: INVALID → FIXED

Updated

5 years ago
Keywords: verifyme
No crash with testcase = verified.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?

Updated

5 years ago
Keywords: verifyme
Created attachment 706801 [details] [diff] [review]
Crashtest

Updated

5 years ago
Attachment #706801 - Flags: review?(rjesup)
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?
Attachment #706801 - Flags: review?(rjesup) → review+
(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?
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 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.
(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).
The bug in relation to what should happen actually is bug 773986.
Actually, let's drop this all-together. I think Randell's point is right that there's no point to checking this in.

Updated

5 years ago
Flags: in-testsuite? → in-testsuite-

Updated

5 years ago
Attachment #706801 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.