1126036 - NrIceCtx::StartChecks called before NrIceCtx::StartGathering , leading to failure to create STUN peer ctxs

Status: VERIFIED FIXED

(Core :: WebRTC: Networking, defect)

Comment 1

[Byron Campen [:bwc]]

Attachment: MozReview Request: bz://1126036/bwc

/r/3001 - Bug 1126036: Queue runnables for starting gathering and checking in PCMedia until the proxy lookup is complete.

Pull down this commit:

hg pull review -r 5dcd67bcc05cc5d88d2a7e11c0cb95bd4b49e617

Comment 2

[Byron Campen [:bwc]]

try (first try push ended up looking funny, so did again, check parent of try changeset if you like):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f68cf96f363b

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8554870 [details]
MozReview Request: bz://1126036/bwc

https://reviewboard.mozilla.org/r/2999/#review2319

I think that I'd like to see this a little better encapsulated.  I don't think that that even has performance penalties.  It would reduce some of the duplication though.

That suggests that two methods are exposed:
PerformIceCtxOperation(op); -> enqueue operation, or dispatch if already running
SetIceCtxReady(); -> triggers the release of all those operations to STS

If you make PerformIceCtxOperation clever enough, you might avoid a refcount operation, even.  I wouldn't do that though.  Clever is bad.

Comment 4

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/2999/#review2321

Sure, why not.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8554870 [details]
MozReview Request: bz://1126036/bwc

/r/3001 - Bug 1126036: Queue runnables for starting gathering and checking in PCMedia until the proxy lookup is complete.

Pull down this commit:

hg pull review -r 7f4f3404d41e88dfbe9155f3c2cc1c177d1d0bdc

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8554870 [details]
MozReview Request: bz://1126036/bwc

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7592d95ee169

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8554870 [details]
MozReview Request: bz://1126036/bwc

https://reviewboard.mozilla.org/r/2999/#review2323

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
(Diff revision 2)
>      pcm_->mProxyResolveCompleted = true;
> -    pcm_->GatherIfReady();
> +    pcm_->FlushIceCtxOperationQueueIfReady();

It's odd to call an "IfReady" function immediately after setting the ready variable.

Maybe move the assignment inside the function, and call it SetIceCtxReady().

Comment 8

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/2999/#review2325

> It's odd to call an "IfReady" function immediately after setting the ready variable.
> 
> Maybe move the assignment inside the function, and call it SetIceCtxReady().

Idea here is that someday we may have more than one bool that guards this, and the caller shouldn't have to know all of them.

Comment 10

[Richard Pappalardo [:rpapa][:rpappalardo]]

Adding NI as self-reminder to re-verify once patch has landed

Comment 11

[Maire Reavy [:mreavy]]

This is affecting enough folks that I'd like to ask the sheriffs to land this directly on m-c as a bustage fix and then ask them to re-spin today's Nightly.

Byron -- Is the patch ready to land, or are there more changes you'd like to make?

Comment 12

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c82fb962499a

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c82fb962499a

Comment 14

[Richard Pappalardo [:rpapa][:rpappalardo]]

works for me now

Comment 15

[Byron Campen [:bwc]]

\o/

Comment 16

[ryan]

Comment on attachment 8554870 [details]
MozReview Request: bz://1126036/bwc

Approval Request Comment
[Feature/regressing bug #]: 1126036
[User impact if declined]: Issue 94907 cannot be resolved
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: 
[String/UUID change made/needed]: No

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to ryan from comment #16)
> Comment on attachment 8554870 [details]
> MozReview Request: bz://1126036/bwc
> 
> Approval Request Comment
> [Feature/regressing bug #]: 1126036
You have referenced this bug. Is there another bug that introduced this issue? In what Firefox release was this issue first introduced?

> [User impact if declined]: Issue 94907 cannot be resolved
This doesn't look like a Bugzilla reference. Can you please provide a link and explain the impact to the Firefox user base? From comment 11, I take it that this does have end user impact.

> [Describe test coverage new/current, TreeHerder]: None
How has this change been tested?

> [Risks and why]: 
What potential issues do you think there may be from landing this patch? What parts of the code does it touch? What might break? What cases have you been unable to test?

Comment 18

[Byron Campen [:bwc]]

The regressing bug was bug 949703, which was also requested for uplift.

Comment 19

[ryan]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #17)
> (In reply to ryan from comment #16)
> > Comment on attachment 8554870 [details]
> > MozReview Request: bz://1126036/bwc
> > 
> > Approval Request Comment
> > [Feature/regressing bug #]: 1126036
> You have referenced this bug. Is there another bug that introduced this
> issue? In what Firefox release was this issue first introduced?


Sorry, it's not always clear what the fields require or how they relate to the issue.

> > [User impact if declined]: Issue 94907 cannot be resolved
> This doesn't look like a Bugzilla reference. Can you please provide a link
> and explain the impact to the Firefox user base? From comment 11, I take it
> that this does have end user impact.

Type-o: 949703

> > [Describe test coverage new/current, TreeHerder]: None
> How has this change been tested?

The testing involved within 949703

> > [Risks and why]: 
> What potential issues do you think there may be from landing this patch?
> What parts of the code does it touch? What might break? What cases have you
> been unable to test?

Same as 949703: as this is a fix to thread-related issue related to 949703

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8554870 [details]
MozReview Request: bz://1126036/bwc

As this is a prereq for bug 949703 and I did not approve that bug for uplift, I'm going to deny this one as well. If there is more that I should know to consider these bugs for uplift, let's continue the conversation in bug 949703.

Aurora-

Comment 22

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1126036: Queue runnables for starting gathering and checking in PCMedia until the proxy lookup is complete.