Closed Bug 969469 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: WebRTC: Networking, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
blocking-b2g 1.3+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: ekr, Assigned: kk1fff)

Details

Attachments

(1 file)

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.
Patrick, can you please do this for 1.3.
Assignee: nobody → pwang
based on comment 1, nominating 1.3?.
blocking-b2g: --- → 1.3?
(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?
(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?
(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.
blocking-b2g: 1.3? → 1.3+
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?
Attachment #8377412 - Flags: review?(ekr)
(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?
Per comment #9
Flags: needinfo?(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.
Attachment #8377412 - Flags: review?(docfaraday)
Comment on attachment 8377412 [details] [diff] [review]
Use SyncRunnable to get local interface on gonk

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

lgtm
Attachment #8377412 - Flags: review?(docfaraday) → review+
https://hg.mozilla.org/mozilla-central/rev/25623a3aa663
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Please request approval-mozilla-b2g28 on this patch as 1.3 blockers no longer have auto-approval for uplift.
Flags: needinfo?(ekr) → needinfo?(pwang)
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:
Attachment #8377412 - Flags: approval-mozilla-b2g28?
Flags: needinfo?(pwang)
Attachment #8377412 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
You need to log in before you can comment on or make changes to this bug.