969469 - Don't spin event loop during interface gathering on Gonk/WebRTC

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Eric Rescorla (:ekr)]

in gonk_addrs.c:


  // Get network interface list.
  std::vector<NetworkInterface> interfaces;
  if (NS_FAILED(NS_DispatchToMainThread(
                    mozilla::WrapRunnableNMRet(&GetInterfaces, &interfaces, &rv),
                    NS_DISPATCH_SYNC))) {
    return R_FAILED;
  }


This will spin the STS event loop which creates reentrancy and we don't
want it. Currently WebRTC is the only user of STS in content processes
on gonk, so please replace with SyncRunnable. In future, we may
consider rewriting this to pre-fetch the interface list asynchronously
and make this fast.

Comment 1

[Eric Rescorla (:ekr)]

Patrick, can you please do this for 1.3.

Comment 2

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Use SyncRunnable to get local interface on gonk

Comment 3

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

based on comment 1, nominating 1.3?.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #3)
> based on comment 1, nominating 1.3?.

What's the user impact for not doing this? What's the likelihood of this happening?

Comment 5

[Eric Rescorla (:ekr)]

(In reply to Jason Smith [:jsmith] from comment #4)
> (In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #3)
> > based on comment 1, nominating 1.3?.
> 
> What's the user impact for not doing this?

Unknown behavior.


> What's the likelihood of this
> happening?

Reasonably high.


Patrick, I think this may actually have some modest compilation
errors. Can you cleanup and do a try push?

Comment 6

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

I have pushed it to try: https://tbpl.mozilla.org/?tree=Try&rev=5b4e0eb4a4ec

Comment 7

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Jason Smith [:jsmith] from comment #4)
> (In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #3)
> > based on comment 1, nominating 1.3?.
> 
> What's the user impact for not doing this? What's the likelihood of this
> happening?

This is fix of bug 966802 on WebRTC side. The stack overflow is from this nested event loop.

Comment 8

[Eric Rescorla (:ekr)]

Comment on attachment 8377412 [details] [diff] [review]
Use SyncRunnable to get local interface on gonk

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

I know this doesn't compile, cause I missed some includes.

Patrick, can you fix this and bring it back for review?

Comment 9

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #8)
> I know this doesn't compile, cause I missed some includes.

I don't get compiling error in my local build. And try server builds this patch successfully. Maybe the header is included in other header. Which header should I include?

Comment 10

[Preeti Raghunath(:Preeti)]

Per comment #9

Comment 11

[Eric Rescorla (:ekr)]

Comment on attachment 8377412 [details] [diff] [review]
Use SyncRunnable to get local interface on gonk

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

Since I wrote this originally, asking Byron for review.

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8377412 [details] [diff] [review]
Use SyncRunnable to get local interface on gonk

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

lgtm

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/b2g-inbound/rev/25623a3aa663

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25623a3aa663

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Please request approval-mozilla-b2g28 on this patch as 1.3 blockers no longer have auto-approval for uplift.

Comment 16

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 8377412 [details] [diff] [review]
Use SyncRunnable to get local interface on gonk

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 867933
User impact if declined: Can cause stack overflow, program behavior becomes undefined.
Testing completed: Yes
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch:

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/81480906c296