1304269 - Intermittent TEST-UNEXPECTED-TIMEOUT | dom/media/tests/mochitest/test_peerConnection_noTrickleOffer.html | application timed out after 330 seconds with no output

Status: RESOLVED DUPLICATE of bug 1304519

(Core :: WebRTC, defect, P3)

Description

[Treeherder Bug Filer]

Filed by: philringnalda [at] gmail.com

https://treeherder.mozilla.org/logviewer.html#?job_id=3744805&repo=autoland

https://archive.mozilla.org/pub/firefox/tinderbox-builds/autoland-win32-debug/1474380556/autoland_win7_ix-debug_test-mochitest-media-e10s-bm109-tests1-windows-build238.txt.gz

Comment 1

[Nils Ohlmeier [:drno]]

Hmm really bizarre. ICE logging claims both PC's successfully connected within the first second of the test running. But the mochitest logging claims the ICE connection state never progressed from 'new' on one side and 'checking' on the second PC to any other state.
I'm wondering if it would help to cleanup the waitForIceConnection() mess.

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: Bug 1304269: lets promise ice from now on.

Review commit: https://reviewboard.mozilla.org/r/80286/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80286/

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8793562 [details]
Bug 1304269: lets promise ice from now on.

https://reviewboard.mozilla.org/r/80286/#review79070

Seeing stuff that try barfed on, will re-review once that's cleared up.

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8793562 [details]
Bug 1304269: lets promise ice from now on.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80286/diff/1-2/

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8793562 [details]
Bug 1304269: lets promise ice from now on.

https://reviewboard.mozilla.org/r/80286/#review79152

With nits.

::: dom/media/tests/mochitest/pc.js:767
(Diff revision 2)
> +  this.iceConnectedReject;
> +  this.iceConnected = new Promise((resolve, reject) => {
> +    this.iceConnectedResolve = resolve;
> +    this.iceConnectedReject = reject;
> +  });
>    this.iceCheckingRestartExpected = false;

We can get rid of this, right?

::: dom/media/tests/mochitest/pc.js:787
(Diff revision 2)
> +    if (iceState === "connected") {
> +      this.iceConnectedResolve();
> +    } else if (iceState == "failed") {
> +      this.iceConnectedReject();
> +    }

Why === on one, but == on the other?

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8793562 [details]
Bug 1304269: lets promise ice from now on.

https://reviewboard.mozilla.org/r/80286/#review79152

> We can get rid of this, right?

No we can't because it is still used to decide when it is okay to have the ICE connection state to go backwards.

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8793562 [details]
Bug 1304269: lets promise ice from now on.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80286/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/552477d11cd0
lets promise ice from now on. r=bwc

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/552477d11cd0

Comment 10

[Ryan VanderMeulen [:RyanVM]]

What bug caused this?

Comment 11

[Nils Ohlmeier [:drno]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> What bug caused this?

There is no specific bug I point at, just my observation from comment #1. It's mostly that the test code was written less then ideal back in the days before we had Promises. This is simply an attempt to handle waiting for ICE to connect in a way more clean way. My hope is that this could actually solve some intermittents around ICE failing to connect. But we should wait and see.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Yeah, I'm mainly asking to assess which other branches might be affected.

Comment 13

[Nils Ohlmeier [:drno]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Yeah, I'm mainly asking to assess which other branches might be affected.

Sure. The not ideal test code has in our repo's since ages (way before 49). I was initially thinking to simply let this ride the train.
But I can take an action item to look at how our ICE orange numbers are looking next week. And then we decide if it's worth uplifting.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/57257bd35a87

Comment 15

[Andreas Pehrson [:pehrsons] (On leave; back Aug 1st 2020)]

Sorry to disappoint, this was bug 1304519.