Closed Bug 1097804 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 12 obsolete files)

23.38 KB, patch
Details | Diff | Splinter Review
2.15 KB, patch
Details | Diff | Splinter Review
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.
Blocks: 1079348
Assignee: nobody → rbarker
Depends on: 1093934
Attached patch Mini Necko v1 (obsolete) — Splinter Review
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.
Blocks: 1101651
Attachment #8522523 - Flags: review?(nfroyd)
Attachment #8522523 - Flags: review?(mcmanus)
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.)
Attachment #8522523 - Flags: review?(nfroyd) → feedback+
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
Attachment #8522523 - Flags: review?(mcmanus) → review-
Attached patch Necko Standalone v2 (obsolete) — Splinter Review
Updated to address review comments.
Attachment #8522523 - Attachment is obsolete: true
Attached patch Necko Standalone v3 (obsolete) — Splinter Review
Added missing new files.
Attachment #8544877 - Attachment is obsolete: true
Added missing new files.
Attachment #8544878 - Attachment is obsolete: true
Comment on attachment 8544894 [details] [diff] [review]
Necko Standalone v3

I believe all review comments have been addressed.
Attachment #8544894 - Flags: review?(mcmanus)
(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.
Attached patch Part 1: Necko Standalone v5 (obsolete) — Splinter Review
Attachment #8544894 - Attachment is obsolete: true
Attachment #8544894 - Flags: review?(mcmanus)
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.
Attachment #8545564 - Flags: review?(mcmanus)
Attachment #8545564 - Attachment description: Necko Standalone v5 → Part 1: Necko Standalone v5
Attachment #8544895 - Attachment description: Unicode lib needed by standalone Necko v2 → Part 2: Unicode lib needed by standalone Necko v2
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?
Flags: needinfo?(rbarker)
>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.
(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?
Flags: needinfo?(rbarker)
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 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
(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.
No longer blocks: 1079348
Blocks: 1079348
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!
Attachment #8545564 - Flags: review?(mcmanus) → review+
Attached patch Part 1: Necko standalone v6 (obsolete) — Splinter Review
Rebased to tip of inbound.
Carry forward r+ from :mcmanus
Attachment #8545564 - Attachment is obsolete: true
Attachment #8544895 - Attachment is obsolete: true
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.
Attachment #8552754 - Flags: review?(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.
Attachment #8552754 - Flags: review?(gps) → review+
Rebased to current inbound
carry forward r+ from :gps
Attachment #8552754 - Attachment is obsolete: true
Rebased to current inbound.
Carry forward r+ from gps
Attachment #8558856 - Attachment is obsolete: true
Keywords: checkin-needed
Rebased to current inbound.
Carry forward r+ from mcmanus
Attachment #8587527 - Attachment is obsolete: true
Flags: needinfo?(rbarker)
Rebased to current inbound.
Carry forward r+ from gps
Attachment #8587528 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6f160bf4fd43
https://hg.mozilla.org/mozilla-central/rev/3451d72b8e1a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla40
See Also: → 1340030
You need to log in before you can comment on or make changes to this bug.