Closed Bug 1022896 Opened 5 years ago Closed 5 years ago

B2G network setup & teardown in WebRTC tests is broken

Categories

(Core :: WebRTC: Networking, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: drno, Assigned: drno)

Details

Attachments

(1 file, 1 obsolete file)

As seen in this log file: https://tbpl.mozilla.org/php/getParsedLog.php?id=41343219&tree=Try&full=1#error0

1) The current B2G network setup code proceeds with instantiating the PC's even though the network setup has not finished (and ICE gathering starts right after PC creation).
2) The network teardown happens before PC's get closed.
3) Tests which are not using the PeerConnectionWrapper have no network B2G network setup/teardown at all.
This patch moves the network initialization before the test and only starts the tests function once the network has been initialized. It also moves the teardown into a wrapper function which needs to be called in case PC's get created inside the test function itself (oppose to using the PeerConnectionWrappers).
I decided against jib's suggestion of overloading SimpleTest, because the name overloading makes it more complicated to understand and maintain the code. But this solution is less risky then my last attempt, because it is straight forward search and replace s/SimpleTest.finish/finishNetworkTest/g

Try run is green: https://tbpl.mozilla.org/?tree=Try&rev=46d758f1e416
Attachment #8439409 - Flags: review?(rjesup)
Attachment #8439409 - Flags: review?(jib)
Attachment #8439409 - Flags: review?(rjesup) → review+
Comment on attachment 8439409 [details] [diff] [review]
bug_1022896_fix_b2g_network_setup.patch

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

Not sure what runTest(func, true) does different from runTest(func), but it seems like this should be preserved. r=me with that.

::: dom/media/tests/mochitest/pc.js
@@ +395,5 @@
>       */
> +    tearDownNetwork: function(onSuccess, onFailure) {
> +      if (isNetworkReady()) {
> +        script.addMessageListener('network-disabled', function (message) {
> +          info("Network interface teared down");

torn

@@ +414,5 @@
>  
>  /**
> + * Check if network setup for Gonk is needed
> + *
> + * @return {function} provides a function to teardown the network (if needed)

Obsolete comment now that function no longer returns anything.

@@ +416,5 @@
> + * Check if network setup for Gonk is needed
> + *
> + * @return {function} provides a function to teardown the network (if needed)
> + */
> +function startNetworkAndGetTeardownFunction(onSuccess) {

Misleading name now that function no longer returns anything.

::: dom/media/tests/mochitest/test_dataChannel_basicAudio.html
@@ -21,4 @@
>      test = new DataChannelTest();
>      test.setMediaConstraints([{audio: true}], [{audio: true}]);
>      test.run();
> -  }, true);

New code effectively calls runTest(func) while old code called runTest(func, true) here and a number of other places. Is change intentional?
Attachment #8439409 - Flags: review?(jib) → review+
Addressed jib's findings.

Carrying forward r+=jib,jesup
Attachment #8439409 - Attachment is obsolete: true
(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> ::: dom/media/tests/mochitest/test_dataChannel_basicAudio.html
> @@ -21,4 @@
> >      test = new DataChannelTest();
> >      test.setMediaConstraints([{audio: true}], [{audio: true}]);
> >      test.run();
> > -  }, true);
> 
> New code effectively calls runTest(func) while old code called runTest(func,
> true) here and a number of other places. Is change intentional?

This is an intentional cleanup, as runTest() takes only one argument.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9dd148f66997
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.