1272101 - [FlyWeb] Review Necko changes for landing in mozilla-central

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Kannan Vijayan [:djvj]]

Attachment: flyweb-network.patch

Sub-bug for reviewing Necko changes for landing in m-c.

Comment 1

[Kannan Vijayan [:djvj]]

Comment on attachment 8751441 [details] [diff] [review]
flyweb-network.patch

This bug blocks the main review bug for FlyWeb (bug 1272092).  The main review bug directly attaches all the patches relating to FlyWeb, so it can be used to find code in other implementation areas.

Comment 2

[u408661]

Kannan - FYI, I'm going to be on PTO starting tomorrow until Monday (May 16), so (barring a miracle happening today) I won't get to this review until then. It will be my top priority on Monday, though, so don't think I've forgotten about you :)

Comment 3

[Kannan Vijayan [:djvj]]

Thanks for the heads up!

Comment 4

[u408661]

Comment on attachment 8751441 [details] [diff] [review]
flyweb-network.patch

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

OK, here goes round one. Looks pretty good so far. I'll apologize if some of the comments seem out of order, I didn't review the files in the order splinter lists them (which is not necessarily the most useful ordering). Also, if any comments are a bit silly - well, that's what happens when I get to the end of a long day of doing nothing but reviews, sorry :)

One thing I would like to see across all new methods is MOZ_ASSERTs to ensure they're running on the proper thread (assuming the code isn't explicitly intended to run on more than one thread).

I'd also like to know the status of a security review for this (especially, but not limited to, fuzzing the servers).

One stylistic nit that I probably didn't catch nearly all of was instances of

  if (...) {
    ...
  }
  else {
    ...
  }

should be

  if (...) {
    ...
  } else {
    ...
  }

with the "else {" on the same line as the closing }.

There's probably others that I've missed, as well. Oh well, not the end of the world :)

For the next round, if you could post both the full diff (as it is now), as well as an interdiff from this version to the new version (so I can see what changed) that would be awesome. Also, this was a big patch. Don't be surprised if I see some things next time around that I missed this time, even if nothing changes in the bits I call out next time :)

::: dom/flyweb/FlyWebConnection.cpp
@@ +77,5 @@
> +  return FlyWebConnectionBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +NS_METHOD
> +FlyWebConnection::ReadSegmentsFunc(nsIInputStream* aIn,

Again (or perhaps first, based on ordering in splinter) - call this WriteSegmentsFunc? (There's another comment on a similarly-named thing that will explain why)

@@ +337,5 @@
> +
> +  char header[HeaderSize];
> +  NetworkEndian::writeUint32(header, utf8Data.Length());
> +
> +  char term[HeaderSize];

Might be more understandable to fully write out "terminator" here - took me a second to realize what this was.

@@ +340,5 @@
> +
> +  char term[HeaderSize];
> +  NetworkEndian::writeUint32(term, 0);
> +
> +  nsCString buffer = NS_LITERAL_CSTRING("M") +

It might be better to have these message types (I'm assuming that's what they are) defined somewhere with descriptive names, instead of trying to figure out what "M" (or "F", or "E", or ...) means based on the function context

@@ +413,5 @@
> +    rv = sts->CreateInputTransport(body,
> +                                   /* aStartOffset */ 0,
> +                                   /* aReadLimit */ -1,
> +                                   /* aCloseWhenDone */ true,
> +                                   getter_AddRefs(transport));

Might be yet another stylistic difference, but I find the param name comments work better after the param

(body,
 0, /* aStartOffset */
 -1, /* aReadLimit */
 ...);

since that way the param is always first and I don't have to scan the line for it, and the name serves only as a secondary reminder of what the param maps to.

@@ +419,5 @@
> +
> +    nsCOMPtr<nsIInputStream> wrapper;
> +    rv = transport->OpenInputStream(/* aFlags */ 0,
> +                                    /* aSegmentSize */ 0,
> +                                    /* aSegmentCount */ 0,

Same with ordering of param & comment

::: dom/flyweb/FlyWebService.cpp
@@ +86,5 @@
> +
> +  bool mDiscoveryActive;
> +  uint32_t mNumConsecutiveStartDiscoveryFailures;
> +
> +  DiscoveryState mDiscoveryState;

Is there a reason we don't just have mDiscoveryActive as another state within mDiscoveryState? Seems like there's an opportunity for things to become out-of-sync

@@ +93,5 @@
> +  nsCOMPtr<nsIDNSServiceDiscovery> mDNSServiceDiscovery;
> +  nsCOMPtr<nsICancelable> mCancelDiscovery;
> +  nsTHashtable<nsStringHashKey> mNewServiceSet;
> +
> +  struct DiscoveredInfo 

nit: trailing whitespace

@@ +194,5 @@
> +
> +  // Clear the new service array.
> +  mNewServiceSet.Clear();
> +
> +  // If mDiscoveryActive is false, then stop discovery immediately.

Better comment here - the "mDiscoveryActive is false" bit is obvious from the code, but what does it *mean*?

@@ +216,5 @@
> +  LOG_I("///////////////////////////////////////////");
> +  MOZ_ASSERT(mDiscoveryState == DISCOVERY_STOPPING);
> +  mDiscoveryState = DISCOVERY_IDLE;
> +
> +  // If mDiscoveryActive is false, then discard all results and do not proceed.

Again, what does mDiscoveryActive is false mean? I assume it means we're not actively running a discovery process, but could be clearer from the comment

@@ +235,5 @@
> +
> +  // Notify FlyWebService of changed service list.
> +  mService->NotifyDiscoveredServicesChanged();
> +
> +  // Start discovery again immediately.

Does this risk some kind of stop/start/stop/start/... loop? What are the reasons OnDiscoveryStopped could fire, and why is it safe to restart immediately?

@@ +292,5 @@
> +  MOZ_ASSERT(mDiscoveryState == DISCOVERY_STOPPING);
> +  mDiscoveryState = DISCOVERY_IDLE;
> +
> +  // If discovery is active, start discovery again immediately.
> +  if (mDiscoveryActive) {

Ah, I think this (and OnStartDiscoveryFailed) have answered my question about separate mDiscoveryActive/mDiscoveryState. Does it maybe make sense to rename mDiscoveryActive to something like mDiscoveryShouldBeActive? That seems to fit its purpose better (at least here). Up to you.

@@ +630,5 @@
> +  return mServiceMap.Contains(aServiceId);
> +}
> +
> +nsresult
> +FlyWebMDNSService::ConnectToService(const nsAString& aServiceId,

I don't see any actual connection going on here, only creation of a uri, and setting up some info about a connection (which may or may not exist at this point, I'm not sure)

@@ +688,5 @@
> +    FlyWebService::GetOrCreate()->FindPublishedServerByName(aServer->GetName());
> +  MOZ_ASSERT(existingServer);
> +
> +  // FIXME: Ensure that there is not already a service being
> +  // advertised under the same name.

This FIXME and the MOZ_ASSERT above it seem to be doing/implying opposite things...

@@ +733,5 @@
> +
> +void
> +FlyWebMDNSService::EnsureDiscoveryStopped()
> +{
> +  mDiscoveryActive = false;

This does not appear to actually ensure discovery is stopped - nowhere is StopDiscovery called :)

@@ +836,5 @@
> +
> +  httpServer->Init(-1, server);
> +  mServers.AppendElement(server);
> +  auto autoRemovePublishedServer = MakeScopeExit([&] {
> +    if (httpServer) { httpServer->Close(); }

Again, maybe a stylistic difference between dom and necko, but prefer not having if body on the same line, even for one-liners

@@ +868,5 @@
> +  if (aRv.Failed()) {
> +    return nullptr;
> +  }
> +
> +  return promise.forget();

This does not appear to actually connect to a server (nor does the promise created actually seem to have anything directly to do with connecting to a server). Should this be named differently? Is this creating a promise that will be resolved when the connection completes?

::: dom/flyweb/HttpServer.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

So generally speaking in this file - are there particular threads that the methods are expected to be called on? Main thread? Socket thread? Please MOZ_ASSERT for any thread you can.

Also, I've noticed a few HTTP cases that aren't handled - Transfer-Encoding: chunked comes to mind. I know we can probably rely on Firefox to not do anything crazy, but someone trying to be malicious won't be acting how we expect :) Have we had anyone fuzz this? We should.

@@ +49,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsIPrefBranch> prefService;
> +  prefService = do_GetService(NS_PREFSERVICE_CONTRACTID);
> +
> +  if (Preferences::GetBool("flyweb.use-tls", false)) {

I would rather use-tls default to true in code, and only be made false if the pref is explicitly set. Also, do we have this in all.js somewhere?

@@ +54,5 @@
> +    nsCOMPtr<nsILocalCertService> lcs =
> +      do_CreateInstance("@mozilla.org/security/local-cert-service;1");
> +    rv = lcs->GetOrCreateCert(NS_LITERAL_CSTRING("flyweb"), this);
> +  }
> +  else {

dom may have different conventions from necko, but shouldn't this else be on the same line as the closing brace?

@@ +326,5 @@
> +  return NS_OK;
> +}
> +
> +NS_METHOD
> +HttpServer::Connection::ReadSegmentsFunc(nsIInputStream* aIn,

This might be better called WriteSegmentsFunc, since its type is nsWriteSegmentFun (SO MUCH FUN!), and it is, in fact, performing the part of WriteSegments opposite your call to ReadSegments.

BTW, if you loathe the naming of {Read,Write}Segments, you're not alone. But, they're old and cranky, so what're you gonna do?

@@ +365,5 @@
> +
> +nsresult
> +HttpServer::Connection::ConsumeInput(const char*& aBuffer,
> +                                     const char* aEnd)
> +{

So I worry a little bit about this - necko already has a lot of HTTP parsing code (though not necessarily for requests), and HTTP/1.1 is... finicky, to say the least :) Is there any way we can look into unifying or using what necko already has available? Doing so would likely avoid potential issues in the future.

@@ +397,5 @@
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      mInputBuffer.Truncate();
> +    }
> +    else {

again, |} else {| ?

@@ +416,5 @@
> +      // Since we've given the pipe unlimited size, we should never
> +      // end up needing to block.
> +      MOZ_ASSERT(rv != NS_BASE_STREAM_WOULD_BLOCK);
> +      if (NS_FAILED(rv)) {
> +        written = size;

This feels suspect to me - if the write failed, why would we just pretend we wrote |size| bytes?

@@ +445,5 @@
> +  return found;
> +}
> +
> +static bool
> +IsWebSocketRequest(InternalRequest* aRequest, uint32_t aHttpVersion)

We should have Michal look over websocket stuff, since he knows more about it than I do - probably just a quick once-over to ensure it fits the protocol would be fine here.

@@ +457,5 @@
> +  if (!str.EqualsLiteral("GET")) {
> +    return false;
> +  }
> +
> +  InternalHeaders* headers = aRequest->Headers();

So I see (I think) that this type comes from the Fetch code - is there any particular reason we didn't use nsHttp{Request,Response}Head from necko? They might need some extending to fit this use case (for example, parsing request headers instead of having them supplied by necko), but it might be something to think about for a follow-up, so we're not duplicating effort.

@@ +557,5 @@
> +    mPendingReq->Headers()->SetGuard(HeadersGuardEnum::Immutable, res);
> +
> +    // Check for WebSocket
> +    if (IsWebSocketRequest(mPendingReq, mPendingReqVersion)) {
> +      LOG_V("HttpServer::Connection::ConsumeLine(%p) - Fire OnRequest", this);

Should this say "Fire OnWebSocket"?

@@ +601,5 @@
> +      nsresult rv;
> +      mRemainingBodySize = header.ToInteger(&rv);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    }
> +    else {

} else {

@@ +609,5 @@
> +    if (mRemainingBodySize) {
> +      LOG_V("HttpServer::Connection::ConsumeLine(%p) - Starting consume body", this);
> +      mState = eBody;
> +
> +      // We use an unlimited buffer size here to ensure

Unlimited buffer worries me - sounds like a potential for a DoS. I'm not saying "no" to it (it may be reasonable in the 99 percentile case), but I want to make sure we think really hard about it before moving forward.

@@ +625,5 @@
> +
> +      mCurrentRequestBody = do_QueryInterface(output);
> +      mPendingReq->SetBody(input);
> +    }
> +    else {

} else {

@@ +706,5 @@
> +    if (pending.first() == aRequest) {
> +      MOZ_ASSERT(!handledResponse);
> +      MOZ_ASSERT(!pending.second());
> +
> +      pending.second() = aResponse;

Is there another way to do this? It always looks weird, assigning a value to the result of a function call... (maybe I'm just too set in my ways, though)

@@ +1029,5 @@
> +    if (mFirstChunk) {
> +      mFirstChunk = false;
> +      MOZ_ASSERT(mSeparator.IsEmpty());
> +    }
> +    else {

} else {

@@ +1179,5 @@
> +        }
> +      }
> +      mOutputBuffers.RemoveElementAt(0);
> +    }
> +    else {

} else {

::: dom/flyweb/HttpServer.h
@@ +14,5 @@
> +#include "nsIAsyncOutputStream.h"
> +#include "mozilla/Variant.h"
> +#include "nsIRequestObserver.h"
> +#include "mozilla/MozPromise.h"
> +#include "nsITransportProvider.h"

I don't see this file anywhere, and it doesn't appear to be in current m-c. Missing from this patch?

::: dom/webidl/FlyWebConnection.webidl
@@ +11,5 @@
> +
> +interface FlyWebConnection : EventTarget {
> +  //readonly attribute DOMString serviceId;
> +  //readonly attribute DOMString sessionId;
> +  //readonly attribute DOMString? origin; // Website running other peer

Would be good to know why some of the attributes here are commented out (not just these 3, some below, as well)

::: mobile/android/base/java/org/mozilla/gecko/NetworkInfoService.java
@@ +1,1 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*-

Just a note: my java knowledge is pretty low, so you might want to have someone with more java knowledge look this file over :) (also, a mobile/android peer, since I'm not officially allowed to r+ this - you might have this included in one of the other reviews already, though)

@@ +1,5 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*-
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> + 

nit: trailing whitespace (there's a lot in this file - just look at splinter to find them)

@@ +112,5 @@
> +        String model = Build.MODEL;
> +        if (model.startsWith(manufacturer)) {
> +            return capitalize(model);
> +        }
> +        return capitalize(manufacturer) + " " + model;

This assumes (perhaps rightly) that |manufacturer| will never be an empty string - might be better to call |capitalize| on the final concatenated string? (That would also require not adding a space if |manufacturer| is empty)

@@ +124,5 @@
> +        char ch0 = str.charAt(0);
> +        if (Character.isLetter(ch0) && Character.isLowerCase(ch0)) {
> +            boolean upcase = true;
> +            // But don't capitalize the first letter if it's an 'i' followed
> +            // by a non-lowercase letter.  Sheesh.

"Sheesh" indeed. Also, please expand on why "i+LETTER" is a special case.

@@ +129,5 @@
> +            if (ch0 == 'i') {
> +                upcase = false;
> +                if (str.length() >= 2) {
> +                    char ch1 = str.charAt(1);
> +                    if (Character.isLetter(ch1) && Character.isLowerCase(ch1)) {

Maybe invert the condition and set upcase = false here. It's easy to lose the |upcase = false| you have a few lines up, so the |upcase = true| here looks at first glance like you're just re-setting true.

::: netwerk/base/NetworkInfoServiceAndroid.jsm
@@ +47,5 @@
> +  listNetworkAddresses(aListener) {
> +    send(":ListNetworkAddresses", {}, (result, err) => {
> +      if (err) {
> +        log("ListNetworkAddresses Failed: (" + err + ")");
> +        aListener.onListNetworkAddressesFailed(err);

Here you call onList..Failed with an error argument, but the idl for the listener shows Failed taking no arguments.

@@ +59,5 @@
> +  getHostname(aListener) {
> +    send(":GetHostname", {}, (result, err) => {
> +      if (err) {
> +        log("GetHostname Failed: (" + err + ")");
> +        aListener.onGetHostnameFailed(err);

Again, argument here, no argument in listener idl

::: netwerk/base/NetworkInfoServiceCocoa.cpp
@@ +19,5 @@
> +namespace mozilla {
> +namespace net {
> +
> +static nsresult
> +ListInterfaceAddresses(int aFd, const char* aIface, AddrMapType& aAddrMap);

Hrm... I feel like we should be able to unify the Linux & OS X implementations into a NetworkInfoServiceUnix.cpp. So much duplicate code makes me sadface.

@@ +93,5 @@
> +    return NS_OK;
> +}
> +
> +nsresult
> +GetHostnameImpl(nsACString& aHostname)

So after going through the third one of these - on all machines that use the native (not js-bridged-to-java) implementation, it appears that GetHostnameImpl is 100% the same on all of them. Just unify them (while addressing review comments I made in the other two implementations).

::: netwerk/base/NetworkInfoServiceImpl.h
@@ +15,5 @@
> +nsresult
> +ListAddressesImpl(AddrMapType& aAddrMap);
> +
> +nsresult
> +GetHostnameImpl(nsACString& aHostname);

Not wild about having "Impl" in the names here - it doesn't add anything (other than 4 characters) in my opinion :)

@@ +18,5 @@
> +nsresult
> +GetHostnameImpl(nsACString& aHostname);
> +
> +} // namespace net
> +} // namespace mozilla

So there might be some overlap with this code and some code :bagder wrote for network identification (on at least windows/mac/linux, possibly android). We should ni? him and find out if there is - and if there is, we should try to unify if possible so we're not doing the same work in two different places (thus risking having a bug in both, but only fixing it in one place)

::: netwerk/base/NetworkInfoServiceLinux.cpp
@@ +29,5 @@
> +    if (fd < 0) {
> +        return NS_ERROR_FAILURE;
> +    }
> +
> +    auto autoCloseSocket = MakeScopeExit([&] {

I was totally unaware this existed. I shall shove it in my back pocket to be used (or more likely forgotten) at a later point :)

@@ +34,5 @@
> +      close(fd);
> +    });
> +
> +    struct ifconf ifconf;
> +    char buf[16384];

That's a scary high magic number - care to comment it? (I imagine it has some basis in the reality of what the ioctl gives back)

@@ +43,5 @@
> +        return NS_ERROR_FAILURE;
> +    }
> +
> +    struct ifreq* ifreq = ifconf.ifc_req;
> +    for (int i = 0; i < ifconf.ifc_len; ) {

Maybe add a comment here that the loop condition update is at the bottom (or perhaps better - use a while loop, since update at the bottom is assumed on that type)

@@ +72,5 @@
> +    int family;
> +    switch(family=ifreq.ifr_addr.sa_family) {
> +      case AF_INET:
> +      case AF_INET6:
> +        getnameinfo(&ifreq.ifr_addr, sizeof ifreq.ifr_addr, host, sizeof host, 0, 0, NI_NUMERICHOST);

Use parens in your sizeof here, just for consistency's sake

@@ +82,5 @@
> +        return NS_OK;
> +    }
> +
> +    nsCString ifaceStr;
> +    ifaceStr.Append(aInterface);

.AssignASCII

@@ +85,5 @@
> +    nsCString ifaceStr;
> +    ifaceStr.Append(aInterface);
> +
> +    nsCString addrStr;
> +    addrStr.Append(host);

.AssignASCII

@@ +104,5 @@
> +
> +  // Ensure that there is always a terminating NUL byte.
> +  hostnameBuf[255] = '\0';
> +
> +  // Find the first '.', terminate string there.

Use strchr

@@ +112,5 @@
> +      break;
> +    }
> +  }
> +
> +  if (strlen(hostnameBuf) == 0) {

Like with the windows impl, I'm torn on instant-understandability of strlen vs. almost-instant understandability + compactness of !hostnameBuf[0]

@@ +116,5 @@
> +  if (strlen(hostnameBuf) == 0) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  aHostname = hostnameBuf;

.AssignASCII

::: netwerk/base/NetworkInfoServiceWindows.cpp
@@ +20,5 @@
> +{
> +  UniquePtr<MIB_IPADDRTABLE> ipAddrTable;
> +  DWORD size = sizeof(MIB_IPADDRTABLE);
> +
> +  ipAddrTable.reset((MIB_IPADDRTABLE*) malloc(size));

Generally I would say "don't cast the malloc return value" - and I'm going to say that here too, but only on the condition that UniquePtr::reset can handle getting a void* where it expects a T*

@@ +26,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (GetIpAddrTable(ipAddrTable.get(), &size, 0) == ERROR_INSUFFICIENT_BUFFER) {
> +    ipAddrTable.reset((MIB_IPADDRTABLE*) malloc(size));

ditto malloc cast comment

@@ +48,5 @@
> +    nsCString indexString;
> +    indexString.AppendInt(index, 10);
> +
> +    nsCString addrString;
> +    addrString.Append(ipAddrString);

.AssignASCII instead of .Append

@@ +68,5 @@
> +
> +  // Ensure that there is always a terminating NUL byte.
> +  hostnameBuf[255] = '\0';
> +
> +  // Find the first '.', terminate string there.

Use strchr instead of rolling your own :)

@@ +76,5 @@
> +      break;
> +    }
> +  }
> +
> +  if (strlen(hostnameBuf) == 0) {

I'm torn - strlen is perhaps more readable, but I like the idea of just checking |!hostnameBuf[0]| instead... up to you, I suppose

@@ +80,5 @@
> +  if (strlen(hostnameBuf) == 0) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  aHostname = hostnameBuf;

Maybe use .AssignASCII here to make it obvious that, yes, aHostname can, in fact, be changed (even though it's passed by ref, etc, etc). Also makes this more consistent with ListAddresses above.

::: netwerk/base/nsINetworkInfoService.idl
@@ +11,5 @@
> +interface nsIListNetworkAddressesListener : nsISupports
> +{
> +  void onListedNetworkAddresses([array, size_is(aAddressArraySize)] in string aAddressArray,
> +                                in unsigned long aAddressArraySize);
> +  void onListNetworkAddressesFailed();

Please comment on the arguments (what consumers can expect them to look like, for example)

@@ +31,5 @@
> +[scriptable, uuid(55fc8dae-4a58-4e0f-a49b-901cbabae809)]
> +interface nsINetworkInfoService : nsISupports
> +{
> +  /**
> +   * Get an array of strings representing all available newtork addresses

You're not really "getting" the array here - you're starting a process that will eventually call your listener back with the array.

@@ +36,5 @@
> +   */
> +  void listNetworkAddresses(in nsIListNetworkAddressesListener aListener);
> +
> +  /**
> +   * Return the hostname of the current machine.

Same here - the hostname isn't returned, it's passed callback asynchronously.

::: netwerk/base/nsNetworkInfoService.cpp
@@ +7,5 @@
> +#include "nsNetworkInfoService.h"
> +#include "mozilla/ScopeExit.h"
> +
> +#if defined(XP_MACOSX) || defined(XP_WIN) || defined(XP_LINUX)
> +#include "NetworkInfoServiceImpl.h"

I know we have moz.build guards on not building this on machines that don't match one of those things, but it might be nice to have a

    #else
    #error "ARGH!"

clause just in case something unexpected happens in the future (but please, replace my argh with a better error message!)

@@ +39,5 @@
> +    return NS_OK;
> +  }
> +
> +  uint32_t addrCount = addrMap.Count();
> +  const char** addrStrings = (const char**) malloc(sizeof(const char*) * addrCount);

Don't cast malloc return values. Also, I prefer |sizeof(*addrStrings)| to |sizeof(const char*)|, but that's minor

@@ +41,5 @@
> +
> +  uint32_t addrCount = addrMap.Count();
> +  const char** addrStrings = (const char**) malloc(sizeof(const char*) * addrCount);
> +  if (!addrStrings) {
> +    aListener->OnListNetworkAddressesFailed();

This matches the IDL for argument count (good!) but, as I noted in the android implementation, doesn't match that (bad!)

@@ +63,5 @@
> +  nsresult rv;
> +  nsCString hostnameStr;
> +  rv = GetHostnameImpl(hostnameStr);
> +  if (NS_FAILED(rv)) {
> +    aListener->OnGetHostnameFailed();

Arg count here good, android bad :)

::: netwerk/base/nsNetworkInfoService.js
@@ +20,5 @@
> +function setQueryInterface(cls, ...aQIList) {
> +  cls.prototype.QueryInterface = XPCOMUtils.generateQI(aQIList);
> +}
> +
> +class nsNetworkInfoService {

What?! Get out of here with this pretty, non-prototypcal-inheritance js! (Not really, this is awesome! Carry on!)

::: netwerk/base/nsSocketTransport2.cpp
@@ +892,5 @@
> +                                   const nsACString &hostRoute, uint16_t portRoute,
> +                                   nsIProxyInfo *proxyInfo,
> +                                   const mozilla::net::NetAddr* addr)
> +{
> +  nsresult rv = Init(socketTypes, typeCount, host, port, hostRoute, portRoute, proxyInfo);

See my comment in the .h

::: netwerk/base/nsSocketTransport2.h
@@ +136,5 @@
> +    // info).
> +    nsresult InitPreResolved(const char **socketTypes, uint32_t typeCount,
> +                             const nsACString &host, uint16_t port,
> +                             const nsACString &hostRoute, uint16_t portRoute,
> +                             nsIProxyInfo *proxyInfo,

Do we really want to be able to pass proxy info in here? I thought the whole idea here was this is for locally-discovered services, so we shouldn't need a proxy here. Same thing goes for hostRoute & portRoute, really.

I'm not saying it's definitely wrong, but something doesn't feel right. Seems more like InitPreResolved should take socketTypes, typeCount, host, port, and addr, and then the call through to Init would set dummy (empty) values for hostRoute, portRoute, and proxyInfo.

::: netwerk/base/nsSocketTransportService2.cpp
@@ +728,5 @@
> +    // Check FlyWeb table for host mappings.  If one exists, then use that.
> +    RefPtr<mozilla::dom::FlyWebService> fws =
> +        mozilla::dom::FlyWebService::GetExisting();
> +    if (fws) {
> +        nsresult rv = fws->CreateTransportForHost(types, typeCount, host, port,

I'm not crazy about having something in mozilla::dom being used to create a transport - it seems backwards (or at least poor separation of concerns). There may be no good way around it, though (I haven't looked at the dom portions of this patch yet)

@@ +730,5 @@
> +        mozilla::dom::FlyWebService::GetExisting();
> +    if (fws) {
> +        nsresult rv = fws->CreateTransportForHost(types, typeCount, host, port,
> +                                                  hostRoute, portRoute,
> +                                                  proxyInfo, result);

Hrm... looking at this, I can now see more of an argument for the way you did InitPreResolved in nsSocketTransport, but I still think this bears a bit more thinking.

::: netwerk/build/nsNetModule.cpp
@@ +61,5 @@
> +#ifdef BUILD_NETWORK_INFO_SERVICE
> +#include "nsNetworkInfoService.h"
> +typedef mozilla::net::nsNetworkInfoService nsNetworkInfoService;
> +NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsNetworkInfoService, Init)
> +#endif // BUILD_NETWORK_INFO_SERVICE

Put this at the end of the list of constructors (historically, that's how it seems to be done - newer stuff is at the end)

@@ +704,5 @@
>  }
>  
> +#ifdef BUILD_NETWORK_INFO_SERVICE
> +NS_DEFINE_NAMED_CID(NETWORKINFOSERVICE_CID);
> +#endif // BUILD_NETWORK_INFO_SERVICE

Please put the new CID at the end of the giant list of named CIDs

@@ +855,5 @@
>  
>  static const mozilla::Module::CIDEntry kNeckoCIDs[] = {
> +#ifdef BUILD_NETWORK_INFO_SERVICE
> +    { &kNETWORKINFOSERVICE_CID, false, nullptr, nsNetworkInfoServiceConstructor },
> +#endif

Again, please put at the end of the list

@@ +1012,5 @@
>  
>  static const mozilla::Module::ContractIDEntry kNeckoContracts[] = {
> +#ifdef BUILD_NETWORK_INFO_SERVICE
> +    { NETWORKINFOSERVICE_CONTRACT_ID, &kNETWORKINFOSERVICE_CID },
> +#endif

End of the list :)

::: netwerk/dns/mdns/libmdns/fallback/DNSPacket.jsm
@@ +263,5 @@
> + */
> +function _flagsToValue(flags) {
> +  let value = 0x0000;
> +
> +  value = value << 1;

no-op, not needed?

::: netwerk/dns/mdns/libmdns/fallback/DataReader.jsm
@@ +15,5 @@
> +    this._cursor = startByte;
> +  }
> +
> +  get buffer() {
> +    return this.data.buffer;

Any reason to not just do |this._data.buffer| here? (since |get data()| just returns |this._data|)

::: netwerk/dns/mdns/libmdns/fallback/DataWriter.jsm
@@ +72,5 @@
> +    this.putValue(0);
> +  }
> +
> +  putLengthString(string) {
> +    this.putValue(string.length);

I doubt it will happen much (if at all) in practice, but you should check to make sure that this value fits in 1 byte (which is what putValue will assume, since it's being called with no length). Sounds like a recipe for disaster if it doesn't.

::: netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm
@@ +18,5 @@
> +Cu.import('resource://gre/modules/DNSRecord.jsm');
> +Cu.import('resource://gre/modules/DNSResourceRecord.jsm');
> +Cu.import('resource://gre/modules/DNSTypes.jsm');
> +
> +const DEBUG = true;

Set to false before committing

@@ +214,5 @@
> +    this._checkStartBroadcastTimer();
> +
> +    // Check to see if sockets should be closed, and if so close them.
> +    this._checkCloseSockets();
> + 

nit: trailing whitespace

@@ +371,5 @@
> +  }
> +
> +  _handleQueryPacket(packet, message) {
> +    packet.getRecords(['QD']).forEach((record) => {
> +      let isFlyWeb = record.name.indexOf("flyweb") != -1;

isFlyWeb does not appear to be used anywhere here

@@ +532,5 @@
> +    }
> +
> +    this._discovered.set(key, {
> +      serviceInfo: serviceInfo,
> +      expireTime: expireTime 

nit: trailing whitespace

@@ +686,5 @@
> +        resolve(_addresses);
> +      }
> +    });
> +  });
> +}

I don't see any handling for |listNetworkAddresses| failing

::: dom/webidl/moz.build
@@ +159,5 @@
>      'FileList.webidl',
>      'FileMode.webidl',
>      'FileReader.webidl',
>      'FileReaderSync.webidl',
> +    #'FlyWebConnection.webidl',

Why is this commented out?

@@ +786,5 @@
>      'DeviceStorageChangeEvent.webidl',
>      'DOMTransactionEvent.webidl',
>      'DownloadEvent.webidl',
>      'ErrorEvent.webidl',
> +    #'FlyWebClientConnectEvent.webidl',

And this?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FlyWebConnection.cpp and FlyWebConnection.h is dead code. We're not even building it these days. Sorry, i didn't notice this earlier.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Commenting on the parts of the patch that I'm familiar with, and where I had something to say. If I didn't comment assume that I agree or didn't know the code.

> ::: dom/flyweb/FlyWebService.cpp
> @@ +688,5 @@
> > +    FlyWebService::GetOrCreate()->FindPublishedServerByName(aServer->GetName());
> > +  MOZ_ASSERT(existingServer);
> > +
> > +  // FIXME: Ensure that there is not already a service being
> > +  // advertised under the same name.
> 
> This FIXME and the MOZ_ASSERT above it seem to be doing/implying opposite
> things...

This reminds me. The code currently relies on that two websites never publish a server with the same name. That's not something that we can rely on I think.

One possible solution is that we append "(<origin>)" to the end of the name. That's a good idea to do for security reasons anyway, and would solve the problem of collisions. Probably we can skip the "https://" part of the origin if the publishing page comes from https (should be essentially always).

> @@ +836,5 @@
> > +
> > +  httpServer->Init(-1, server);
> > +  mServers.AppendElement(server);
> > +  auto autoRemovePublishedServer = MakeScopeExit([&] {
> > +    if (httpServer) { httpServer->Close(); }
> 
> Again, maybe a stylistic difference between dom and necko, but prefer not
> having if body on the same line, even for one-liners

Yeah, the body should be on a separate line according to gecko style rules i think.

> @@ +868,5 @@
> > +  if (aRv.Failed()) {
> > +    return nullptr;
> > +  }
> > +
> > +  return promise.forget();
> 
> This does not appear to actually connect to a server (nor does the promise
> created actually seem to have anything directly to do with connecting to a
> server). Should this be named differently? Is this creating a promise that
> will be resolved when the connection completes?

This function is dead code. We should remove it.

> ::: dom/flyweb/HttpServer.cpp
> @@ +1,1 @@
> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> So generally speaking in this file - are there particular threads that the
> methods are expected to be called on? Main thread? Socket thread? Please
> MOZ_ASSERT for any thread you can.

This should all run on the main thread, except for some functions on the StreamCopier class.

Though I suspect that a lot of this code will run just fine on background threads as well. But I think auditing for that could be done later.

> Also, I've noticed a few HTTP cases that aren't handled - Transfer-Encoding:
> chunked comes to mind. I know we can probably rely on Firefox to not do
> anything crazy, but someone trying to be malicious won't be acting how we
> expect :) Have we had anyone fuzz this? We should.

We should definitely have someone fuzz this.

I didn't implement chunked encoding for incoming request. Contemporary browsers don't seem to ever use chunked encoding in requests. The webservers that I looked at (mainly the python SimpleHTTPServer and our own httpd.js) does not seem to support it.

I would not be surprised if chunked-encoding-requests will get added to browsers in a not too distant future given that we're getting closer to having stream primitivies exposed to web pages. However I think we can wait until we get there before adding support to HttpServer. I definitely think that websites will need to opt in specifically to allowing chunked requests, since I bet many servers don't handle it properly.

But maybe we should more explicitly error out if we get requests with Transfer-Encoding set.

> @@ +49,5 @@
> > +  nsresult rv;
> > +  nsCOMPtr<nsIPrefBranch> prefService;
> > +  prefService = do_GetService(NS_PREFSERVICE_CONTRACTID);
> > +
> > +  if (Preferences::GetBool("flyweb.use-tls", false)) {
> 
> I would rather use-tls default to true in code, and only be made false if
> the pref is explicitly set. Also, do we have this in all.js somewhere?

We will as soon as it's properly implemented. There's still some bugs preventing it from working. Currently looks like those bugs are in TLSServerSocket, and not in HttpServer, so possibly the current code will remain as-is.

> @@ +326,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +NS_METHOD
> > +HttpServer::Connection::ReadSegmentsFunc(nsIInputStream* aIn,
> 
> This might be better called WriteSegmentsFunc, since its type is
> nsWriteSegmentFun (SO MUCH FUN!), and it is, in fact, performing the part of
> WriteSegments opposite your call to ReadSegments.
> 
> BTW, if you loathe the naming of {Read,Write}Segments, you're not alone.
> But, they're old and cranky, so what're you gonna do?

Ugh! Can we use any other name than WriteSegmentsFunc? That is literally the most confusing name I can think of. Maybe "WriteToStreamFunc" or "ReadSegmentsHelper"?

> @@ +365,5 @@
> > +
> > +nsresult
> > +HttpServer::Connection::ConsumeInput(const char*& aBuffer,
> > +                                     const char* aEnd)
> > +{
> 
> So I worry a little bit about this - necko already has a lot of HTTP parsing
> code (though not necessarily for requests), and HTTP/1.1 is... finicky, to
> say the least :) Is there any way we can look into unifying or using what
> necko already has available? Doing so would likely avoid potential issues in
> the future.

If there's something that we can reuse that's great. I ended up staying away from a lot of the existing http code code because it so full of XPCOM. But I agree code sharing is safer, so probably work looking harder.

> @@ +397,5 @@
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +      mInputBuffer.Truncate();
> > +    }
> > +    else {
> 
> again, |} else {| ?
> 
> @@ +416,5 @@
> > +      // Since we've given the pipe unlimited size, we should never
> > +      // end up needing to block.
> > +      MOZ_ASSERT(rv != NS_BASE_STREAM_WOULD_BLOCK);
> > +      if (NS_FAILED(rv)) {
> > +        written = size;
> 
> This feels suspect to me - if the write failed, why would we just pretend we
> wrote |size| bytes?

This just causes us to skip the rest of request body so that we can start parsing the next http request on the socket.

> @@ +457,5 @@
> > +  if (!str.EqualsLiteral("GET")) {
> > +    return false;
> > +  }
> > +
> > +  InternalHeaders* headers = aRequest->Headers();
> 
> So I see (I think) that this type comes from the Fetch code - is there any
> particular reason we didn't use nsHttp{Request,Response}Head from necko?

The necko classes are super painful to use because they are so full of XPCOM :(

And eventually we need to create InternalRequest objects anyway so that we can expose a Request object to the webpage. 

But I agree that it sucks that we have two separate classes for this. My preferred solution would be to deCOMtaminate the necko classes and then make both the fetch code and this code use it. But that seems better as a followup.

> @@ +609,5 @@
> > +    if (mRemainingBodySize) {
> > +      LOG_V("HttpServer::Connection::ConsumeLine(%p) - Starting consume body", this);
> > +      mState = eBody;
> > +
> > +      // We use an unlimited buffer size here to ensure
> 
> Unlimited buffer worries me - sounds like a potential for a DoS. I'm not
> saying "no" to it (it may be reasonable in the 99 percentile case), but I
> want to make sure we think really hard about it before moving forward.

I agree with this concern, but it was the only reasonable thing I could think of.

We have to consume the whole request body. Simply not reading the request body after X MB will cause subsequent requests to get blocked until the server has processed the current request. Which might lead to staying blocked forever depending on the server logic.

The other alternative is to stream to disk once we reach X MB of in-memory data. But I think that's better done separately since it'll be a lot of work.

Btw, our infrastructure around necko streams is really sad. There was basically nothing to make copying between streams easier, much less copy while doing chunked encoding. The only thing that I could find is nsIAsyncStreamCopier(2), and that is only able to copy whole streams, which is not good enough for our use case.

That's not to mention the fact that nsIChannel and nsIAsyncInputStream both represent an asynch readable stream of data. And that nsIInputStream can both represent a sync readable stream or an async readable stream. But that mess is likely too hard to clean up at this point. At least as long as we need to worry about addons.

> @@ +706,5 @@
> > +    if (pending.first() == aRequest) {
> > +      MOZ_ASSERT(!handledResponse);
> > +      MOZ_ASSERT(!pending.second());
> > +
> > +      pending.second() = aResponse;
> 
> Is there another way to do this? It always looks weird, assigning a value to
> the result of a function call... (maybe I'm just too set in my ways, though)

This is how mfbt Tuple works. The alternative would be to use a struct I guess.


> ::: dom/flyweb/HttpServer.h
> @@ +14,5 @@
> > +#include "nsIAsyncOutputStream.h"
> > +#include "mozilla/Variant.h"
> > +#include "nsIRequestObserver.h"
> > +#include "mozilla/MozPromise.h"
> > +#include "nsITransportProvider.h"
> 
> I don't see this file anywhere, and it doesn't appear to be in current m-c.
> Missing from this patch?

See bug 1272100 which this one should depend on.

> ::: netwerk/base/nsSocketTransportService2.cpp
> @@ +728,5 @@
> > +    // Check FlyWeb table for host mappings.  If one exists, then use that.
> > +    RefPtr<mozilla::dom::FlyWebService> fws =
> > +        mozilla::dom::FlyWebService::GetExisting();
> > +    if (fws) {
> > +        nsresult rv = fws->CreateTransportForHost(types, typeCount, host, port,
> 
> I'm not crazy about having something in mozilla::dom being used to create a
> transport - it seems backwards (or at least poor separation of concerns).
> There may be no good way around it, though (I haven't looked at the dom
> portions of this patch yet)

We could have FlyWebService just return a resolved IP+port, and then let nsSocketTransportService2.cpp create the actual transport. I don't have an opinion either way.

Comment 7

[u408661]

(In reply to Jonas Sicking (:sicking) from comment #6)
> This reminds me. The code currently relies on that two websites never
> publish a server with the same name. That's not something that we can rely
> on I think.
> 
> One possible solution is that we append "(<origin>)" to the end of the name.
> That's a good idea to do for security reasons anyway, and would solve the
> problem of collisions. Probably we can skip the "https://" part of the
> origin if the publishing page comes from https (should be essentially
> always).

That sounds like a good plan, to me.

> > ::: dom/flyweb/HttpServer.cpp
> > @@ +1,1 @@
> > > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > 
> > So generally speaking in this file - are there particular threads that the
> > methods are expected to be called on? Main thread? Socket thread? Please
> > MOZ_ASSERT for any thread you can.
> 
> This should all run on the main thread, except for some functions on the
> StreamCopier class.
> 
> Though I suspect that a lot of this code will run just fine on background
> threads as well. But I think auditing for that could be done later.

All sounds good to me - let's put some ASSERTs in, and we can loosen that restriction later once we've audited to make sure it's reasonable.

> > Also, I've noticed a few HTTP cases that aren't handled - Transfer-Encoding:
> > chunked comes to mind. I know we can probably rely on Firefox to not do
> > anything crazy, but someone trying to be malicious won't be acting how we
> > expect :) Have we had anyone fuzz this? We should.
> 
> We should definitely have someone fuzz this.
> 
> I didn't implement chunked encoding for incoming request. Contemporary
> browsers don't seem to ever use chunked encoding in requests. The webservers
> that I looked at (mainly the python SimpleHTTPServer and our own httpd.js)
> does not seem to support it.

Well, neither of those are the greatest examples ;) But I agree that we don't necessarily need to support chunked transfer-encoding off the bat. We should handle it in some fashion, though. I'm fine with sending a 500 error (since we're supposed to support it, but don't yet)

> I would not be surprised if chunked-encoding-requests will get added to
> browsers in a not too distant future given that we're getting closer to
> having stream primitivies exposed to web pages. However I think we can wait
> until we get there before adding support to HttpServer. I definitely think
> that websites will need to opt in specifically to allowing chunked requests,
> since I bet many servers don't handle it properly.
> 
> But maybe we should more explicitly error out if we get requests with
> Transfer-Encoding set.
> 
> > @@ +49,5 @@
> > > +  nsresult rv;
> > > +  nsCOMPtr<nsIPrefBranch> prefService;
> > > +  prefService = do_GetService(NS_PREFSERVICE_CONTRACTID);
> > > +
> > > +  if (Preferences::GetBool("flyweb.use-tls", false)) {
> > 
> > I would rather use-tls default to true in code, and only be made false if
> > the pref is explicitly set. Also, do we have this in all.js somewhere?
> 
> We will as soon as it's properly implemented. There's still some bugs
> preventing it from working. Currently looks like those bugs are in
> TLSServerSocket, and not in HttpServer, so possibly the current code will
> remain as-is.

Excellent!

> > @@ +326,5 @@
> > > +  return NS_OK;
> > > +}
> > > +
> > > +NS_METHOD
> > > +HttpServer::Connection::ReadSegmentsFunc(nsIInputStream* aIn,
> > 
> > This might be better called WriteSegmentsFunc, since its type is
> > nsWriteSegmentFun (SO MUCH FUN!), and it is, in fact, performing the part of
> > WriteSegments opposite your call to ReadSegments.
> > 
> > BTW, if you loathe the naming of {Read,Write}Segments, you're not alone.
> > But, they're old and cranky, so what're you gonna do?
> 
> Ugh! Can we use any other name than WriteSegmentsFunc? That is literally the
> most confusing name I can think of. Maybe "WriteToStreamFunc" or
> "ReadSegmentsHelper"?

I'm not tied to WriteSegmentsFUnc - WriteToStreamFunc sounds totally reasonable to me. I just don't like having a function called Read<whatever> taking/modifying arguments called countWritten, etc :)

> > @@ +365,5 @@
> > > +
> > > +nsresult
> > > +HttpServer::Connection::ConsumeInput(const char*& aBuffer,
> > > +                                     const char* aEnd)
> > > +{
> > 
> > So I worry a little bit about this - necko already has a lot of HTTP parsing
> > code (though not necessarily for requests), and HTTP/1.1 is... finicky, to
> > say the least :) Is there any way we can look into unifying or using what
> > necko already has available? Doing so would likely avoid potential issues in
> > the future.
> 
> If there's something that we can reuse that's great. I ended up staying away
> from a lot of the existing http code code because it so full of XPCOM. But I
> agree code sharing is safer, so probably work looking harder.

We can probably do all of that as a follow-up (since I suspect it would be a lot of work to unify the two sets).

> > @@ +416,5 @@
> > > +      // Since we've given the pipe unlimited size, we should never
> > > +      // end up needing to block.
> > > +      MOZ_ASSERT(rv != NS_BASE_STREAM_WOULD_BLOCK);
> > > +      if (NS_FAILED(rv)) {
> > > +        written = size;
> > 
> > This feels suspect to me - if the write failed, why would we just pretend we
> > wrote |size| bytes?
> 
> This just causes us to skip the rest of request body so that we can start
> parsing the next http request on the socket.

Ah, that makes sense. Let's add a comment to that effect so it's more obvious.

> > @@ +457,5 @@
> > > +  if (!str.EqualsLiteral("GET")) {
> > > +    return false;
> > > +  }
> > > +
> > > +  InternalHeaders* headers = aRequest->Headers();
> > 
> > So I see (I think) that this type comes from the Fetch code - is there any
> > particular reason we didn't use nsHttp{Request,Response}Head from necko?
> 
> The necko classes are super painful to use because they are so full of XPCOM
> :(
> 
> And eventually we need to create InternalRequest objects anyway so that we
> can expose a Request object to the webpage. 
> 
> But I agree that it sucks that we have two separate classes for this. My
> preferred solution would be to deCOMtaminate the necko classes and then make
> both the fetch code and this code use it. But that seems better as a
> followup.

Indeed, agreed.

> > @@ +609,5 @@
> > > +    if (mRemainingBodySize) {
> > > +      LOG_V("HttpServer::Connection::ConsumeLine(%p) - Starting consume body", this);
> > > +      mState = eBody;
> > > +
> > > +      // We use an unlimited buffer size here to ensure
> > 
> > Unlimited buffer worries me - sounds like a potential for a DoS. I'm not
> > saying "no" to it (it may be reasonable in the 99 percentile case), but I
> > want to make sure we think really hard about it before moving forward.
> 
> I agree with this concern, but it was the only reasonable thing I could
> think of.
> 
> We have to consume the whole request body. Simply not reading the request
> body after X MB will cause subsequent requests to get blocked until the
> server has processed the current request. Which might lead to staying
> blocked forever depending on the server logic.
> 
> The other alternative is to stream to disk once we reach X MB of in-memory
> data. But I think that's better done separately since it'll be a lot of work.

That sounds good. Is it also thinking about placing an arbitrary limit on it for now? Like, do we expect most/all requests coming to this server to be below X {K,M}B, so we can just set a limit for now? If not, then let's just leave it as is, but file the followup so it doesn't get forgotten.

> > @@ +706,5 @@
> > > +    if (pending.first() == aRequest) {
> > > +      MOZ_ASSERT(!handledResponse);
> > > +      MOZ_ASSERT(!pending.second());
> > > +
> > > +      pending.second() = aResponse;
> > 
> > Is there another way to do this? It always looks weird, assigning a value to
> > the result of a function call... (maybe I'm just too set in my ways, though)
> 
> This is how mfbt Tuple works. The alternative would be to use a struct I
> guess.

Eh, it's fine. I just forgot I was looking at a Tuple when I wrote that comment.

> > ::: dom/flyweb/HttpServer.h
> > @@ +14,5 @@
> > > +#include "nsIAsyncOutputStream.h"
> > > +#include "mozilla/Variant.h"
> > > +#include "nsIRequestObserver.h"
> > > +#include "mozilla/MozPromise.h"
> > > +#include "nsITransportProvider.h"
> > 
> > I don't see this file anywhere, and it doesn't appear to be in current m-c.
> > Missing from this patch?
> 
> See bug 1272100 which this one should depend on.

Excellent, good to know.

> > ::: netwerk/base/nsSocketTransportService2.cpp
> > @@ +728,5 @@
> > > +    // Check FlyWeb table for host mappings.  If one exists, then use that.
> > > +    RefPtr<mozilla::dom::FlyWebService> fws =
> > > +        mozilla::dom::FlyWebService::GetExisting();
> > > +    if (fws) {
> > > +        nsresult rv = fws->CreateTransportForHost(types, typeCount, host, port,
> > 
> > I'm not crazy about having something in mozilla::dom being used to create a
> > transport - it seems backwards (or at least poor separation of concerns).
> > There may be no good way around it, though (I haven't looked at the dom
> > portions of this patch yet)
> 
> We could have FlyWebService just return a resolved IP+port, and then let
> nsSocketTransportService2.cpp create the actual transport. I don't have an
> opinion either way.

If that's not a ton of extra work, then let's do that. If it's going to add more than like a day of work, though, it's probably not worth doing.

Comment 8

[Kannan Vijayan [:djvj]]

> So there might be some overlap with this code and some code :bagder wrote for network identification (on at least windows/mac/linux, possibly android). We should ni? him and find out if there is - and if there is, we should try to unify if possible so we're not doing the same work in two different places (thus risking having a bug in both, but only fixing it in one place)

Daniel: Nick is commenting here on our code to obtain a list of network IP addresses, and a hostname, for the current host.  Do we have existing code that will do this?  Please see 'NetworkInfoService' (nsINetworkInfoService and related implementation) in the patch attached to this bug.

Comment 9

[Kannan Vijayan [:djvj]]

Attachment: flyweb-network-updates.patch

Delta patch of updates to networking code.

Comment 10

[Kannan Vijayan [:djvj]]

Comment responses.  See delta patch that's been posted.  I'll build a full patchset and post that as well as soon as I finish build-testing all the changes so far.

> ::: dom/flyweb/FlyWebConnection.cpp

All of FlyWebConnection.cpp has been removed.  We'll put it in later when it's actually going to be hooked up functionality.

> ::: dom/flyweb/FlyWebService.cpp
> @@ +86,5 @@
> > +
> > +  bool mDiscoveryActive;
> > +  uint32_t mNumConsecutiveStartDiscoveryFailures;
> > +
> > +  DiscoveryState mDiscoveryState;
>
> Is there a reason we don't just have mDiscoveryActive as another state within mDiscoveryState? Seems like there's an opportunity for things to become out-of-sync

mDiscoveryState is separate from mDiscoveryActive because the state reflects the actual state of the current system, and the active flag reflects the desired state requested by calls.  They are loosely coupled and need to be so.

For example, when discovery is active, we repeatedly start and stop discovery on a timed basis.  So: mDiscoveryActive gets set to true, and initiates a loop whereby the mDiscoveryState goes from (IDLE => STARTING => RUNNING => STOPPING => IDLE) repeatedly.  When mDiscoveryActive becomes false, the state transitions (which may be in the middle of STARTING, RUNNING, or STOPPING) are directed towards the IDLE Final state.

So let's say we make discovery active and we're currently IDLE.  We'll get ready to start discovery and move into the STARTING state.  Let's say we get a stop-discovery request before we have received the event that puts us in a RUNNING state.  We will set mDiscoveryActive to false, but do nothing else, because we can't take any immediate action when in the STARTING state.  Once we receive the event that puts us into the RUNNING state, we re-check the mDiscoveryActive field, and if false, kick off the stop-discovery process.

Splitting this up into two variables lets us cleanly separate the desired state of the system (whether discovery should be happening or not), from the mechanics of interacting with the discovery backend (what sutation the raw discovery state machine is in).

> @@ +194,5 @@
> > +
> > +  // Clear the new service array.
> > +  mNewServiceSet.Clear();
> > +
> > +  // If mDiscoveryActive is false, then stop discovery immediately.
>
> Better comment here - the "mDiscoveryActive is false" bit is obvious from
> the code, but what does it *mean*?
>
> Again, what does mDiscoveryActive is false mean? I assume it means we're
> not actively running a discovery process, but could be clearer from the
> comment

I documented both mDiscoveryActive and mDiscoveryState, and changed this comment to be clearer.

> @@ +235,5 @@
> > +
> > +  // Notify FlyWebService of changed service list.
> > +  mService->NotifyDiscoveredServicesChanged();
> > +
> > +  // Start discovery again immediately.
>
> Does this risk some kind of stop/start/stop/start/... loop? What are the reasons OnDiscoveryStopped could fire, and why is it safe to restart immediately?

That is the intent.  When discovery is active, we start the network discovery, listen for a while, stop it, and start it again.. and keep doing that until we get a stop-discovery request from the frontend.  Discovery is only active when there are active listeners for discovery, which only happens when the user has opened the FlyWeb Discovered Services List UI.

Basically when discovery is active we poll the network on a slow loop.

There's a risk of repeatedly starting and failing to start, and busy-looping that way.. so we avoid that by keeping track of consecutive failures in mNumConsecutiveStartDiscoveryFailures.  If there are 3 failures in a row, we don't redo network discovery even if service discovery is active.

> @@ +292,5 @@
> > +  MOZ_ASSERT(mDiscoveryState == DISCOVERY_STOPPING);
> > +  mDiscoveryState = DISCOVERY_IDLE;
> > +
> > +  // If discovery is active, start discovery again immediately.
> > +  if (mDiscoveryActive) {
>
> Ah, I think this (and OnStartDiscoveryFailed) have answered my question
> about separate mDiscoveryActive/mDiscoveryState. Does it maybe make sense
> to rename mDiscoveryActive to something like mDiscoveryShouldBeActive? That
> seems to fit its purpose better (at least here). Up to you.

I can see the confusion here.  The way I see it, there are two notions of
discovery: "service discovery" from the perpsective of the caller code that uses
the FlyWeb chrome API, and "network discovery" which is what the MDNS backend of
FlYWeb uses to implement "service discovery".

So the mDiscoveryActive tracks the "service discovery" state, which is simply
either "on" or "off".

The mDiscoveryState tracks the "network discovery" state, which is a slightly more
complicated state machine.

What do you think about changing this to mServiceDiscoveryActive and mNetworkDiscoveryState,
with appropriate documentation on the variables?

> @@ +630,5 @@
> > +  return mServiceMap.Contains(aServiceId);
> > +}
> > +
> > +nsresult
> > +FlyWebMDNSService::ConnectToService(const nsAString& aServiceId,
>
> I don't see any actual connection going on here, only creation of a uri,
> and setting up some info about a connection (which may or may not exist
> at this point, I'm not sure)

Right, so this is also a terminology issue.  From a FlyWeb perspective, a user "discovers and connects to" a service.  This is a logical connection between the user and the service, not a network connection.  For traditional IP networks, this notion of "connection" is a no-op.  You don't have to do anything special to gain the ability to open a socket to request a resource from a host: you just need to know its IP and port.

For a bluetooth-based service, this "connection" step might be more meaningfull:  The user might be asked to enter a PIN or something to establish a connection to the service.  Basically, with IP networks we're used to the idea that if you can address something, you can open a socket to it.  In the FlyWeb context, you might be able to address a service (via discovery), but you may need to take some extra steps (establish a session with it in some transport-specific way), before you're able to actually make connections to it.

The caller of this method (FlyWebService::ConnectToService) takes the discovered service info and adds it to a "connected service table", associating it with the generated UUID as the hostname it's available under.  Later on when we try to make network requests to URLS involving this generated hostname, we check that "connected service table" for the hostname, and if it exists, route the request to the appropriate service.

Maybe it would be clearer to refer to this as a "Session"?  I don't know.  I'm open to suggestions on how to name these things more clearly.

> @@ +688,5 @@
> > +    FlyWebService::GetOrCreate()->FindPublishedServerByName(aServer->GetName());
> > +  MOZ_ASSERT(existingServer);
> > +
> > +  // FIXME: Ensure that there is not already a service being
> > +  // advertised under the same name.
>
> This FIXME and the MOZ_ASSERT above it seem to be doing/implying opposite things...

The FIXME is stale, and I have removed it.  This was an implementation detail: whether the caller of FlyWebMDNSService::StartDiscoveryOf() registered the name of the new service before calling it, or it whether it was this method's responsibility to register that name.  The FIXME assumed the latter implementation approach.  Turned out that didn't work, and I changed it so that the caller already added the service to the table before calling in, but forgot to remove the FIXME.

> @@ +733,5 @@
> > +
> > +void
> > +FlyWebMDNSService::EnsureDiscoveryStopped()
> > +{
> > +  mDiscoveryActive = false;
>
> This does not appear to actually ensure discovery is stopped - nowhere is StopDiscovery called :)

This interacts with the state machine modeled by mDiscoveryState.  So, if mDiscoveryState is IDLE, we're all good because discovery is not running anyway.  If mDiscoveryState is not IDLE (e.g. it's STARTING, RUNNING, or STOPPING), then the callback handlers for each of those state transitions will check mDiscoveryActive and if it's false, push the network discovery state towards IDLE.

> @@ +836,5 @@
> > +
> > +  httpServer->Init(-1, server);
> > +  mServers.AppendElement(server);
> > +  auto autoRemovePublishedServer = MakeScopeExit([&] {
> > +    if (httpServer) { httpServer->Close(); }
>
> Again, maybe a stylistic difference between dom and necko, but prefer not
> having if body on the same line, even for one-liners

This code has been all cleaned up by Jonas recently.  Leaving till next review round.

> @@ +868,5 @@
> > +  if (aRv.Failed()) {
> > +    return nullptr;
> > +  }
> > +
> > +  return promise.forget();
>
> This does not appear to actually connect to a server (nor does the promise
> created actually seem to have anything directly to do with connecting to
> a server). Should this be named differently? Is this creating a promise that
> will be resolved when the connection completes?

This goes back to the semantic difference between a FlyWeb frontend notion of "connecting to a server" and the our technical understanding of connecting to a server (which is pretty much "open a socket to it").  From the perspective of a user, if you've 'connected' to a server, it means that you have performed the necessary preliminary steps so that you're able to make web requests from it.

For the MDNS backend, this connection process is very trivial.  It mostly involves the generation of a unique hostname for the server (to prevent servers from naming themselves and leading to origin-based issues), and marking the discovered service as "connected" so that we can route subsequent requests for that unique hostname to the correct local-network host (bypassing DNS).

> ::: dom/flyweb/HttpServer.cpp
> @@ +1,1 @@
> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>
> So generally speaking in this file - are there particular threads that the
> methods are expected to be called on? Main thread? Socket thread? Please
> MOZ_ASSERT for any thread you can.

CreateTransportForHost should be called from the main thread, but other than that everything else should be Main thread.

Quick question: what thread is nSocketTransportService2::CreateTransport called in?

> Also, I've noticed a few HTTP cases that aren't handled - Transfer-Encoding:
> chunked comes to mind. I know we can probably rely on Firefox to not do
> anything crazy, but someone trying to be malicious won't be acting how we
> expect :) Have we had anyone fuzz this? We should.

No, no fuzzing yet :)  Do you mean chunked transfer on requests?  Yeah, we eventually need to cover fuzzing and more thorough handling of HTTP.  Stephanie Ouillon is looking into the initial security review of this stuff.

> @@ +49,5 @@
> > +  nsresult rv;
> > +  nsCOMPtr<nsIPrefBranch> prefService;
> > +  prefService = do_GetService(NS_PREFSERVICE_CONTRACTID);
> > +
> > +  if (Preferences::GetBool("flyweb.use-tls", false)) {
>
> I would rather use-tls default to true in code, and only be made false
> if the pref is explicitly set. Also, do we have this in all.js somewhere?

The https code is not ready for prime-time yet.  I think it should be almost-ready,
and we'll enable it as soon as we get it working reliably.


> @@ +54,5 @@
> > +    nsCOMPtr<nsILocalCertService> lcs =
> > +      do_CreateInstance("@mozilla.org/security/local-cert-service;1");
> > +    rv = lcs->GetOrCreateCert(NS_LITERAL_CSTRING("flyweb"), this);
> > +  }
> > +  else {
>
> dom may have different conventions from necko, but shouldn't this else
> be on the same line as the closing brace?

I have not seen that convention in DOM either.  Fixed all instances of it in the file.

> @@ +326,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +NS_METHOD
> > +HttpServer::Connection::ReadSegmentsFunc(nsIInputStream* aIn,
>
> This might be better called WriteSegmentsFunc, since its type is
> nsWriteSegmentFun (SO MUCH FUN!), and it is, in fact, performing the part
> of WriteSegments opposite your call to ReadSegments.
>
> BTW, if you loathe the naming of {Read,Write}Segments, you're not alone.
> But, they're old and cranky, so what're you gonna do?

Jonas wrote this code, which I'm glad about ;)  I'll punt this decision to him.  From the caller perspective (mInput->ReadSegments(ReadSegmentsFunc, ...)), it seems like a reasonable name.

I'm not familiar enough with the API and usage here to really construct a useful opinion/argument on it.

Jonas: Can you comment?


> @@ +365,5 @@
> > +
> > +nsresult
> > +HttpServer::Connection::ConsumeInput(const char*& aBuffer,
> > +                                     const char* aEnd)
> > +{
>
> So I worry a little bit about this - necko already has a lot of HTTP parsing
> code (though not necessarily for requests), and HTTP/1.1 is... finicky, to
> say the least :) Is there any way we can look into unifying or using what
> necko already has available? Doing so would likely avoid potential issues in
> the future.

This is a reasonable suggestion, but can we punt that for a post-London cleanup?
I don't know if we have time to rework parsing entirely before June, given the other
work that's ongoing.


> @@ +416,5 @@
> > +      // Since we've given the pipe unlimited size, we should never
> > +      // end up needing to block.
> > +      MOZ_ASSERT(rv != NS_BASE_STREAM_WOULD_BLOCK);
> > +      if (NS_FAILED(rv)) {
> > +        written = size;
>
> This feels suspect to me - if the write failed, why would we just pretend we wrote |size| bytes?

Fixed, we return the error |rv| now.

> @@ +457,5 @@
> > +  if (!str.EqualsLiteral("GET")) {
> > +    return false;
> > +  }
> > +
> > +  InternalHeaders* headers = aRequest->Headers();
>
> So I see (I think) that this type comes from the Fetch code - is there
> any particular reason we didn't use nsHttp{Request,Response}Head from
> necko? They might need some extending to fit this use case (for example,
> parsing request headers instead of having them supplied by necko), but it
> might be something to think about for a follow-up, so we're not duplicating
> effort.

Noted.  Including "Jonas" in this comment so I can CTRL-F things in the response I should talk to him about.

> @@ +609,5 @@
> > +    if (mRemainingBodySize) {
> > +      LOG_V("HttpServer::Connection::ConsumeLine(%p) - Starting consume body", this);
> > +      mState = eBody;
> > +
> > +      // We use an unlimited buffer size here to ensure
>
> Unlimited buffer worries me - sounds like a potential for a DoS. I'm not
> saying "no" to it (it may be reasonable in the 99 percentile case), but I
> want to make sure we think really hard about it before moving forward.

I agree.  If the webpage hangs onto the request indefinitely without consuming
the body, then it's just a bad server, and its clients will experience timeouts,
which is fine.

Moreover, we don't really expect huge request sizes to show up.  I don't see
why this has to be over a megabyte or so in size.

Jonas: can you comment?

> @@ +706,5 @@
> > +    if (pending.first() == aRequest) {
> > +      MOZ_ASSERT(!handledResponse);
> > +      MOZ_ASSERT(!pending.second());
> > +
> > +      pending.second() = aResponse;
>
> Is there another way to do this? It always looks weird, assigning a value
> to the result of a function call... (maybe I'm just too set in my ways,
> though)

Took a look at mozilla::Pair, there's no setFirst() or setSecond() methods.. the design of the API seems to be intended to use the lvalue reference returns for assignment.

> ::: dom/flyweb/HttpServer.h
> @@ +14,5 @@
> > +#include "nsIAsyncOutputStream.h"
> > +#include "mozilla/Variant.h"
> > +#include "nsIRequestObserver.h"
> > +#include "mozilla/MozPromise.h"
> > +#include "nsITransportProvider.h"
>
> I don't see this file anywhere, and it doesn't appear to be in current m-c. Missing from this patch?

The related changes for this are in the WebSocket changeset posted in bug 1272100.

> ::: dom/webidl/FlyWebConnection.webidl
> @@ +11,5 @@
> > +
> > +interface FlyWebConnection : EventTarget {
> > +  //readonly attribute DOMString serviceId;
> > +  //readonly attribute DOMString sessionId;
> > +  //readonly attribute DOMString? origin; // Website running other peer
>
> Would be good to know why some of the attributes here are commented out (not just these 3, some below, as well)

All of FlyWebConnection has been removed until we build that feature.

> @@ +112,5 @@
> > +        String model = Build.MODEL;
> > +        if (model.startsWith(manufacturer)) {
> > +            return capitalize(model);
> > +        }
> > +        return capitalize(manufacturer) + " " + model;
>
> This assumes (perhaps rightly) that |manufacturer| will never be an empty
> string - might be better to call |capitalize| on the final concatenated
> string? (That would also require not adding a space if |manufacturer| is
> empty)

Build.MANUFACTURER should never be empty, so this should be OK.

> @@ +124,5 @@
> > +        char ch0 = str.charAt(0);
> > +        if (Character.isLetter(ch0) && Character.isLowerCase(ch0)) {
> > +            boolean upcase = true;
> > +            // But don't capitalize the first letter if it's an 'i' followed
> > +            // by a non-lowercase letter.  Sheesh.
>
> "Sheesh" indeed. Also, please expand on why "i+LETTER" is a special case.

Some device names are of the form 'iFoo', which we don't want to transform into 'IFoo' since the lowercase is part of the proper name.  I looked through all the android devices that start with 'i[A-Z]..' are following the 'iDevice' naming model.  This is just a heuristic.

> Here you call onList..Failed with an error argument, but the idl for the listener shows Failed taking no arguments.
> ...
> Again, argument here, no argument in listener idl

Right you are.  Removed the error argument from the call.

> ::: netwerk/base/NetworkInfoServiceCocoa.cpp
> @@ +19,5 @@
> > +namespace mozilla {
> > +namespace net {
> > +
> > +static nsresult
> > +ListInterfaceAddresses(int aFd, const char* aIface, AddrMapType& aAddrMap);
>
> Hrm... I feel like we should be able to unify the Linux & OS X
> implementations into a NetworkInfoServiceUnix.cpp. So much duplicate
> code makes me sadface.

I want to keep these separate because I think there's a better way to get the hostname on MacOS that uses the Cocoa APIs.  I'm using the ioctl-based implementation for now, but I think we can do better when we come back to this.

> @@ +93,5 @@
> > +    return NS_OK;
> > +}
> > +
> > +nsresult
> > +GetHostnameImpl(nsACString& aHostname)
>
> So after going through the third one of these - on all machines that
> use the native (not js-bridged-to-java) implementation, it appears that
> GetHostnameImpl is 100% the same on all of them. Just unify them (while
> addressing review comments I made in the other two implementations).

Only the OSX and Linux implementations use the same calls, and I don't intend for that to remain the case in the future.  Windows uses Win32-specific APIs (GetIpAddrTable and a bunch of windows-specific structuers), and Android uses the Java bridge to call into Android APIs.

> ::: netwerk/base/NetworkInfoServiceImpl.h
> @@ +15,5 @@
> > +nsresult
> > +ListAddressesImpl(AddrMapType& aAddrMap);
> > +
> > +nsresult
> > +GetHostnameImpl(nsACString& aHostname);
>
> Not wild about having "Impl" in the names here - it doesn't add anything (other than 4 characters) in my opinion :)

ListAddresses and GetHostname are method names on the nsNetworkInfoService.  I'd prefer to keep the non-method top-level implementation functions separately named from the method names, for clarity's sake.

> @@ +18,5 @@
> > +nsresult
> > +GetHostnameImpl(nsACString& aHostname);
> > +
> > +} // namespace net
> > +} // namespace mozilla
>
> So there might be some overlap with this code and some code :bagder wrote
> for network identification (on at least windows/mac/linux, possibly android).
> We should ni? him and find out if there is - and if there is, we should
> try to unify if possible so we're not doing the same work in two different
> places (thus risking having a bug in both, but only fixing it in one place)

Needinfo-ed him on it.

> @@ +34,5 @@
> > +      close(fd);
> > +    });
> > +
> > +    struct ifconf ifconf;
> > +    char buf[16384];
>
> That's a scary high magic number - care to comment it? (I imagine it
> has some basis in the reality of what the ioctl gives back)

It's 16k.  I cribbed it from example code.. I think we just need something large enough that a list of interfaces is probably not going to exceed it.  Don't know if it needs to be anything special.  I can use 0x4000 if that makes it clearer.


> @@ +112,5 @@
> > +      break;
> > +    }
> > +  }
> > +
> > +  if (strlen(hostnameBuf) == 0) {
>
> Like with the windows impl, I'm torn on instant-understandability of
> strlen vs. almost-instant understandability + compactness of !hostnameBuf[0]

Aside from perf, I like the literate nature of this.  We're testing for an empty string, and this conveys that fact really clearly.  Stuff like |!hostnameBuf[0]| doesn't make it exactly clear what we're testing for.  I don't have a strong opinion on this, but I'd midly prefer to leave it as is.

> ::: netwerk/base/NetworkInfoServiceWindows.cpp
> @@ +20,5 @@
> > +{
> > +  UniquePtr<MIB_IPADDRTABLE> ipAddrTable;
> > +  DWORD size = sizeof(MIB_IPADDRTABLE);
> > +
> > +  ipAddrTable.reset((MIB_IPADDRTABLE*) malloc(size));
>
> Generally I would say "don't cast the malloc return value" - and I'm
> going to say that here too, but only on the condition that UniquePtr::reset
> can handle getting a void* where it expects a T*

reset() takes a typed Pointer, looking at the UniquePtr implementation.  I think we need the cast here.


::: netwerk/base/nsNetworkInfoService.js
@@ +20,5 @@
> +function setQueryInterface(cls, ...aQIList) {
> +  cls.prototype.QueryInterface = XPCOMUtils.generateQI(aQIList);
> +}
> +
> +class nsNetworkInfoService {

What?! Get out of here with this pretty, non-prototypcal-inheritance js! (Not really, this is awesome! Carry on!)

> ::: netwerk/base/nsSocketTransport2.h
> @@ +136,5 @@
> > +    // info).
> > +    nsresult InitPreResolved(const char **socketTypes, uint32_t typeCount,
> > +                             const nsACString &host, uint16_t port,
> > +                             const nsACString &hostRoute, uint16_t portRoute,
> > +                             nsIProxyInfo *proxyInfo,
>
> Do we really want to be able to pass proxy info in here? I thought the
> whole idea here was this is for locally-discovered services, so we
> shouldn't need a proxy here. Same thing goes for hostRoute & portRoute,
> really.
>
> I'm not saying it's definitely wrong, but something doesn't feel right.
> Seems more like InitPreResolved should take socketTypes, typeCount, host,
> port, and addr, and then the call through to Init would set dummy (empty)
> values for hostRoute, portRoute, and proxyInfo.

Technically InitPreResolved is for FlyWeb, but conceptually I see it as separate.
Overall the notion is that we always resolve through DNS to get the address.  InitPreResolved
provides a mechanism to initialize the transport in the case where some mechanism has been
used to resolve the hostname independently of DNS.  That happens to be FlyWeb in this case,
but it could technically be some arbitrary DNS-bypass mechanism.

It seems more true to the interface that we pass all arguments along, and FlyWeb just happens
to be a user of InitPreResolved that doesn't do stuff with proxyInfo.

> ::: netwerk/base/nsSocketTransportService2.cpp
> @@ +728,5 @@
> > +    // Check FlyWeb table for host mappings.  If one exists, then use that.
> > +    RefPtr<mozilla::dom::FlyWebService> fws =
> > +        mozilla::dom::FlyWebService::GetExisting();
> > +    if (fws) {
> > +        nsresult rv = fws->CreateTransportForHost(types, typeCount, host, port,
>
> I'm not crazy about having something in mozilla::dom being used to create
> a transport - it seems backwards (or at least poor separation of concerns).
> There may be no good way around it, though (I haven't looked at the dom
> portions of this patch yet)

There probably is a better way to do this, but not one that's easy to implement without
some heavy-handed refactoring to eliminate the assumption of "connections are always TCP
sockets".  The current system is built heavily with TCP + DNS assumptions in mind.

This a bit of warty hack, but it works quite well in practice, and it's pretty localized,
so I'm hoping you can let it slide :)

> ::: netwerk/dns/mdns/libmdns/fallback/DataReader.jsm
> @@ +15,5 @@
> > +    this._cursor = startByte;
> > +  }
> > +
> > +  get buffer() {
> > +    return this.data.buffer;
>
> Any reason to not just do |this._data.buffer| here? (since |get data()|
> just returns |this._data|)

Nope, no reason.  Well, maybe I can make up a reason if you really wanted me to, but it wouldn't be a good one :)

Comment 11

[Daniel Stenberg [:bagder]]

(In reply to Kannan Vijayan [:djvj] from comment #8)

> Daniel: Nick is commenting here on our code to obtain a list of network IP
> addresses, and a hostname, for the current host.  Do we have existing code
> that will do this?  Please see 'NetworkInfoService' (nsINetworkInfoService
> and related implementation) in the patch attached to this bug.

I'm not aware of any existing code that gets the host name.

We have code that obtains a list of all the interfaces and traverses that list (in the network change detection logic; netwerk/system/win32/nsNotifyAddrListener.cpp, netwerk/system/mac/nsNetworkLinkService.mm and netwerk/system/linux/nsNotifyAddrListener_Linux.cpp), but it isn't really "obtaining a list of network IP addresses" but looks at more things in the returned data: flags, names etc in the interfaces so it isn't immediately obvious to me that the chances for code reuse or making a common function for this are very good.

We don't use the host name in the network change jungle.

Comment 12

[Kannan Vijayan [:djvj]]

Landed the first set of changes in response to review:
http://hg.mozilla.org/projects/larch/rev/eaeb86dd4a19

Comment 13

[Kannan Vijayan [:djvj]]

Attachment: flyweb-network.patch

Updated "full diff" of network changes between moz-cental and flyweb branches.

Comment 14

[u408661]

Comment on attachment 8754505 [details] [diff] [review]
flyweb-network-updates.patch

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

Just one quick comment on the interdiff - I'll look through the full patch again (as well as respond to your comments) and proclaim r+/r- on that :)

::: netwerk/base/nsNetworkInfoService.cpp
@@ +41,5 @@
>      return NS_OK;
>    }
>  
>    uint32_t addrCount = addrMap.Count();
> +  const char** addrStrings = (const char**) malloc(sizeof(*addrStrings) * addrCount);

Shouldn't need the cast of the malloc return value here.

Comment 15

[u408661]

(In reply to Kannan Vijayan [:djvj] from comment #10)
> Comment responses.  See delta patch that's been posted.  I'll build a full
> patchset and post that as well as soon as I finish build-testing all the
> changes so far.
> 
> > ::: dom/flyweb/FlyWebConnection.cpp
> 
> All of FlyWebConnection.cpp has been removed.  We'll put it in later when
> it's actually going to be hooked up functionality.

:thumbsup:

> > ::: dom/flyweb/FlyWebService.cpp
> > @@ +86,5 @@
> > > +
> > > +  bool mDiscoveryActive;
> > > +  uint32_t mNumConsecutiveStartDiscoveryFailures;
> > > +
> > > +  DiscoveryState mDiscoveryState;
> >
> > Is there a reason we don't just have mDiscoveryActive as another state within mDiscoveryState? Seems like there's an opportunity for things to become out-of-sync
> 
> mDiscoveryState is separate from mDiscoveryActive because the state reflects
> the actual state of the current system, and the active flag reflects the
> desired state requested by calls.  They are loosely coupled and need to be
> so.
> 
> For example, when discovery is active, we repeatedly start and stop
> discovery on a timed basis.  So: mDiscoveryActive gets set to true, and
> initiates a loop whereby the mDiscoveryState goes from (IDLE => STARTING =>
> RUNNING => STOPPING => IDLE) repeatedly.  When mDiscoveryActive becomes
> false, the state transitions (which may be in the middle of STARTING,
> RUNNING, or STOPPING) are directed towards the IDLE Final state.
> 
> So let's say we make discovery active and we're currently IDLE.  We'll get
> ready to start discovery and move into the STARTING state.  Let's say we get
> a stop-discovery request before we have received the event that puts us in a
> RUNNING state.  We will set mDiscoveryActive to false, but do nothing else,
> because we can't take any immediate action when in the STARTING state.  Once
> we receive the event that puts us into the RUNNING state, we re-check the
> mDiscoveryActive field, and if false, kick off the stop-discovery process.
> 
> Splitting this up into two variables lets us cleanly separate the desired
> state of the system (whether discovery should be happening or not), from the
> mechanics of interacting with the discovery backend (what sutation the raw
> discovery state machine is in).

Excellent. Your comments you added really help clear this up, thanks.

> > @@ +235,5 @@
> > > +
> > > +  // Notify FlyWebService of changed service list.
> > > +  mService->NotifyDiscoveredServicesChanged();
> > > +
> > > +  // Start discovery again immediately.
> >
> > Does this risk some kind of stop/start/stop/start/... loop? What are the reasons OnDiscoveryStopped could fire, and why is it safe to restart immediately?
> 
> That is the intent.  When discovery is active, we start the network
> discovery, listen for a while, stop it, and start it again.. and keep doing
> that until we get a stop-discovery request from the frontend.  Discovery is
> only active when there are active listeners for discovery, which only
> happens when the user has opened the FlyWeb Discovered Services List UI.
> 
> Basically when discovery is active we poll the network on a slow loop.
> 
> There's a risk of repeatedly starting and failing to start, and busy-looping
> that way.. so we avoid that by keeping track of consecutive failures in
> mNumConsecutiveStartDiscoveryFailures.  If there are 3 failures in a row, we
> don't redo network discovery even if service discovery is active.

OK, I must've missed/forgotten about the failure count when I made that comment :)

> > @@ +630,5 @@
> > > +  return mServiceMap.Contains(aServiceId);
> > > +}
> > > +
> > > +nsresult
> > > +FlyWebMDNSService::ConnectToService(const nsAString& aServiceId,
> >
> > I don't see any actual connection going on here, only creation of a uri,
> > and setting up some info about a connection (which may or may not exist
> > at this point, I'm not sure)
> 
> Right, so this is also a terminology issue.  From a FlyWeb perspective, a
> user "discovers and connects to" a service.  This is a logical connection
> between the user and the service, not a network connection.  For traditional
> IP networks, this notion of "connection" is a no-op.  You don't have to do
> anything special to gain the ability to open a socket to request a resource
> from a host: you just need to know its IP and port.
> 
> For a bluetooth-based service, this "connection" step might be more
> meaningfull:  The user might be asked to enter a PIN or something to
> establish a connection to the service.  Basically, with IP networks we're
> used to the idea that if you can address something, you can open a socket to
> it.  In the FlyWeb context, you might be able to address a service (via
> discovery), but you may need to take some extra steps (establish a session
> with it in some transport-specific way), before you're able to actually make
> connections to it.
> 
> The caller of this method (FlyWebService::ConnectToService) takes the
> discovered service info and adds it to a "connected service table",
> associating it with the generated UUID as the hostname it's available under.
> Later on when we try to make network requests to URLS involving this
> generated hostname, we check that "connected service table" for the
> hostname, and if it exists, route the request to the appropriate service.
> 
> Maybe it would be clearer to refer to this as a "Session"?  I don't know. 
> I'm open to suggestions on how to name these things more clearly.

Ah, I see, this is effectively the same action as bluetooth or WiDi pairing. Yeah, I think a better name would go a long way to making this less confusing. Maybe "pair with service" or "mark service usable"? Add in a comment that, for IP networks "pairing" (or whatever you choose to call it) is a no-op in terms of actual network activity, and I think we're good to go.

> > @@ +733,5 @@
> > > +
> > > +void
> > > +FlyWebMDNSService::EnsureDiscoveryStopped()
> > > +{
> > > +  mDiscoveryActive = false;
> >
> > This does not appear to actually ensure discovery is stopped - nowhere is StopDiscovery called :)
> 
> This interacts with the state machine modeled by mDiscoveryState.  So, if
> mDiscoveryState is IDLE, we're all good because discovery is not running
> anyway.  If mDiscoveryState is not IDLE (e.g. it's STARTING, RUNNING, or
> STOPPING), then the callback handlers for each of those state transitions
> will check mDiscoveryActive and if it's false, push the network discovery
> state towards IDLE.

OK, Add a comment here (if you didn't already - I don't remember seeing it in the interdiff) to reduce the perceived mismatch in work done between this and EnsureDiscoveryStarted.

> > @@ +868,5 @@
> > > +  if (aRv.Failed()) {
> > > +    return nullptr;
> > > +  }
> > > +
> > > +  return promise.forget();
> >
> > This does not appear to actually connect to a server (nor does the promise
> > created actually seem to have anything directly to do with connecting to
> > a server). Should this be named differently? Is this creating a promise that
> > will be resolved when the connection completes?
> 
> This goes back to the semantic difference between a FlyWeb frontend notion
> of "connecting to a server" and the our technical understanding of
> connecting to a server (which is pretty much "open a socket to it").  From
> the perspective of a user, if you've 'connected' to a server, it means that
> you have performed the necessary preliminary steps so that you're able to
> make web requests from it.
> 
> For the MDNS backend, this connection process is very trivial.  It mostly
> involves the generation of a unique hostname for the server (to prevent
> servers from naming themselves and leading to origin-based issues), and
> marking the discovered service as "connected" so that we can route
> subsequent requests for that unique hostname to the correct local-network
> host (bypassing DNS).

OK, can we do some renaming to reduce the mismatch between the frontend notion of "connecting" and the technical details?

> > ::: dom/flyweb/HttpServer.cpp
> > @@ +1,1 @@
> > > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >
> > So generally speaking in this file - are there particular threads that the
> > methods are expected to be called on? Main thread? Socket thread? Please
> > MOZ_ASSERT for any thread you can.
> 
> CreateTransportForHost should be called from the main thread, but other than
> that everything else should be Main thread.

Do you mean CreateTransportForHost should be called from *off* the main thread?

> Quick question: what thread is nSocketTransportService2::CreateTransport
> called in?

Looks that everything that calls it (at least in HTTP land) is on the socket thread, but nsSocketTransportService2::CreateTransport itself never checks that (not sure if that's something we should fix in STS or not).

> > Also, I've noticed a few HTTP cases that aren't handled - Transfer-Encoding:
> > chunked comes to mind. I know we can probably rely on Firefox to not do
> > anything crazy, but someone trying to be malicious won't be acting how we
> > expect :) Have we had anyone fuzz this? We should.
> 
> No, no fuzzing yet :)  Do you mean chunked transfer on requests?  Yeah, we
> eventually need to cover fuzzing and more thorough handling of HTTP. 
> Stephanie Ouillon is looking into the initial security review of this stuff.

Excellent. I'm mostly just worried about the fact that we have zero handling for chunked (and C-E's like gzip, etc). Whether we accept it or not is less relevant, but we should know what our behavior is expected to be in those situations.

> > @@ +326,5 @@
> > > +  return NS_OK;
> > > +}
> > > +
> > > +NS_METHOD
> > > +HttpServer::Connection::ReadSegmentsFunc(nsIInputStream* aIn,
> >
> > This might be better called WriteSegmentsFunc, since its type is
> > nsWriteSegmentFun (SO MUCH FUN!), and it is, in fact, performing the part
> > of WriteSegments opposite your call to ReadSegments.
> >
> > BTW, if you loathe the naming of {Read,Write}Segments, you're not alone.
> > But, they're old and cranky, so what're you gonna do?
> 
> Jonas wrote this code, which I'm glad about ;)  I'll punt this decision to
> him.  From the caller perspective (mInput->ReadSegments(ReadSegmentsFunc,
> ...)), it seems like a reasonable name.
> 
> I'm not familiar enough with the API and usage here to really construct a
> useful opinion/argument on it.
> 
> Jonas: Can you comment?

Jonas and I had a discussion about it a few comments higher up. We'll see what comes of it :)

> > @@ +365,5 @@
> > > +
> > > +nsresult
> > > +HttpServer::Connection::ConsumeInput(const char*& aBuffer,
> > > +                                     const char* aEnd)
> > > +{
> >
> > So I worry a little bit about this - necko already has a lot of HTTP parsing
> > code (though not necessarily for requests), and HTTP/1.1 is... finicky, to
> > say the least :) Is there any way we can look into unifying or using what
> > necko already has available? Doing so would likely avoid potential issues in
> > the future.
> 
> This is a reasonable suggestion, but can we punt that for a post-London
> cleanup?
> I don't know if we have time to rework parsing entirely before June, given
> the other
> work that's ongoing.

Yeah, I think punting to a follow-up makes total sense. Just wanted to get it on people's radars, mostly. It will likely be a lot of work to unify.

> > @@ +416,5 @@
> > > +      // Since we've given the pipe unlimited size, we should never
> > > +      // end up needing to block.
> > > +      MOZ_ASSERT(rv != NS_BASE_STREAM_WOULD_BLOCK);
> > > +      if (NS_FAILED(rv)) {
> > > +        written = size;
> >
> > This feels suspect to me - if the write failed, why would we just pretend we wrote |size| bytes?
> 
> Fixed, we return the error |rv| now.

See comment 7 - Jonas explained the reasoning there, and understanding it, I think setting written = size makes sense (though maybe you and Jonas know better or had a separate discussion about it). Take a look at his and my discussion around that point, and see whether returning the error makes more sense, or just commenting to explain the reason makes more sense.

> > @@ +457,5 @@
> > > +  if (!str.EqualsLiteral("GET")) {
> > > +    return false;
> > > +  }
> > > +
> > > +  InternalHeaders* headers = aRequest->Headers();
> >
> > So I see (I think) that this type comes from the Fetch code - is there
> > any particular reason we didn't use nsHttp{Request,Response}Head from
> > necko? They might need some extending to fit this use case (for example,
> > parsing request headers instead of having them supplied by necko), but it
> > might be something to think about for a follow-up, so we're not duplicating
> > effort.
> 
> Noted.  Including "Jonas" in this comment so I can CTRL-F things in the
> response I should talk to him about.

Excellent. Again, I think a lot of this unification of parsing can/should be done in followups. It will probably be a decent amount of work (most of which is of the technical debt variety), but I think at least considering it would be a good idea in the long run.

> > @@ +609,5 @@
> > > +    if (mRemainingBodySize) {
> > > +      LOG_V("HttpServer::Connection::ConsumeLine(%p) - Starting consume body", this);
> > > +      mState = eBody;
> > > +
> > > +      // We use an unlimited buffer size here to ensure
> >
> > Unlimited buffer worries me - sounds like a potential for a DoS. I'm not
> > saying "no" to it (it may be reasonable in the 99 percentile case), but I
> > want to make sure we think really hard about it before moving forward.
> 
> I agree.  If the webpage hangs onto the request indefinitely without
> consuming
> the body, then it's just a bad server, and its clients will experience
> timeouts,
> which is fine.
> 
> Moreover, we don't really expect huge request sizes to show up.  I don't see
> why this has to be over a megabyte or so in size.
> 
> Jonas: can you comment?

So Jonas did comment, and both sides have decent arguments. I'll leave it up to you two to decide what the right course is (since you know FlyWeb better than anyone), but please make sure you document the decision well in comments so later readers can understand why things are how they are :)

> > @@ +706,5 @@
> > > +    if (pending.first() == aRequest) {
> > > +      MOZ_ASSERT(!handledResponse);
> > > +      MOZ_ASSERT(!pending.second());
> > > +
> > > +      pending.second() = aResponse;
> >
> > Is there another way to do this? It always looks weird, assigning a value
> > to the result of a function call... (maybe I'm just too set in my ways,
> > though)
> 
> Took a look at mozilla::Pair, there's no setFirst() or setSecond() methods..
> the design of the API seems to be intended to use the lvalue reference
> returns for assignment.

Yeah. Stupid modern C++... if only I hadn't learned C++ last millennium... :-P

> > ::: netwerk/base/NetworkInfoServiceCocoa.cpp
> > @@ +19,5 @@
> > > +namespace mozilla {
> > > +namespace net {
> > > +
> > > +static nsresult
> > > +ListInterfaceAddresses(int aFd, const char* aIface, AddrMapType& aAddrMap);
> >
> > Hrm... I feel like we should be able to unify the Linux & OS X
> > implementations into a NetworkInfoServiceUnix.cpp. So much duplicate
> > code makes me sadface.
> 
> I want to keep these separate because I think there's a better way to get
> the hostname on MacOS that uses the Cocoa APIs.  I'm using the ioctl-based
> implementation for now, but I think we can do better when we come back to
> this.

Alright, but please file a followup to either rewrite using Cocoa or unify the two (depending on the results of if there actually is a better way to do this on OS X).

> > @@ +93,5 @@
> > > +    return NS_OK;
> > > +}
> > > +
> > > +nsresult
> > > +GetHostnameImpl(nsACString& aHostname)
> >
> > So after going through the third one of these - on all machines that
> > use the native (not js-bridged-to-java) implementation, it appears that
> > GetHostnameImpl is 100% the same on all of them. Just unify them (while
> > addressing review comments I made in the other two implementations).
> 
> Only the OSX and Linux implementations use the same calls, and I don't
> intend for that to remain the case in the future.  Windows uses
> Win32-specific APIs (GetIpAddrTable and a bunch of windows-specific
> structuers), and Android uses the Java bridge to call into Android APIs.

So while that's true for ListAddressesImpl, unless I'm missing something, GetHostnameImpl is 100% the same across OS X, Linux, and Windows (and a diff of copy/pasted copies of each confirms - unless, of course, I messed up my pasting - wouldn't be the first time). GetHostnameImpl is the one I think we should unify (and just have a build-time #ifdef to differentiate between the android & non-android versions).

> > ::: netwerk/base/NetworkInfoServiceImpl.h
> > @@ +15,5 @@
> > > +nsresult
> > > +ListAddressesImpl(AddrMapType& aAddrMap);
> > > +
> > > +nsresult
> > > +GetHostnameImpl(nsACString& aHostname);
> >
> > Not wild about having "Impl" in the names here - it doesn't add anything (other than 4 characters) in my opinion :)
> 
> ListAddresses and GetHostname are method names on the nsNetworkInfoService. 
> I'd prefer to keep the non-method top-level implementation functions
> separately named from the method names, for clarity's sake.

That's reasonable. Historically, we seem to use "Do<Whatever>" instead of "<Whatever>Impl" in necko, so think about renaming that way, but I'm not going to r- you for this no matter which you choose :)

> > @@ +34,5 @@
> > > +      close(fd);
> > > +    });
> > > +
> > > +    struct ifconf ifconf;
> > > +    char buf[16384];
> >
> > That's a scary high magic number - care to comment it? (I imagine it
> > has some basis in the reality of what the ioctl gives back)
> 
> It's 16k.  I cribbed it from example code.. I think we just need something
> large enough that a list of interfaces is probably not going to exceed it. 
> Don't know if it needs to be anything special.  I can use 0x4000 if that
> makes it clearer.

No need to change the way you write the value, but let's add a comment explaining why you're using that value (even if it is just "I totally stole this from examples in manpages" :D)

> > @@ +112,5 @@
> > > +      break;
> > > +    }
> > > +  }
> > > +
> > > +  if (strlen(hostnameBuf) == 0) {
> >
> > Like with the windows impl, I'm torn on instant-understandability of
> > strlen vs. almost-instant understandability + compactness of !hostnameBuf[0]
> 
> Aside from perf, I like the literate nature of this.  We're testing for an
> empty string, and this conveys that fact really clearly.  Stuff like
> |!hostnameBuf[0]| doesn't make it exactly clear what we're testing for.  I
> don't have a strong opinion on this, but I'd midly prefer to leave it as is.

Fine by me. I don't imagine this will be a hot section of the code, so perf impact will probably be negligible :)

> > ::: netwerk/base/NetworkInfoServiceWindows.cpp
> > @@ +20,5 @@
> > > +{
> > > +  UniquePtr<MIB_IPADDRTABLE> ipAddrTable;
> > > +  DWORD size = sizeof(MIB_IPADDRTABLE);
> > > +
> > > +  ipAddrTable.reset((MIB_IPADDRTABLE*) malloc(size));
> >
> > Generally I would say "don't cast the malloc return value" - and I'm
> > going to say that here too, but only on the condition that UniquePtr::reset
> > can handle getting a void* where it expects a T*
> 
> reset() takes a typed Pointer, looking at the UniquePtr implementation.  I
> think we need the cast here.

Booooooooooooo. void* 4 lyfe! (Alright, leave it, since UniquePtr is going to be picky.)

> > ::: netwerk/base/nsSocketTransport2.h
> > @@ +136,5 @@
> > > +    // info).
> > > +    nsresult InitPreResolved(const char **socketTypes, uint32_t typeCount,
> > > +                             const nsACString &host, uint16_t port,
> > > +                             const nsACString &hostRoute, uint16_t portRoute,
> > > +                             nsIProxyInfo *proxyInfo,
> >
> > Do we really want to be able to pass proxy info in here? I thought the
> > whole idea here was this is for locally-discovered services, so we
> > shouldn't need a proxy here. Same thing goes for hostRoute & portRoute,
> > really.
> >
> > I'm not saying it's definitely wrong, but something doesn't feel right.
> > Seems more like InitPreResolved should take socketTypes, typeCount, host,
> > port, and addr, and then the call through to Init would set dummy (empty)
> > values for hostRoute, portRoute, and proxyInfo.
> 
> Technically InitPreResolved is for FlyWeb, but conceptually I see it as
> separate.
> Overall the notion is that we always resolve through DNS to get the address.
> InitPreResolved
> provides a mechanism to initialize the transport in the case where some
> mechanism has been
> used to resolve the hostname independently of DNS.  That happens to be
> FlyWeb in this case,
> but it could technically be some arbitrary DNS-bypass mechanism.

So... that's exactly what the routed arguments are for - DNS-bypass (in some way). Right now, they're used for Alt-Svc, which is basically an HTTP-based DNS CNAME. So right now, they're used for hostname->hostname mapping, but I'm not sure there's any reason we couldn't use them for a hostname->IP mapping. I think we should investigate getting rid of InitPreResolved, and just pass our pre-resolved information as the hostRoute and portRoute to regular ol' Init.

> It seems more true to the interface that we pass all arguments along, and
> FlyWeb just happens
> to be a user of InitPreResolved that doesn't do stuff with proxyInfo.

But if we end up keeping InitPreResolved, I think this sounds fine.

> > ::: netwerk/base/nsSocketTransportService2.cpp
> > @@ +728,5 @@
> > > +    // Check FlyWeb table for host mappings.  If one exists, then use that.
> > > +    RefPtr<mozilla::dom::FlyWebService> fws =
> > > +        mozilla::dom::FlyWebService::GetExisting();
> > > +    if (fws) {
> > > +        nsresult rv = fws->CreateTransportForHost(types, typeCount, host, port,
> >
> > I'm not crazy about having something in mozilla::dom being used to create
> > a transport - it seems backwards (or at least poor separation of concerns).
> > There may be no good way around it, though (I haven't looked at the dom
> > portions of this patch yet)
> 
> There probably is a better way to do this, but not one that's easy to
> implement without
> some heavy-handed refactoring to eliminate the assumption of "connections
> are always TCP
> sockets".  The current system is built heavily with TCP + DNS assumptions in
> mind.
> 
> This a bit of warty hack, but it works quite well in practice, and it's
> pretty localized,
> so I'm hoping you can let it slide :)

Yeah, I think this is probably OK.

Comment 16

[u408661]

Comment on attachment 8754919 [details] [diff] [review]
flyweb-network.patch

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

Looking pretty good so far. I definitely want to see one more round (and probably go through my own notes again to double-check things) before r+ing, but I think we're almost there!

::: dom/flyweb/FlyWebPublishedServer.cpp
@@ +163,5 @@
> +
> +  nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(GetOwner());
> +  AutoJSContext cx;
> +  GlobalObject
> +    global(cx, nsGlobalWindow::Cast(window)->FastGetGlobalJSObject());

Here I go bloviating on stylistic issues in not-my-module again, but I (personally) find this more readable as

  GlobalObject global(
      cx, ns...);

but like I said... not my module :)

::: dom/flyweb/FlyWebService.cpp
@@ +44,5 @@
> +  , public nsIDNSServiceResolveListener
> +  , public nsIDNSRegistrationListener
> +  , public nsITimerCallback
> +{
> +  friend class FlyWebService;

I know it's implicit, but explicitly calling out "private:" here might be helpful

::: dom/flyweb/HttpServer.cpp
@@ +386,5 @@
> +  return NS_OK;
> +}
> +
> +NS_METHOD
> +HttpServer::Connection::ReadSegmentsFunc(nsIInputStream* aIn,

See our discussion from a few comments up about naming - I still don't like ReadSegmentsFunc, but maybe it's just because I know way too much about {Read,Writ}Segments :)

::: netwerk/base/NetworkInfoServiceAndroid.jsm
@@ +35,5 @@
> +  Messaging.sendRequestForResult(msg)
> +    .then(result => callback(result, null),
> +          err => callback(null, typeof err === "number" ? err : FAILURE_INTERNAL_ERROR));
> +}
> +function sendNIS(type, data, callback) {

This doesn't seem to be used?

::: netwerk/base/NetworkInfoServiceCocoa.cpp
@@ +94,5 @@
> +    return NS_OK;
> +}
> +
> +nsresult
> +GetHostnameImpl(nsACString& aHostname)

Just reiterating my wish to unify the GetHostnameImpls :)

::: netwerk/base/NetworkInfoServiceImpl.h
@@ +12,5 @@
> +
> +typedef nsDataHashtable<nsCStringHashKey, nsCString> AddrMapType;
> +
> +nsresult
> +ListAddressesImpl(AddrMapType& aAddrMap);

nit: for the .h, put return type on the same line as the function name.

::: netwerk/base/NetworkInfoServiceWindows.cpp
@@ +32,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +  }
> +
> +  DWORD retVal = GetIpAddrTable(ipAddrTable.get(), &size, 0);

Let's put this second call to GetIpAddrTable in the failure handling above - no need to call twice if the first call succeeds.

DWORD retVal;
if ((retVal = GetIpAddrTable(...)) == ERROR_INSUFFICIENT_BUFFER) {
  ipAddrTable.reset(...);
  if (!ipAddrTable) {
    return ...;
  }
  retVal = GetIpAddrTable(...);
}

@@ +38,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  for (DWORD i = 0; i < ipAddrTable->dwNumEntries; i++) {
> +    /* stuff here */

Care to expand on the stuff? :)

::: netwerk/build/nsNetModule.cpp
@@ +48,5 @@
>  #endif
>  
> +#if defined(XP_MACOSX) || defined(XP_WIN) || \
> +    (defined(XP_LINUX) && !defined(MOZ_WIDGET_ANDROID))
> +#define BUILD_NETWORK_INFO_SERVICE 1

Seems like it would make more sense to make this defined in the moz.build, where you check which source files to build, as well.

::: netwerk/dns/mdns/libmdns/MDNSResponderOperator.cpp
@@ +631,5 @@
>    // Resolve TXT record
>    int count = TXTRecordGetCount(aTxtLen, aTxtRecord);
>    LOG_I("resolve: txt count = %d, len = %d", count, aTxtLen);
> +  nsCOMPtr<nsIWritablePropertyBag2> attributes = new nsHashPropertyBag();
> +  if (NS_WARN_IF(!attributes)) { return; }

I know this is a cut-n-paste job, but since we're here, fix this to follow style with body on its own line.

::: netwerk/test/unit/test_local_cert.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

Seems like this would belong better with the psm tests, no?

Comment 17

[Kannan Vijayan [:djvj]]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #15)
> Ah, I see, this is effectively the same action as bluetooth or WiDi pairing.
> Yeah, I think a better name would go a long way to making this less
> confusing. Maybe "pair with service" or "mark service usable"? Add in a
> comment that, for IP networks "pairing" (or whatever you choose to call it)
> is a no-op in terms of actual network activity, and I think we're good to go.

I've renamed all the internal code to use "pairWith" naming conventions instead.

> OK, Add a comment here (if you didn't already - I don't remember seeing it
> in the interdiff) to reduce the perceived mismatch in work done between this
> and EnsureDiscoveryStarted.

Done.

> Do you mean CreateTransportForHost should be called from *off* the main
> thread?

Yep, that's what I meant.  Accordingly, it takes a lock before it works with any of the core table data maintained by the FlyWeb discovery code.

> Jonas and I had a discussion about it a few comments higher up. We'll see
> what comes of it :)

Yeah, I'll leave it to him.

> > RE: HttpServer parsing unification.
> Yeah, I think punting to a follow-up makes total sense. Just wanted to get
> it on people's radars, mostly. It will likely be a lot of work to unify.

Done.  See bug 1275390.

> > Fixed, we return the error |rv| now.
> 
> See comment 7 - Jonas explained the reasoning there, and understanding it, I
> think setting written = size makes sense (though maybe you and Jonas know
> better or had a separate discussion about it). Take a look at his and my
> discussion around that point, and see whether returning the error makes more
> sense, or just commenting to explain the reason makes more sense.

Ok, I changed it back to the way it was.  I don't fully understand HOW it does what Jonas says it's doing, but I'll run with it.

> So Jonas did comment, and both sides have decent arguments. I'll leave it up
> to you two to decide what the right course is (since you know FlyWeb better
> than anyone), but please make sure you document the decision well in
> comments so later readers can understand why things are how they are :)

I'll go with the "when in doubt, leave as is" policy and not touch this for now.

> > Took a look at mozilla::Pair, there's no setFirst() or setSecond() methods..
> > the design of the API seems to be intended to use the lvalue reference
> > returns for assignment.
> 
> Yeah. Stupid modern C++... if only I hadn't learned C++ last millennium...
> :-P

Frankly it rubs me the wrong way too.  |setX(v)| is just so much more descriptive than |x() = v|.

> Alright, but please file a followup to either rewrite using Cocoa or unify
> the two (depending on the results of if there actually is a better way to do
> this on OS X).

Filed.  See bug 1275373.

> So while that's true for ListAddressesImpl, unless I'm missing something,
> GetHostnameImpl is 100% the same across OS X, Linux, and Windows (and a diff
> of copy/pasted copies of each confirms - unless, of course, I messed up my
> pasting - wouldn't be the first time). GetHostnameImpl is the one I think we
> should unify (and just have a build-time #ifdef to differentiate between the
> android & non-android versions).
> ...
> That's reasonable. Historically, we seem to use "Do<Whatever>" instead of
> "<Whatever>Impl" in necko, so think about renaming that way, but I'm not
> going to r- you for this no matter which you choose :)

Both done.  GetHostnameImpl has been moved out of the individual impl files and
into the main file.  The "Impl" suffix has been replaced with a "Do" prefix
for both of the prior "Impl" functions.

> No need to change the way you write the value, but let's add a comment
> explaining why you're using that value (even if it is just "I totally stole
> this from examples in manpages" :D)

Done.

> So... that's exactly what the routed arguments are for - DNS-bypass (in some
> way). Right now, they're used for Alt-Svc, which is basically an HTTP-based
> DNS CNAME. So right now, they're used for hostname->hostname mapping, but
> I'm not sure there's any reason we couldn't use them for a hostname->IP
> mapping. I think we should investigate getting rid of InitPreResolved, and
> just pass our pre-resolved information as the hostRoute and portRoute to
> regular ol' Init.
> 
> ...
> 
> But if we end up keeping InitPreResolved, I think this sounds fine.

From how it was described to me by either you or mcmanus, routed hosts are
specifically a HTTP2 thing allowing for secure resources to be served from
an alternate host from the actual origin host (e.g. for CDNs, etc.).

Given the design intent of routed hosts, it feels like it would be abusing
those semantics to use them for FlyWeb's purposes.  FlyWeb's interception of
requests is not to provide an alternate host, but to provide an alternate
way (non-DNS) to resolve a connection to a given host/origin.



(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #16)
> Here I go bloviating on stylistic issues in not-my-module again, but I
> (personally) find this more readable as
> 
>   GlobalObject global(
>       cx, ns...);
> 
> but like I said... not my module :)

The existing syntax was objectively ugly.  Fixed.  It all fits on one line
anyway.

> I know it's implicit, but explicitly calling out "private:" here might be
> helpful

Done.

> See our discussion from a few comments up about naming - I still don't like
> ReadSegmentsFunc, but maybe it's just because I know way too much about
> {Read,Writ}Segments :)

I'm very inclined to let you and Jonas duke it out on this :)  He's too busy with e10s compat to work on review fixes, so shall we put a pin on this?

> > +function sendNIS(type, data, callback) {
> 
> This doesn't seem to be used?

Indeed it is not.  Removed.

> > +  for (DWORD i = 0; i < ipAddrTable->dwNumEntries; i++) {
> > +    /* stuff here */
> 
> Care to expand on the stuff? :)

You know, just ip address stuff.
That was a leftover comment from when I was poking around the windows
socket code, just a search token to quickly find where to add the
loop body.  Comment removed.

> 
> ::: netwerk/build/nsNetModule.cpp
> @@ +48,5 @@
> >  #endif
> >  
> > +#if defined(XP_MACOSX) || defined(XP_WIN) || \
> > +    (defined(XP_LINUX) && !defined(MOZ_WIDGET_ANDROID))
> > +#define BUILD_NETWORK_INFO_SERVICE 1
> 
> Seems like it would make more sense to make this defined in the moz.build,
> where you check which source files to build, as well.

The cpp files for the impl are in 'netwerk/base'.  This file is in 'netwerk/build'.  Not sure how to go about this - add a config option in 'netwerk/base/moz.build' that shows up as a #define in 'network/build/nsNetModule.cpp'.

> 
> ::: netwerk/dns/mdns/libmdns/MDNSResponderOperator.cpp
> @@ +631,5 @@
> >    // Resolve TXT record
> >    int count = TXTRecordGetCount(aTxtLen, aTxtRecord);
> >    LOG_I("resolve: txt count = %d, len = %d", count, aTxtLen);
> > +  nsCOMPtr<nsIWritablePropertyBag2> attributes = new nsHashPropertyBag();
> > +  if (NS_WARN_IF(!attributes)) { return; }
> 
> I know this is a cut-n-paste job, but since we're here, fix this to follow
> style with body on its own line.

This file is _full_ of that convention.  Filed a bug to fix all of those: bug 1275396


> Seems like this would belong better with the psm tests, no?

What/where are those?

Comment 18

[Kannan Vijayan [:djvj]]

Pushed fixes relating to review comments: https://hg.mozilla.org/projects/larch/rev/96e80e3b85ff

Comment 19

[u408661]

(In reply to Kannan Vijayan [:djvj] from comment #17)
> > So... that's exactly what the routed arguments are for - DNS-bypass (in some
> > way). Right now, they're used for Alt-Svc, which is basically an HTTP-based
> > DNS CNAME. So right now, they're used for hostname->hostname mapping, but
> > I'm not sure there's any reason we couldn't use them for a hostname->IP
> > mapping. I think we should investigate getting rid of InitPreResolved, and
> > just pass our pre-resolved information as the hostRoute and portRoute to
> > regular ol' Init.
> > 
> > ...
> > 
> > But if we end up keeping InitPreResolved, I think this sounds fine.
> 
> From how it was described to me by either you or mcmanus, routed hosts are
> specifically a HTTP2 thing allowing for secure resources to be served from
> an alternate host from the actual origin host (e.g. for CDNs, etc.).
> 
> Given the design intent of routed hosts, it feels like it would be abusing
> those semantics to use them for FlyWeb's purposes.  FlyWeb's interception of
> requests is not to provide an alternate host, but to provide an alternate
> way (non-DNS) to resolve a connection to a given host/origin.

Hrm... now that you mention this, I think I remember seeing that conversation scroll by on IRC a while back. OK, I'm fine with this as is.

> > See our discussion from a few comments up about naming - I still don't like
> > ReadSegmentsFunc, but maybe it's just because I know way too much about
> > {Read,Writ}Segments :)
> 
> I'm very inclined to let you and Jonas duke it out on this :)  He's too busy
> with e10s compat to work on review fixes, so shall we put a pin on this?

Sure. Maybe add a comment saying ReadSegmentsFunc is playing the part of WriteSegments, just so weirdos like me will understand what's going on?

> > > +  for (DWORD i = 0; i < ipAddrTable->dwNumEntries; i++) {
> > > +    /* stuff here */
> > 
> > Care to expand on the stuff? :)
> 
> You know, just ip address stuff.

Story of my life, man :D

> > ::: netwerk/build/nsNetModule.cpp
> > @@ +48,5 @@
> > >  #endif
> > >  
> > > +#if defined(XP_MACOSX) || defined(XP_WIN) || \
> > > +    (defined(XP_LINUX) && !defined(MOZ_WIDGET_ANDROID))
> > > +#define BUILD_NETWORK_INFO_SERVICE 1
> > 
> > Seems like it would make more sense to make this defined in the moz.build,
> > where you check which source files to build, as well.
> 
> The cpp files for the impl are in 'netwerk/base'.  This file is in
> 'netwerk/build'.  Not sure how to go about this - add a config option in
> 'netwerk/base/moz.build' that shows up as a #define in
> 'network/build/nsNetModule.cpp'.

Oof, yeah, I missed that "different directories" thing. Let's just leave it as is.

> > Seems like this would belong better with the psm tests, no?
> 
> What/where are those?

security/manager/ssl/tests/unit

Comment 20

[Kannan Vijayan [:djvj]]

Attachment: flyweb-network.patch

Updated amalgamated network changes patch.

Comment 21

[u408661]

Comment on attachment 8756517 [details] [diff] [review]
flyweb-network.patch

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

Oy, sorry this round took so long. I got distracted by shiny things. I'm like a damned puppy or something!

Anyway, this is looking pretty good! One thing left (based on changes made in the last round), but r=me with that addressed!

::: netwerk/base/NetworkInfoServiceWindows.cpp
@@ +31,5 @@
> +    if (!ipAddrTable) {
> +      return NS_ERROR_FAILURE;
> +    }
> +    DWORD retVal = GetIpAddrTable(ipAddrTable.get(), &size, 0);
> +    if (retVal != NO_ERROR) {

Hrm, I think we need to re-organize this a little bit more (sorry). Right now, we're not handling the case when the first GetIpAddrTable fails for any reason other than ERROR_INSUFFICIENT_BUFFER.

DWORD retVal;
if ((retVal = GetIpAddrTable(...)) == ERROR_INSUFFICIENT_BUFFER) {
  ...
  retVal = GetIpAddrTable(...);
}
if (retVal != NO_ERROR) {
  return NS_ERROR_FAILURE;
}

Comment 22

[Kannan Vijayan [:djvj]]

Fixed and pushed: http://hg.mozilla.org/projects/larch/rev/23a96762bc11

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83b448bdcd80
Changes in preparation for FlyWeb landing.  Add nsINetworkInfoService and implementation for various platforms. r=hurley

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4053b6d8c77
Changes in preparation for FlyWeb landing.  Add JS implementation of nsDNSServiceDiscovery. r=hurley

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/83b448bdcd80
https://hg.mozilla.org/mozilla-central/rev/c4053b6d8c77

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Pulsebot from comment #23)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/83b448bdcd80
> Changes in preparation for FlyWeb landing.  Add nsINetworkInfoService and
> implementation for various platforms. r=hurley

There are a lot of problems with the Android pieces here. Worryingly, this somehow made it into the tree without anyone who knows that code reviewing it. Please back it out and we can talk about how to proceed.

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

My main problem is that NetworkInfoServiceAndroid is implemented in JS and uses messaging to talk to Java when there is likely no reason to do so. My preferences for how this should be implemented, in descending order:

1) Use the same implementation that we have for Linux. It's kinda Linux, after all.
2) Use getaddrinfo(). :gcp says they had their own version of something like this for webrtc, so maybe ask those folks for details there.
3) Use the SDK bindings facility (examples here[0]) to bind java.net.NetworkInterface and use that from NetworkInfoServiceAndroid.cpp.

I strongly suspect 1 or 2 will work out, but no way to know until we try.

[0] https://dxr.mozilla.org/mozilla-central/source/widget/android/bindings

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #27)
> 2) Use getaddrinfo(). :gcp says they had their own version of something like
> this for webrtc, so maybe ask those folks for details there.

Whoops, I think I meant getifaddrs()[0] here. It looks like this is missing in Android, however. The SIOCGIFCONF solution you already have for Linux should work, though.

[0] http://man7.org/linux/man-pages/man3/getifaddrs.3.html

Comment 29

[u408661]

FWIW, getifaddrs looks feasable on OS X, as well.

And yes, please have someone from the android team review the android bits before re-landing (see comment 4 and comment 28)

Comment 30

[u408661]

Also, see bug 1277546 - we need to change the conditions for building nsNetworkInfoService (you can fold that change into the re-landing here)

Comment 31

[Gian-Carlo Pascutto [:gcp]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #27)
> 1) Use the same implementation that we have for Linux. It's kinda Linux,
> after all.
> 2) Use getaddrinfo(). :gcp says they had their own version of something like
> this for webrtc, so maybe ask those folks for details there.

FWIW my memory was wrong and I thought of getifaddrs. getaddrinfo works on the Android versions we support (it didn't use to), we have in-tree replacements for getifaddrs.

Comment 32

[Kannan Vijayan [:djvj]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #26)
> (In reply to Pulsebot from comment #23)
> > Pushed by [e-mail]:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/83b448bdcd80
> > Changes in preparation for FlyWeb landing.  Add nsINetworkInfoService and
> > implementation for various platforms. r=hurley
> 
> There are a lot of problems with the Android pieces here. Worryingly, this
> somehow made it into the tree without anyone who knows that code reviewing
> it. Please back it out and we can talk about how to proceed.

Could you give me quick review on the patch?  I can work on updates and push to the tree before uplift, if that's an acceptable path forward.

I'd prefer to use the official android APIs for this, largely because we have to shim out to Java anyway for the gethostname() stuff (to allow for more sophisticated hostname code that uses the more robust android Device/Manufacturer APIs).. and this means we probably want to keep the message-pass-to-java approach.

Could you explain what the major problems are with the patch?

Comment 33

[Kannan Vijayan [:djvj]]

Attachment: network-info-service.patch

Comment 34

[Kannan Vijayan [:djvj]]

Attachment: nis-android-fix.patch

Patch changes implementation of nsINetworkInfoService on Android to use Linux C APIs instead of bridging to java via jsm.

Try run here ongoing here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ba8e8a1bf08

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8759282 [details] [diff] [review]
nis-android-fix.patch

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

Much better, thanks.

Comment 36

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8759244 [details] [diff] [review]
network-info-service.patch

Unsetting the r? on this patch -- we talked on IRC.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddb08c99af9a
Fix nsINetworkInfoService implementation on Android to use Linux C APIs instead of Android Java APIs. r=snorp

Comment 38

[Carsten Book [:Tomcat]]

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