1097804 - Create a library containing nsISocketTransportService and nsIDNS that can be used to support standalone WebRTC

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Randall Barker [:rbarker]]

To enable building a standalone WebRTC library, nsISocketTransportService and nsIDNS need to be able to built into a stand alone library that maybe used by the WebRTC library.

Comment 1

[Randall Barker [:rbarker]]

Attachment: Mini Necko v1

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8522523 [details] [diff] [review]
Mini Necko v1

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

::: netwerk/dns/nsHostResolver.cpp
@@ +215,1 @@
>      Telemetry::Accumulate(Telemetry::DNS_BLACKLIST_COUNT, mBlacklistedCount);

As discussed on IRC: a significant amount of these are only there for the Telemetry::Accumulate() calls, so we might want to stub/define those out instead.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8522523 [details] [diff] [review]
Mini Necko v1

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

This looks OK from an xpcom point of view, modulo the point raised below.  This is really in large part necko-stuff anyway, so I don't know that there's a lot for me to say here.

::: xpcom/libxpcomrt/XPCOMRTInit.cpp
@@ +101,5 @@
>  NS_ShutdownXPCOMRT()
>  {
>    nsresult rv = NS_OK;
> +  {
> +    nsCOMPtr<nsIThread> thread = do_GetCurrentThread();

Why are you scoping things here?  (I can sort of believe it's needed, but I would like clarification on this.  This change also seems like something that should go in the main xpcomrt patch, so that the network-related changes here are smaller and more obvious.)

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8522523 [details] [diff] [review]
Mini Necko v1

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

A couple thoughts - let's call it standalone instead of mini. We may want to expand this to support something like the iPhone over time.

this looks really easy to regress through normal necko hacking. how do we avoid that?

I concur a telemetry stub will get rid of a ton of ifdefs, please do that

I'm concerned that some of the stubs, like GetURLSpecFromFile() shouldn't just remove code and return NS_OK - that seems like a recipe for disaster. They should at least throw and if you think this code shouldn't be called at all then they should assert in the stubbed path. This applies to a lot of the call sites.

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +4,4 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsSocketTransportService2.h"
> +#if !defined(MOZILLA_XPCOMRT_API)

if you can rearrange this to be 1 or 2 ifdefs in the header list I would rather have that than preserve the sorting.

::: netwerk/mini/nsNetModuleMini.cpp
@@ +18,5 @@
> +#include "nsPISocketTransportService.h"
> +#include "nscore.h"
> +
> +extern const mozilla::Module kNeckoMiniModule;
> +

let's put this code in namespace mozilla::net

@@ +20,5 @@
> +
> +extern const mozilla::Module kNeckoMiniModule;
> +
> +nsresult
> +NS_InitNetModuleMini()

don't use the NS_ prefix anymore when we're in the namespace

Comment 5

[Randall Barker [:rbarker]]

Attachment: Necko Standalone v2

Updated to address review comments.

Comment 6

[Randall Barker [:rbarker]]

Attachment: Unicode lib needed by standalone Necko v1

Comment 7

[Randall Barker [:rbarker]]

Attachment: Necko Standalone v3

Added missing new files.

Comment 8

[Randall Barker [:rbarker]]

Attachment: Part 2: Unicode lib needed by standalone Necko v2

Added missing new files.

Comment 9

[Randall Barker [:rbarker]]

Comment on attachment 8544894 [details] [diff] [review]
Necko Standalone v3

I believe all review comments have been addressed.

Comment 10

[Randall Barker [:rbarker]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3)
> Comment on attachment 8522523 [details] [diff] [review]
> Mini Necko v1
> 
> Review of attachment 8522523 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks OK from an xpcom point of view, modulo the point raised below. 
> This is really in large part necko-stuff anyway, so I don't know that
> there's a lot for me to say here.
> 
> ::: xpcom/libxpcomrt/XPCOMRTInit.cpp
> @@ +101,5 @@
> >  NS_ShutdownXPCOMRT()
> >  {
> >    nsresult rv = NS_OK;
> > +  {
> > +    nsCOMPtr<nsIThread> thread = do_GetCurrentThread();
> 
> Why are you scoping things here?  (I can sort of believe it's needed, but I
> would like clarification on this.  This change also seems like something
> that should go in the main xpcomrt patch, so that the network-related
> changes here are smaller and more obvious.)

The scoping comes from the code I cribbed from. I lost the comment explaining the scoping: Block it so that the COMPtr will get deleted before we hit servicemanager shutdown.

I put the comment back in.

I misunderstood what you meant about moving the reformatting of the XPCOMRT Init function until right now. I'll try and address that and up load new patches for Bug 1093934 and this bug.

Comment 11

[Randall Barker [:rbarker]]

Attachment: Part 1: Necko Standalone v5

Comment 12

[Randall Barker [:rbarker]]

Comment on attachment 8545564 [details] [diff] [review]
Part 1: Necko Standalone v5

Fixed the formatting issue in earlier patch so there is less xpcom in this patch.

Comment 13

[Patrick McManus [:mcmanus]]

This is good. thanks.

I'm deeply concerned about how easy it would be to regress this. is a MOZILLA_XPCOMRT_API part of the ci? Is it considered a non-regressable target?

Comment 14

[Gian-Carlo Pascutto [:gcp]]

>I'm deeply concerned about how easy it would be to regress this. is a MOZILLA_XPCOMRT_API part of the ci? Is it considered a non-regressable target?

Together with this landing, a patch should land (bug 1101651) that causes the WebRTC unit tests to be linked against this library.

Comment 15

[Randall Barker [:rbarker]]

(In reply to Patrick McManus [:mcmanus] from comment #13)
> This is good. thanks.
> 
> I'm deeply concerned about how easy it would be to regress this. is a
> MOZILLA_XPCOMRT_API part of the ci? Is it considered a non-regressable
> target?

Yes, this will make things more complicated. The patches in bug 1101651 link against the MOZILLA_XPCOMRT_API targets so link breakage should be noticeable at build time. I do not believe the WebRTC unit test would reveal functionality breakage. Perhaps there are unit tests I can tie into to get better coverage?

Comment 16

[Patrick McManus [:mcmanus]]

I think build time failure is fine.. what test suite in the CI makes that build?

I want to make sure some variation of a try run would show the failure.

Comment 17

[Patrick McManus [:mcmanus]]

Comment on attachment 8545564 [details] [diff] [review]
Part 1: Necko Standalone v5

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

please double check regressions to this can be diagnosed via try server r=mcmanus

Comment 18

[Randall Barker [:rbarker]]

(In reply to Patrick McManus [:mcmanus] from comment #17)
> Comment on attachment 8545564 [details] [diff] [review]
> Part 1: Necko Standalone v5
> 
> Review of attachment 8545564 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> please double check regressions to this can be diagnosed via try server
> r=mcmanus

I have verified that mediapipeline_unittest, sdp_unittest, buffered_stun_socket_unittest, ice_unittest, nrappkit_unittest, rlogringbuffer_unittest, runnable_utils_unittest, simpletokenbucket_unittest, sockettransportservice_unittest, TestSyncRunnable, transport_unittests, and turn_unittest are all linked against standalone XPCOM and Necko and run on try.

Comment 19

[Patrick McManus [:mcmanus]]

Comment on attachment 8545564 [details] [diff] [review]
Part 1: Necko Standalone v5

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

I thought I set the r+ flag on this when I left my last comment, but I just saw it in my queue. Sorry!

Comment 20

[Randall Barker [:rbarker]]

Attachment: Part 1: Necko standalone v6

Rebased to tip of inbound.
Carry forward r+ from :mcmanus

Comment 21

[Randall Barker [:rbarker]]

Attachment: Part 2: Unicode lib needed by standalone Necko v3

Comment 22

[Randall Barker [:rbarker]]

Comment on attachment 8552754 [details] [diff] [review]
Part 2: Unicode lib needed by standalone Necko v3

Small moz.build addition to enable building standalone WebRTC library.

Comment 23

[Randall Barker [:rbarker]]

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

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8552754 [details] [diff] [review]
Part 2: Unicode lib needed by standalone Necko v3

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

You didn't need build peer review for this. But I'll give it to you.

Comment 25

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1097804-Part-1-Create-a-library-containing-nsISocketTransportService-and-nsIDNS-that-can-be-used-to-support-standalone-WebRTC-15020315-6905fce.patch

Rebased to current inbound.
Carry forward r+ from mcmanus

Comment 26

[Randall Barker [:rbarker]]

Attachment: 0002-Bug-1097804-Part-2-Added-xpcomrt-build-of-library-containing-unicode-util-functions-15020315-67d3b35.patch

Rebased to current inbound
carry forward r+ from :gps

Comment 27

[Randall Barker [:rbarker]]

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

Comment 28

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1097804-Part-1-Create-a-library-containing-nsISocketTransportService-and-nsIDNS-that-can-be-used-to-support-standalone-WebRTC-15040210-52bfe35.patch

Rebased to current inbound.
Carry forward r+ from mcmanus

Comment 29

[Randall Barker [:rbarker]]

Attachment: 0002-Bug-1097804-Part-2-Added-xpcomrt-build-of-library-containing-unicode-util-functions-15040210-e114cdc.patch

Rebased to current inbound.
Carry forward r+ from gps

Comment 30

[Randall Barker [:rbarker]]

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

Comment 31

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

https://hg.mozilla.org/integration/fx-team/rev/e1f294f7ca90
https://hg.mozilla.org/integration/fx-team/rev/c7da4d4c09aa

Comment 32

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

These were backed out so I could back out bug 1101651 for test failures:

https://hg.mozilla.org/integration/fx-team/rev/1f1973da3d17
https://hg.mozilla.org/integration/fx-team/rev/eebb50c44389

Comment 33

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1097804-Part-1-Create-a-library-containing-nsISocketTransportService-and-nsIDNS-that-can-be-used-to-support-standalone-WebRTC-15040315-ecd04e7.patch

Rebased to current inbound.
Carry forward r+ from mcmanus

Comment 34

[Randall Barker [:rbarker]]

Attachment: 0002-Bug-1097804-Part-2-Added-xpcomrt-build-of-library-containing-unicode-util-functions-15040315-2f64869.patch

Rebased to current inbound.
Carry forward r+ from gps

Comment 35

[Randall Barker [:rbarker]]

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

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f160bf4fd43
https://hg.mozilla.org/integration/mozilla-inbound/rev/3451d72b8e1a

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f160bf4fd43
https://hg.mozilla.org/mozilla-central/rev/3451d72b8e1a