Closed
Bug 838318
Opened 12 years ago
Closed 12 years ago
Crashtest 791330.html timeout because of missing try/catch
Categories
(Core :: WebRTC, defect)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: whimboo, Assigned: whimboo)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 1 obsolete file)
|
1.80 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
At the moment 791330.html runs into a timeout because we don't catch the failure which is thrown when you call addStream() after the peer connection has been closed. The upcoming patch will fix that.
|
Assignee | |
Comment 1•12 years ago
|
||
Attachment #710359 -
Flags: review?(rjesup)
|
||
Comment 2•12 years ago
|
||
Comment on attachment 710359 [details] [diff] [review]
Patch v1
Review of attachment 710359 [details] [diff] [review]:
-----------------------------------------------------------------
review- only cause you need to split this patch up and my head is turning weirdly seeing the commented out code. One patch per purpose.
::: dom/media/tests/crashtests/791330.html
@@ +26,1 @@
> }, function () {});
Might as well also fix this here too by adding a finish call as well instead of an empty function. Even though we shouldn't hit this path, may be worth to stick it in while you are cleaning this up.
::: dom/media/tests/crashtests/799419.html
@@ +5,5 @@
> -->
> <head>
> <meta charset="utf-8">
> <title>2 Peer Connections Create and Close + Fake Video</title>
> <script type="application/javascript">
One patch per purpose. Split this off into a separate patch in a separate bug.
@@ +11,5 @@
> var v0 = mozRTCPeerConnection();
> var v1 = mozRTCPeerConnection();
> +
> +// navigator.mozGetUserMedia({video:true, fake: true},
> +// function(stream) {}, function() {});
Uhh...commented out? Why? If we don't need it, get rid of it.
Attachment #710359 -
Flags: review?(rjesup) → review-
|
|
Assignee | |
Comment 3•12 years ago
|
||
Comment on attachment 710359 [details] [diff] [review]
Patch v1
This is not the right version of the patch. :/
Attachment #710359 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 4•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #2)
> Comment on attachment 710359 [details] [diff] [review]
> ::: dom/media/tests/crashtests/791330.html
> @@ +26,1 @@
> > }, function () {});
>
> Might as well also fix this here too by adding a finish call as well instead
> of an empty function. Even though we shouldn't hit this path, may be worth
> to stick it in while you are cleaning this up.
No, this is correct. As you say we should never hit this case. If it happens it's a bug and we have to be made aware of it. It will only happen when we leave it as is and we run into a timeout.
|
|
Assignee | |
Comment 5•12 years ago
|
||
Missed to qref the latest changes after experimenting with an opt build on the inbound branch.
Attachment #710362 -
Flags: review?(rjesup)
|
||
Updated•12 years ago
|
Attachment #710362 -
Flags: review?(rjesup) → review+
|
|
||
Updated•12 years ago
|
Keywords: checkin-needed
|
|
Assignee | |
Comment 6•12 years ago
|
||
Flags: in-testsuite+
|
||
Comment 7•12 years ago
|
||
Please clear checkin-needed when pushing to inbound.
Keywords: checkin-needed
|
|
||
Comment 8•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
|
|
||
Updated•12 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•