Last Comment Bug 769764 - remove synchronous proxy API and synchronous DNS resolution in nsProxyAutoConfig.js
: remove synchronous proxy API and synchronous DNS resolution in nsProxyAutoCon...
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: Networking: DNS (show other bugs)
: Trunk
: All All
: -- normal with 11 votes (vote)
: mozilla18
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
: 235853 309582 344891 346081 356281 381543 597984 659322 789877 (view as bug list)
Depends on: 896206 235853 589445 750445 767005 773255 778181 785673 791457 791527 791531 795905 797684 799469 799470 815783 817533 828202 828632 829006 829597 829646 834357 902170 903451 910518
Blocks: 766973 791645 347307 364223 507578 628590 669759 687561 781732 784586 795168
  Show dependency treegraph
 
Reported: 2012-06-29 12:22 PDT by Josh Aas
Modified: 2015-07-29 05:16 PDT (History)
52 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 0 (218.93 KB, patch)
2012-08-31 18:37 PDT, Patrick McManus [:mcmanus]
cbiesinger: review-
Details | Diff | Splinter Review
patch 1 (216.13 KB, patch)
2012-09-11 13:16 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
patch 2 (216.52 KB, patch)
2012-09-11 14:28 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
jaas: superreview+
Details | Diff | Splinter Review
patch 3 (219.83 KB, patch)
2012-09-13 08:51 PDT, Patrick McManus [:mcmanus]
mcmanus: review+
mcmanus: superreview+
Details | Diff | Splinter Review
followup.0 (2.53 KB, patch)
2012-09-29 19:10 PDT, Patrick McManus [:mcmanus]
jaas: review+
Details | Diff | Splinter Review

Description Josh Aas 2012-06-29 12:22:39 PDT
We don't want to allow synchronous DNS queries on the main thread -- bug 766973.
Comment 1 Josh Aas 2012-07-14 12:12:39 PDT
*** Bug 659322 has been marked as a duplicate of this bug. ***
Comment 2 Patrick McManus [:mcmanus] 2012-07-24 13:53:28 PDT
Need to fix this in a way that removes blocking for IO as well when doing PAC file manipulations. see 767005 and 235853
Comment 3 Patrick McManus [:mcmanus] 2012-08-07 14:34:09 PDT
*** Bug 597984 has been marked as a duplicate of this bug. ***
Comment 4 Patrick McManus [:mcmanus] 2012-08-24 10:23:48 PDT
These changes are going to break at least foxy proxy. I'll notify the author.

in short - the pac file evaluation service (nsProxyAutoConfig.js) needs to be removed and replaced with a non xpcom c++ component because it needs to execute exclusively off the main thread as the PAC file format is inherently not suitable for the main thread due to the presence of dnsResolve().
Comment 5 Patrick McManus [:mcmanus] 2012-08-31 18:37:08 PDT
Created attachment 657512 [details] [diff] [review]
patch 0

Proxy resolution normally happens synchronously through nsIIOService::NewChannel calling nsProtocolProxyService::Resolve. NewChannel() is of course is a common interface called from many places with many different stacks so this is a practical problem.

The synchronous resolve can also block the main thread for long periods in at least 3 different ways:
1] if configured for "use system proxy settings" windows can block via wpad exploration looking for a proxy even if this LAN does not ever use one. That's our default configuration and showed up on a jank profiler :(
2] If using a PAC file the Resolve() will block while fetching the PAC file itself. There is a mechanism to prevent that, but the callers were not using it. (and indeed it involves having to do an additional async lookup later on, and if they were coded to do that they wouldn't have been doing the synchronous one in the first place). This failure mode isn't actually too bad because loading the pac file is mostly a one time occurance.
3] If a PAC file is in use it can call dnsResolve() with any hostname the javascript chooses. The PAC format requires us to provide that helper and it has a syncronous API so it can block for a very long time.

Unraveling this leads to lots of little features. These are the major pieces:

1] remove the nsiprotocolproxyservice::resolve() api and shift all callers over to ::asyncresolve. Let's get rid of interfaces that block the main thread.

1a] java uses the pluginapi for a synchronous proxy resolution - we're working on changing that too but those wheels turn a little slower.. so provide a temporary deprecated sync-resolve hack for that as part of nsiprotocolproxyservice2

2] nsiioservice::newchannel has to remain synchronous - there are far too many callers. So instead of having nsIIOService figure out the proxy for the channel, change to having the channels themselves work it out after ::asyncopen(). For HTTP this involves rearranging a little of the earliest code that relied on the connection info, but its not a big deal. For Websockets it was pretty easy, the IO service just needs to tell the unproxied channel what flags newChannel was called with so that the proxy lookup can be done correctly later on.. the most disruptive change is probably to FTP.. if FTP finds a SOCKS proxy then it can just install it after asyncopen() in the anticipated way, but if it finds a HTTP proxy that it will use it basically needs to throw away the FTP handler and instantiate an HTTP one instead. That was a lot easier when it was done in NewChannel(), but I implemented it with a redirect and that works very well too.

3] PAC resolution had to be moved to its own thread. The idl for it had to go away and the existing .js implementation needed to be redone in C++ with jsapi without the aid of xpconnect/sandbox and friends which are main thread only interfaces. bz, bholley, and bkaplan helped me muddle through and I am quite grateful. The dnsResolve() helper function was implemented with nsIDNSService::AsyncResolve() and spinning the event loop of the new PAC thread for which everything is known to be re-entrant.

4] In the case of Windows (and maybe OS X, I can't tell for sure) the implementations of nsISystemProxySettings can block and need to be made threadsafe and run off the main thread. (I put them on the pac thread and renamed that the proxy resolution thread). That involved changing the idl a little bit to avoid using interfaces that aren't thread safe (e.g. nsIURI). Unfortunately, the gconf linux implementation must be run on the main thread so a hacky accomodation is made for that. Fortunately it doesn't seem to block.

5] while I was restructuring the PAC resolution queue I made it fail fast in the case where the PAC file had already failed to load at least once (and we are retrying in the background)

On Testing:

* A whole bunch of nsIProtocolProxyService::resolve() calls needed to be converted to asyncResolve().. I filed and fixed a bug or two related to the existing asyncResolve() stuff previous to this patch

* test_protocolproxyservice also adds a quickie test for the deprecated resolve method that has been added

* I hand tested the following scenarios on each of windows, os x, and linux-gconf and linux-libproxy:

 no proxy with http, https, and ftp
 manual proxies for http, https and ftp
 manual socks proxy with http, https and ftp urls
 test with manual http proxy and a no-proxy filter
 a manually defined pac file with http htps and ftp urls.. the pac file used dnsresolve()
 system settings with manual proxies and http, https, and ftp urls
 system settings with a PAC file with http, https, and ftp urls

I'm sorry such a little initial goal (remove sync dns call) turned into such a large project.
Comment 6 Patrick McManus [:mcmanus] 2012-08-31 18:39:58 PDT
Comment on attachment 657512 [details] [diff] [review]
patch 0

Christian, if you understandably don't have the time to tackle this please let me know.. honza has been on holiday and jason is pretty backed up so I thought I'd see if it would work for you. Thanks!
Comment 7 Patrick McManus [:mcmanus] 2012-08-31 18:59:04 PDT
green try:
https://tbpl.mozilla.org/?tree=Try&rev=350fb3defef1

and there are opt windows and os x builds here if anyone wants to play with it:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mcmanus@ducksong.com-79701d341e04/
Comment 8 Josh Matthews [:jdm] 2012-08-31 21:47:24 PDT
Comment on attachment 657512 [details] [diff] [review]
patch 0

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

I hope you don't mind some extra, immediate feedback. I'm about halfway through nsPACMan.cpp and publishing my comments so far to avoid them being lost.

::: dom/plugins/base/nsPluginHost.cpp
@@ +747,3 @@
>    nsCOMPtr<nsIIOService> ioService;
>  
>    proxyService = do_GetService(NS_PROTOCOLPROXYSERVICE_CONTRACTID, &res);

Now unnecessary.

::: netwerk/base/public/nsIProtocolProxyService.idl
@@ +21,4 @@
>  interface nsIProtocolProxyService : nsISupports
>  {
>      /**
> +     * This flag was historically used for the syncrhonous Resolve() method

s/syncrhonous/synchronous

::: netwerk/base/src/ProxyAutoConfig.cpp
@@ +22,5 @@
> +// Additionally dnsResolve(host) and myIpAddress() are supplied in the same context
> +// but are implemented as c++ helpers. proxyAlert(msg) is similarly defined, but that
> +// is a gecko specific extension.
> +
> +static const char *pacUtils = 

sPacUtils or gPacUtils

@@ +287,5 @@
> +  pac_logToConsole(formattedMessage);
> +}
> +
> +static
> +JSBool geckoResolve(const nsCString &aHostName, nsCString &aDottedDecimal)

I find pointer outparams to be clearer from the caller's perspective that an argument will be modified.

@@ +294,5 @@
> +  nsCOMPtr<PACResolver> helper = new PACResolver();
> +  if (!dns || NS_FAILED(dns->AsyncResolve(aHostName, 0, helper,
> +                                          NS_GetCurrentThread(),
> +                                          getter_AddRefs(helper->mRequest))))
> +    return JS_FALSE;

These JS_(TRUE|FALSE) can just be true/false.

@@ +296,5 @@
> +                                          NS_GetCurrentThread(),
> +                                          getter_AddRefs(helper->mRequest))))
> +    return JS_FALSE;
> +
> +  // spin the event loop of the pac thread until lookup is complete

Add a period on this line to reduce confusion on reading the subsequent one.

@@ +324,5 @@
> +    return JS_FALSE;
> +  
> +  char *hostString = JS_EncodeString(cx, arg1);
> +  nsCString hostName(hostString);
> +  JS_free(cx, hostString);

I'm pretty sure this can use an nsDependentJSString instead, making it much easier to follow.

@@ +348,5 @@
> +  nsCString hostName;
> +
> +  nsCOMPtr<nsIDNSService> dns = do_GetService(NS_DNSSERVICE_CONTRACTID);
> +  if (!dns || NS_FAILED(dns->GetMyHostName(hostName))) {
> +    hostName.Assign("127.0.0.1");

AssignLiteral

@@ +353,5 @@
> +  }
> +
> +  nsCString dottedDecimal;
> +  if (!geckoResolve(hostName, dottedDecimal)) {
> +    dottedDecimal.Assign("127.0.0.1");

AssignLiteral

@@ +372,5 @@
> +  
> +  char *alertString = JS_EncodeString(cx, arg1);
> +  nsCString alertMessage(NS_LITERAL_CSTRING("PAC-alert: "));
> +  alertMessage += alertString;
> +  JS_free(cx, alertString);

I think nsDependentJSString is a win here as well just to avoid explicit memory management.

@@ +375,5 @@
> +  alertMessage += alertString;
> +  JS_free(cx, alertString);
> +
> +  pac_logToConsole(alertMessage);
> +  JS_SET_RVAL(cx, vp, JSVAL_VOID);  /* return undefined */

I do not believe this is necessary, but I would consult with a JS engine guru before staking my life on it.

@@ +395,5 @@
> + public:
> +  static JSRuntimeWrapper *Create()
> +  {
> +    JSRuntimeWrapper *entry = new JSRuntimeWrapper();
> +    NS_ENSURE_TRUE(entry, nullptr);

Infallible new makes this unnecessary.

@@ +448,5 @@
> +
> +  static JSClass sGlobalClass;
> +
> +  JSRuntimeWrapper()
> +    : mRuntime(NULL), mContext(NULL), mGlobal(NULL), mOK(false)

nullptr

@@ +507,5 @@
> +{
> +  mJSNeedsSetup = false;
> +  NS_ABORT_IF_FALSE(!mRunning, "JIT is running");
> +
> +  if (mJSRuntime) {

Unnecessary. Deleting a null pointer is a legal and harmless operation.

@@ +547,5 @@
> +
> +nsresult
> +ProxyAutoConfig::GetProxyForURI(const nsCString &aTestURI,
> +                                const nsCString &aTestHost,
> +                                nsACString &result)

Same feelings about reference outparams as earlier.

@@ +562,5 @@
> +  // while the event loop is spinning on a DNS function
> +  mRunning = true;
> +
> +  nsresult rv = NS_ERROR_FAILURE;
> +  jsval rval;

Move this down to its use.

@@ +568,5 @@
> +    JS_NewStringCopyZ(mJSRuntime->Context(), aTestURI.get());
> +  JSString *hostString =
> +    JS_NewStringCopyZ(mJSRuntime->Context(), aTestHost.get());
> +  jsval argv[2] = { STRING_TO_JSVAL(uriString), STRING_TO_JSVAL(hostString) };
> +  JSBool rv2 = JS_CallFunctionName(mJSRuntime->Context(), mJSRuntime->Global(),

Due to rv's ubiquitous association with nsresult, how about ok?

@@ +571,5 @@
> +  jsval argv[2] = { STRING_TO_JSVAL(uriString), STRING_TO_JSVAL(hostString) };
> +  JSBool rv2 = JS_CallFunctionName(mJSRuntime->Context(), mJSRuntime->Global(),
> +                                   "FindProxyForURL", 2, argv, &rval);
> +
> +  if (rv2 && JSVAL_IS_STRING(rval)) {

rval.isString()

@@ +573,5 @@
> +                                   "FindProxyForURL", 2, argv, &rval);
> +
> +  if (rv2 && JSVAL_IS_STRING(rval)) {
> +      rv = NS_OK;
> +      JSString *str = JSVAL_TO_STRING(rval);

rval.toString()

@@ +576,5 @@
> +      rv = NS_OK;
> +      JSString *str = JSVAL_TO_STRING(rval);
> +      char *cval = JS_EncodeString(mJSRuntime->Context(), str);
> +      result = nsCString(cval);
> +      JS_free(mJSRuntime->Context(), cval);

Same nsDependentJSString stuff here.

@@ +593,5 @@
> +  JS_MaybeGC(mJSRuntime->Context());
> +}
> +
> +ProxyAutoConfig::~ProxyAutoConfig()
> +{

MOZ_COUNT_DTOR

::: netwerk/base/src/ProxyAutoConfig.h
@@ +19,5 @@
> +    : mJSRuntime(nullptr)
> +    , mRunning(false)
> +    , mJSNeedsSetup(false)
> +    , mShutdown(false)
> +  {

MOZ_COUNT_CTOR

::: netwerk/base/src/nsIOService.cpp
@@ +1191,5 @@
> +    { }
> +
> +private:
> +    nsIInterfaceRequestor *mCallbacks;
> +    nsIEventTarget        *mTarget;

Bare XPCOM pointer members for async classes unnerve me.

::: netwerk/base/src/nsPACMan.cpp
@@ +43,5 @@
>  }
>  
>  //-----------------------------------------------------------------------------
>  
> +// The executeCallback runnable is trigger

s/is trigger/triggers/?

@@ +50,2 @@
>  
> +class executeCallback MOZ_FINAL : public nsIRunnable

ExecuteCallback and nsRunnable

@@ +52,4 @@
>  {
>  public:
>    NS_DECL_ISUPPORTS
> +  NS_DECL_NSIRUNNABLE

Unneeded.

@@ +84,5 @@
> +  mCallback->OnQueryComplete(mStatus, mPACString, mPACURL);
> +  mCallback = nullptr;
> +  return NS_OK;
> +}
> +NS_IMPL_THREADSAFE_ISUPPORTS1(executeCallback, nsIRunnable)

Unneeded.

@@ +92,5 @@
> +// The PAC thread must be deleted from the main thread, this class
> +// acts as a proxy to do that, as the PACMan is reference counted
> +// and might be destroyed on either thread
> +
> +class shutdownThread MOZ_FINAL : public nsIRunnable

ShutdownThread, nsRunnable

@@ +97,4 @@
>  {
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIRUNNABLE

Unneeded.

@@ +113,5 @@
> +  NS_ABORT_IF_FALSE(NS_IsMainThread(), "wrong thread");
> +  mThread->Shutdown();
> +  return NS_OK;
> +}
> +NS_IMPL_THREADSAFE_ISUPPORTS1(shutdownThread, nsIRunnable)

Unneeded.

@@ +121,5 @@
> +// pacLoadComplete allows the PAC thread to tell the main thread that
> +// the javascript PAC file has been installed (perhaps unsuccessfully)
> +// and that there is no reason to queue executions anymore
> +
> +class pacLoadComplete MOZ_FINAL : public nsIRunnable

Likewise with above.

@@ +151,5 @@
> +// executePACThreadAction is used to proxy actions from the main
> +// thread onto the PAC thread. There are 3 options: process the queue,
> +// cancel the queue, and setup the javascript context with a new PAC file
> +
> +class executePACThreadAction MOZ_FINAL : public nsIRunnable

As above.

@@ +192,5 @@
> +NS_IMETHODIMP
> +executePACThreadAction::Run()
> +{
> +  NS_ABORT_IF_FALSE(!NS_IsMainThread(), "wrong thread");
> +  if (mCancel) {

A single state variable would be clearer instead of these two boolean conditions.

@@ +204,5 @@
> +
> +    mPACMan->mPAC.Init(mSetupPACURI,
> +                       mSetupPACData);
> +
> +    nsCOMPtr<pacLoadComplete> runnable = new pacLoadComplete(mPACMan);

nsRefPtr

@@ +237,5 @@
>  PendingPACQuery::Complete(nsresult status, const nsCString &pacString)
>  {
>    if (!mCallback)
>      return;
> +  nsCOMPtr<executeCallback> runnable = new executeCallback(mCallback, status);

nsRefPtr

@@ +254,2 @@
>  
> +  nsCOMPtr<executeCallback> runnable = new executeCallback(mCallback, NS_OK);

nsRefPtr

@@ +287,5 @@
> +    if (NS_IsMainThread()) {
> +      mPACThread->Shutdown();
> +    }
> +    else {
> +      nsCOMPtr<shutdownThread> runnable = new shutdownThread(mPACThread);

nsRefPtr
Comment 9 Josh Matthews [:jdm] 2012-09-01 09:47:35 PDT
Comment on attachment 657512 [details] [diff] [review]
patch 0

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

::: netwerk/base/src/nsPACMan.cpp
@@ +324,5 @@
> +  if (mShutdown)
> +    return NS_ERROR_NOT_AVAILABLE;
> +
> +  // Maybe Reload PAC
> +  if (mPACURI && (PR_Now() > mScheduledReload))

I believe we want to use Timestamp::Now in new code when possible.

@@ +333,5 @@
>  
>    if (IsPACURI(uri)) {
> +    // deal with this directly instead of queueing it
> +    nsCAutoString pacString;
> +    query->Complete(NS_OK, pacString);

Passing EmptyCString() is clearer.

@@ +351,5 @@
>    }
>  
> +  if (mShutdown) {
> +    nsCAutoString pacString;
> +    query->Complete(NS_ERROR_NOT_AVAILABLE, pacString);

EmptyCString

@@ +477,3 @@
>  {
> +  NS_ABORT_IF_FALSE(NS_IsMainThread(), "wrong thread");
> +  nsCOMPtr<executePACThreadAction> pending =

nsRefPtr

@@ +485,5 @@
> +void
> +nsPACMan::PostCancelPendingQ(nsresult status)
> +{
> +  NS_ABORT_IF_FALSE(NS_IsMainThread(), "wrong thread");
> +  nsCOMPtr<executePACThreadAction> pending =

nsRefPtr

@@ +487,5 @@
> +{
> +  NS_ABORT_IF_FALSE(NS_IsMainThread(), "wrong thread");
> +  nsCOMPtr<executePACThreadAction> pending =
> +    new executePACThreadAction(this);
> +  pending->CancelQueue (status);

nit: extra space after name

@@ +501,5 @@
> +  nsRefPtr<PendingPACQuery> query;
> +
> +  while (!mPendingQ.isEmpty()) {
> +    query = dont_AddRef(mPendingQ.popLast());
> +    query->Complete(status, pacString);

EmptyCString

@@ +537,5 @@
> +  nsCAutoString pacString;
> +  nsRefPtr<PendingPACQuery> query(dont_AddRef(mPendingQ.popFirst()));
> +
> +  if (mShutdown || IsLoading()) {
> +    query->Complete(NS_ERROR_NOT_AVAILABLE, pacString);

EmptyCString

@@ +568,5 @@
> +
> +  // the systemproxysettings didn't complete the resolution. try via PAC
> +  if (!completed) {
> +    nsresult status = mPAC.GetProxyForURI(query->mSpec, query->mHost, pacString);
> +    query->Complete(status, pacString);

EmptyCString

@@ +620,5 @@
> +    // are main thread only, unfortunately we have to initialize the instance of
> +    // the PAC evaluator (NS_PROXYAUTOCONFIG_CONTRACTID) on the pac thread, because
> +    // that is where it will be used.
> +
> +    nsCOMPtr<executePACThreadAction> pending =

nsRefPtr

::: netwerk/base/src/nsPACMan.h
@@ +47,5 @@
> +                               const nsCString &pacString,
> +                               const nsCString &newPACURL) = 0;
> +};
> +
> +class PendingPACQuery MOZ_FINAL : public nsIRunnable,

Same nsRunnable stuff as before.

@@ +142,5 @@
> +
> +  NS_HIDDEN_(nsresult) Init(nsISystemProxySettings *);
> +  static nsPACMan *sInstance;
> +
> +  // PAC thread operations olny

s/olny/only/

@@ +184,5 @@
> +   * execute the queue if it was otherwise empty
> +   */
> +  nsresult PostQuery(PendingPACQuery *query);
> +
> +  // PAC thread operations olny

s/olny/only/

::: netwerk/base/src/nsProtocolProxyService.cpp
@@ +244,5 @@
>      bool      mDispatched;
>      uint32_t  mResolveFlags;
>  
> +    nsProtocolProxyService            *mPPS;
> +    nsCOMPtr<nsIProtocolProxyService>  mXPComPPS;

Why this change? What's wrong with the refptr?

@@ +751,3 @@
>          }
> +        nsCOMPtr<nsIProxyInfo> tmp = pi;
> +        NS_ADDREF(*result = pi);

Either make the original declaration a COM pointer or get rid of this; it shouldn't be necessary.

@@ +861,5 @@
> +        NS_SUCCEEDED(mSystemProxySettings->GetThreadsafe(&threadsafe)) &&
> +        threadsafe)
> +        rv = mPACMan->Init(mSystemProxySettings);
> +    else
> +        rv = mPACMan->Init(nullptr);

Brace these blocks.

@@ +872,5 @@
> +nsresult
> +nsProtocolProxyService::ResetPACThread()
> +{
> +    if (!mPACMan)
> +        return NS_OK;

Is there any chance this could be called before the first SetupPACThread and mess up some subsequent checks for mPACMan?

@@ +966,5 @@
> +    void OnQueryComplete(nsresult status,
> +                         const nsCString &pacString,
> +                         const nsCString &newPACURL)
> +    {
> +        mMutex.Lock();

AutoMutex

@@ +989,5 @@
> +
> +    Mutex    mMutex;
> +    CondVar  mCondVar;
> +
> +    bool     mCompleted;

This will pack better if placed at the end.

::: netwerk/protocol/ftp/nsFTPChannel.cpp
@@ +133,5 @@
> +nsFtpChannel::GetProxyURI(nsIURI **aProxyURI)
> +{
> +    nsCOMPtr<nsIURI> copy(mProxyURI);
> +    *aProxyURI = copy.get();
> +    copy.forget();

Just do the usual NS_IF_ADDREF(*aProxyURI = mProxyURI) instead.

::: netwerk/protocol/ftp/nsFTPChannel.h
@@ +108,5 @@
>      nsCString                 mEntityID;
>      bool                      mResumeRequested;
>      PRTime                    mLastModifiedTime;
> +    uint32_t                  mProxyResolveFlags;
> +    nsCOMPtr<nsIURI>          mProxyURI;

Move this up with the other COM ptrs for better packing.

::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
@@ +2174,5 @@
>      return nsBaseContentStream::CloseWithStatus(status);
>  }
>  
> +static
> +nsresult CreateHTTPProxiedChannel(nsIURI *uri, nsIProxyInfo *pi, nsIChannel **newChannel)

nit: Type joins static's line in this file.

@@ +2211,5 @@
> +        // be reliabliy determined synchronously without jank due to pac, etc..
> +        LOG(("FTP:(%p) Configured to use a HTTP proxy channel\n", this));
> +
> +        nsCOMPtr<nsIChannel> newChannel;
> +        if (NS_SUCCEEDED(CreateHTTPProxiedChannel (uri, pi,

Nit: extra space after name.

@@ +2215,5 @@
> +        if (NS_SUCCEEDED(CreateHTTPProxiedChannel (uri, pi,
> +                                                   getter_AddRefs(newChannel))) &&
> +            NS_SUCCEEDED(mChannel->Redirect(newChannel, 
> +                                            nsIChannelEventSink::REDIRECT_INTERNAL,
> +                                            true))){

Nit: space before {

::: netwerk/protocol/ftp/nsFtpConnectionThread.h
@@ +34,5 @@
>  #include "nsFtpControlConnection.h"
>  
>  #include "nsICacheEntryDescriptor.h"
>  #include "nsICacheListener.h"
> +#include "nsICancelable.h"

Forward declare this instead.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1228,5 @@
> +
> +NS_IMETHODIMP
> +HttpChannelChild::SetProxyURI(nsIURI *aProxyURI)
> +{
> +  mProxyURI = aProxyURI;;

Nit; extra ;

@@ +1237,5 @@
> +HttpChannelChild::GetProxyURI(nsIURI **aProxyURI)
> +{
> +  nsCOMPtr<nsIURI> copy(mProxyURI);
> +  *aProxyURI = copy.get();
> +  copy.forget();

Same NS_IF_ADDREF request as the ftp channel code.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4384,5 @@
> +
> +    if (!mConnectionInfo->UsingConnect() && mConnectionInfo->UsingHttpProxy()) {
> +        mCaps &= ~NS_HTTP_ALLOW_PIPELINING;
> +        if (gHttpHandler->ProxyPipelining())
> +            mCaps |= ~NS_HTTP_ALLOW_PIPELINING;

This doesn't look right to me.

@@ +4439,3 @@
>          rv = classifier->Start(this);
> +        if (NS_FAILED(rv))
> +            return rv;

When BeginConnect is called from AsyncOpen and the classifier fails, this will no longer cancel the channel. Is that ok?

@@ +4509,5 @@
> +    }
> +    
> +    if (NS_FAILED(rv)) {
> +        Cancel(rv);
> +        DoNotifyListener();

Why is this explicitly necessary?

@@ +4542,5 @@
> +nsHttpChannel::GetProxyURI(nsIURI **aProxyURI)
> +{
> +    nsCOMPtr<nsIURI> copy(mProxyURI);
> +    *aProxyURI = copy.get();
> +    copy.forget();

Same as other NS_IF_ADDREF requests.

::: netwerk/test/unit/test_protocolproxyservice.js
@@ +277,5 @@
>  
>  function filter_test3_1(pi) {
>    check_proxy(pi, "http", "foo", 8080, 0, 10, false);
>    pps.unregisterFilter(filter_3_1);
> +  dump("done filter_test_3\n");

Use do_print for helpful info you want to leave; remove unneeded debug dump statements.

::: netwerk/test/unit/test_proxy-replace_canceled.js
@@ +41,5 @@
>      "function FindProxyForURL(url, host) {return 'PROXY localhost:4444';}"
>    );
>  
> +  // this test assumed that a AsyncOnChannelRedirect query is made for 
> +  // each proxy failover or on {the inital proxy only when PAC mode is used}.

What's with the {}?

::: toolkit/system/unixproxy/nsLibProxySettings.cpp
@@ +104,3 @@
>      }
> +    else {
> +      aResult.Append(NS_LITERAL_CSTRING("PROXY "));

AppendLiteral for both of these
Comment 10 Patrick McManus [:mcmanus] 2012-09-03 18:08:33 PDT
*** Bug 309582 has been marked as a duplicate of this bug. ***
Comment 11 Patrick McManus [:mcmanus] 2012-09-04 06:21:19 PDT
*** Bug 344891 has been marked as a duplicate of this bug. ***
Comment 12 Patrick McManus [:mcmanus] 2012-09-04 06:23:48 PDT
*** Bug 346081 has been marked as a duplicate of this bug. ***
Comment 13 Patrick McManus [:mcmanus] 2012-09-04 06:26:05 PDT
*** Bug 356281 has been marked as a duplicate of this bug. ***
Comment 14 Patrick McManus [:mcmanus] 2012-09-04 06:33:57 PDT
*** Bug 381543 has been marked as a duplicate of this bug. ***
Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2012-09-06 10:34:19 PDT
(In reply to Patrick McManus [:mcmanus] from comment #5)
> 2] nsiioservice::newchannel has to remain synchronous - there are far too
> many callers. So instead of having nsIIOService figure out the proxy for the
> channel, change to having the channels themselves work it out after
> ::asyncopen(). For HTTP this involves rearranging a little of the earliest
> code that relied on the connection info, but its not a big deal. 

I haven't read the patch yet, but doesn't HTTP deal with this already with the "unknown" proxy info and a REDIRECT_INTERNAL?

> I'm sorry such a little initial goal (remove sync dns call) turned into such
> a large project.

That was a big initial goal :p
Comment 16 Patrick McManus [:mcmanus] 2012-09-06 12:39:54 PDT
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #15)
> (In reply to Patrick McManus [:mcmanus] from comment #5)
> > 2] nsiioservice::newchannel has to remain synchronous - there are far too
> > many callers. So instead of having nsIIOService figure out the proxy for the
> > channel, change to having the channels themselves work it out after
> > ::asyncopen(). For HTTP this involves rearranging a little of the earliest
> > code that relied on the connection info, but its not a big deal. 
> 
> I haven't read the patch yet, but doesn't HTTP deal with this already with
> the "unknown" proxy info and a REDIRECT_INTERNAL?
> 

I should have talked about that directly. The HTTP channel, as you note, i already doing this with unknown - so this basically just removes type==unknown as it's not really helpful at all in practice.

The idl says that if the caller doesn't want to deal with unknown then they can just go direct. but, that's crazy talk :) The system is configured to use a proxy, and we've given the caller the means to figure out where that proxy is. There is no reason to make an exception on when that should be used, nor (more importantly) is there is a reason to think that DIRECT is an acceptable alternative. Basically, its an asynchronous world and callers need to live with it.

This is a good place to note that our default config on windows of use system proxy settings would need to return "unknown" to ::Resolve() because the wininet code it uses can block.

So for correctness, every protocol handler needs to be able to resolve "unknown" after AsyncOpen() using proxiedprotocolservice::AsyncResolve(). And that's exactly what my patch does (it just skips the unknown step as basically redundant). My patch also implements it for FTP, which was missing that bit of correctness before... and that's kind of an important type - the "unknown" interface can lure an implementation into skipping the important harder cases and basically being unusable.

HTTP could have continued to implement this with a redirect, but I decided that it was going to be so common we didn't want to do that for two major reasons:

1] the channel owner has to implement nsIChannelEventSink::AsyncOnChannelRedirect (I think, I'm not 100% sure if this fails to OK if not implemented).. since the channel interface is so widely used it should just do the right thing for callers that want proxies but don't want redirects.

2] redirects have problems with duplicating all the headers and other meta info that was changed on the original channel... there are recurring bugs around this.

and as a more minor point, the redirect is a pretty heavyweight process just for setting up the connection information and it can be avoided easily. (i.e. create and destroy a whole channel, a bunch of nsiuris, a series of permission callbacks, etc..)

As a nod towards realism (and the lesser importance of the use case), I did implement the FTP over HTTP case as a redirect :)

While the description of unknown is removed from the idl and the handling removed from httpChannel, I bet there are some vestigal remnants of type==unknown that I should scrub. Just note it the review overall section to remind me.
Comment 17 Christian :Biesinger (don't email me, ping me on IRC) 2012-09-06 13:32:06 PDT
Sounds good.

FYI, it is generally acceptable not to implement nsIChannelEventSink. The exception is if the caller stores a pointer to the nsIChannel, then it does have to implement the sink to be notified when the channel changes.

> 2] redirects have problems with duplicating all the headers and other meta info
> that was changed on the original channel... there are recurring bugs around
> this.

While true, redirects are a fact of life, so that logic needs to be there and be correct anyway.

(still working on the review, but answering these in the meantime)
Comment 18 Christian :Biesinger (don't email me, ping me on IRC) 2012-09-06 13:40:08 PDT
Comment on attachment 657512 [details] [diff] [review]
patch 0

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

::: browser/installer/removed-files.in
@@ -917,5 @@
>    components/nsPlacesDBFlush.js
>    components/nsPlacesExpiration.js
>    components/nsPrivateBrowsingService.js
>    components/nsPrompter.js
> -  components/nsProxyAutoConfig.js

this doesn't look right. this is for files that should be removed (see also the comment at the top of the file). what you actually want is add the .manifest here in the MOZ_OMNIJAR ifdef as well, and then add nsProxyAutoConfig.js and .manifest outside the ifdef.

::: netwerk/base/public/nsIProtocolProxyService.idl
@@ +21,5 @@
>  interface nsIProtocolProxyService : nsISupports
>  {
>      /**
> +     * This flag was historically used for the syncrhonous Resolve() method
> +     * which no longer exists. It does not not have any impact now as all

So... just remove it?

::: netwerk/base/public/nsIProtocolProxyService2.idl
@@ +12,1 @@
>  [scriptable, uuid(dbd9565d-29b1-437a-bff5-2fc339e2c5df)]

need new IID

::: netwerk/base/public/nsIProxiedChannel.idl
@@ +23,5 @@
>     * The returned proxy info must not be modified.
>     */
>    readonly attribute nsIProxyInfo proxyInfo;
> +
> +  /** 

no trailing whitespace please

@@ +26,5 @@
> +
> +  /** 
> +    * If the proxy is later determined from nsIProxyProtocolService::asyncResolve
> +    * use these flags
> +  */

indent this one space further

both of these apply to the other comment tooo

@@ +27,5 @@
> +  /** 
> +    * If the proxy is later determined from nsIProxyProtocolService::asyncResolve
> +    * use these flags
> +  */
> +  attribute unsigned long proxyResolveFlags;

At this point in reading this patch, it is completely unclear to me why these attributes exist and who would set them, which seems like a good indication that the comment needs improvement :-)

::: netwerk/base/public/nsISystemProxySettings.idl
@@ +17,3 @@
>  interface nsISystemProxySettings : nsISupports
>  {
> +    /** 

trailing whitespace

@@ +22,5 @@
> +     * threadSafe to avoid creating main thread jank. The non threadsafe option is
> +     * provided for implementations that do not block but use other main thread only
> +     * functions such as dbus.
> +     */
> +    readonly attribute bool threadsafe;

This should probably be called mainThreadOnly to clarify what it really means (there are three types of threadsafety: main-thread-only, single-thread-only, and fully-threadsafe, and you are trying to make a decision between the first two here)

@@ +32,5 @@
>      
>      /**
> +     * See ProxyAutoConfig::getProxyForURI; this function behaves similarly except
> +     * a more relaxed return string is allowed that includes full urls instead of just
> +     * host:port syntax. e.g. "PROXY http://www.foo.com:8080" instead of 

trailing ws

::: netwerk/base/src/ProxyAutoConfig.cpp
@@ +22,5 @@
> +// Additionally dnsResolve(host) and myIpAddress() are supplied in the same context
> +// but are implemented as c++ helpers. proxyAlert(msg) is similarly defined, but that
> +// is a gecko specific extension.
> +
> +static const char *pacUtils = 

trailing ws; please also fix elsewhere in this file

I assume the JS functions here are all a copy from nsProxyAutoConfig.js, so I haven't really reviewed them

@@ +29,5 @@
> +  "            host.substring(host.length - domain.length) == domain);\n"
> +  "}\n"
> +  ""
> +  "function dnsDomainLevels(host) {\n"
> +  "    return host.split('.').length-1;\n"

spaces around -

@@ +129,5 @@
> +  "    if (argc == 1) {\n"
> +  "        var tmp = parseInt(arguments[0]);\n"
> +  "        if (isNaN(tmp)) {\n"
> +  "            return ((isGMT ? date.getUTCMonth() : date.getMonth()) ==\n"
> +  "getMonth(arguments[0]));\n"

shouldn't this be indented?

@@ +264,5 @@
> +};
> +NS_IMPL_THREADSAFE_ISUPPORTS1(PACResolver, nsIDNSListener)
> +
> +static
> +void pac_logToConsole(nsCString &aMessage)

All caller either use literal strings or get data from the JS API. Why not use UTF-16 strings here, to avoid unnecessary conversions?

@@ +274,5 @@
> +
> +  consoleService->LogStringMessage(NS_ConvertUTF8toUTF16(aMessage).get());
> +}
> +
> +// Javascript errors are logged to the main error console if it is available.

"if it is available"? what does that mean? when isn't it?

@@ +279,5 @@
> +static void
> +pac_error_reporter(JSContext *cx, const char *message, JSErrorReport *report)
> +{
> +  nsCString formattedMessage(NS_LITERAL_CSTRING("PAC Execution Error: "));
> +  formattedMessage += message;

If switching to UTF-16, you could then use report->ucmessage here, and report->uclinebuf below.

@@ +287,5 @@
> +  pac_logToConsole(formattedMessage);
> +}
> +
> +static
> +JSBool geckoResolve(const nsCString &aHostName, nsCString &aDottedDecimal)

I would agree with Josh, though our code style, with "help" from our IDL bindings, like using string references... either way is fine with me

Also: your function naming seems kind of inconsistent:
pac_logToConsole
pac_error_reporter
geckoResolve

Can you make some sort of decision between underscores and interCaps? :)

@@ +297,5 @@
> +                                          getter_AddRefs(helper->mRequest))))
> +    return JS_FALSE;
> +
> +  // spin the event loop of the pac thread until lookup is complete
> +  // other than the DNS completion we only accept new additions to

are you enforcing that anywhere...?

@@ +324,5 @@
> +    return JS_FALSE;
> +  
> +  char *hostString = JS_EncodeString(cx, arg1);
> +  nsCString hostName(hostString);
> +  JS_free(cx, hostString);

yes please! (to what jdm said)

Also, a brief MXRing seems to indicate that we don't define JS_C_STRINGS_ARE_UTF8. Therefore, strings you get from the JS API are not UTF-8 and therefore lossy. So you should definitely use non-CStrings

@@ +372,5 @@
> +  
> +  char *alertString = JS_EncodeString(cx, arg1);
> +  nsCString alertMessage(NS_LITERAL_CSTRING("PAC-alert: "));
> +  alertMessage += alertString;
> +  JS_free(cx, alertString);

...and nsDependentJSString also avoids lossy string conversions

@@ +432,5 @@
> +  {
> +    mOK = true;
> +  }
> +  
> +  bool ISOK()

ISOK -> IsOK?

@@ +488,5 @@
> +};
> +
> +nsresult
> +ProxyAutoConfig::Init(const nsCString &aPACURI,
> +                      const nsCString &aPACScript)

(in the future, maybe this should become nsString&, to avoid being interpreted as ISO-8859-1. However as it is, this matches previous behaviour)

@@ +512,5 @@
> +    delete mJSRuntime;
> +    mJSRuntime = nullptr;
> +  }
> +
> +  if (mPACScript.IsEmpty())

you can probably make this an NS_ABORT_IF_FALSE because due to pacUtils, this should never be empty.

::: netwerk/base/src/ProxyAutoConfig.h
@@ +12,5 @@
> +namespace mozilla { namespace net {
> +
> +class JSRuntimeWrapper;
> +
> +class ProxyAutoConfig  {

You should add some documentation about threading. Looks like expected threading behaviour is: create/use/destroy on any non-main thread, but only a single thread.

::: netwerk/base/src/nsBaseChannel.cpp
@@ +339,5 @@
>  
>  NS_IMETHODIMP
>  nsBaseChannel::Cancel(nsresult status)
>  {
> +  // don't overwrite old cancel code

why is this change needed? you now call pump->cancel() if already cancelled, which should of course be harmless, but why the change?

::: netwerk/base/src/nsIOService.cpp
@@ +606,5 @@
>      rv = handler->NewChannel(aURI, result);
>      NS_ENSURE_SUCCESS(rv, rv);
> +    nsCOMPtr<nsIProxiedChannel> pc = do_QueryInterface(*result);
> +    if (pc) {
> +        pc->SetProxyResolveFlags(aProxyFlags);

I think I'd prefer replacing NewProxiedChannel with a function that can take the flag/uri argument. These two fields are really more like constructor arguments than anything else.

@@ +1191,5 @@
> +    { }
> +
> +private:
> +    nsIInterfaceRequestor *mCallbacks;
> +    nsIEventTarget        *mTarget;

yeah, this is not safe. you do need to addref.

::: netwerk/base/src/nsPACMan.cpp
@@ +224,5 @@
> +                                 bool mainThreadResponse)
> +  : mPACMan(pacMan)
> +  , mCallback(callback)
> +  , mOnMainThreadOnly(mainThreadResponse)
> +  

remove this empty line

::: netwerk/base/src/nsPACMan.h
@@ +54,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIRUNNABLE
> +
> +    PendingPACQuery(nsPACMan *pacMan, nsIURI *uri,

wrong indentation

@@ +204,5 @@
>    bool                         mShutdown;
>    PRTime                       mScheduledReload;
>    uint32_t                     mLoadFailureCount;
> +
> +  bool                         mNamed;

What's that for? I'd really like us to start documenting member variables.

::: netwerk/base/src/nsProtocolProxyService.cpp
@@ +191,5 @@
>              mPPS->ProcessPACString(mPACString, mResolveFlags,
>                                     getter_AddRefs(mProxyInfo));
>  
>              // Now apply proxy filters
> +            if (NS_SUCCEEDED(mStatus)) {

I'm confused by this change; mStatus is not changed since the if two lines up. Did you mean to assign the result of ProcessPACString to mStatus?

@@ +711,5 @@
> +
> +        // www.foo.com:8080 and http://www.foo.com:8080
> +        uint32_t maybeURLLen = end - start;
> +        if (*end)
> +            maybeURLLen--;

Why is this necessary?

@@ +712,5 @@
> +        // www.foo.com:8080 and http://www.foo.com:8080
> +        uint32_t maybeURLLen = end - start;
> +        if (*end)
> +            maybeURLLen--;
> +        nsCString maybeURL(start, maybeURLLen);

nsAutoCString, to avoid a heap allocation? Or even nsDependentSubstring to avoid a copy at all...

@@ +1500,4 @@
>              return NS_OK;
>          }
> +        
> +        nsCAutoString spec;

I think we renamed these to nsAutoCString. unless that didn't land yet?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4318,5 @@
> +    // all failures asynchronously.
> +    if (mLoadGroup)
> +        mLoadGroup->AddRequest(this, nullptr);
> +
> +    // Collect mAsyncOpenTime after we have called all obsrevers like

obsrevers -> observers

@@ +4509,5 @@
> +    }
> +    
> +    if (NS_FAILED(rv)) {
> +        Cancel(rv);
> +        DoNotifyListener();

jdm: why wouldn't it be necessary? What else would notify the listeners?

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1431,5 @@
>      } else {
>          httpChannel = new nsHttpChannel();
>      }
>  
> +    uint8_t caps = mCapabilities;

why did you make this change?

::: toolkit/system/unixproxy/nsUnixSystemProxySettings.cpp
@@ +192,5 @@
>    
> +  // this may return a string like "PROXY http://www.foo.com:8080" when it
> +  // should really be "PROXY www.foo.com 8080", but nsISystemProxySettings
> +  // allows that and callers should fix it up. This is because we are not on
> +  // the main thread and it is difficult to use the URI parser here.

you return false for threadsafe; doesn't this mean you _are_ the main thread?
Comment 19 Patrick McManus [:mcmanus] 2012-09-06 13:48:10 PDT
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #17)
>
> directs are a fact of life, so that logic needs to be there
> and be correct anyway.
> 

maybe we should put in nightly-channel-only code with a pointless internal redirect on every transaction just to see how many bugs we flush out. It would need someone with cycles to devote to the bug reports to spearhead it.
Comment 20 Laurent THALER 2012-09-10 02:21:58 PDT
Hi,

Got this bug at work using corporate proxy mainly on my netvibes page, hang on cdn.netvibes.com, need more than 100s with 15.0.1
52s only with this nightly build 18.0a1 

obviously it's better !
Comment 21 Patrick McManus [:mcmanus] 2012-09-10 05:24:54 PDT
(In reply to lthaler from comment #20)
> Hi,
> 
> Got this bug at work using corporate proxy mainly on my netvibes page, hang
> on cdn.netvibes.com, need more than 100s with 15.0.1
> 52s only with this nightly build 18.0a1 
> 
> obviously it's better !

Hi - there is no code for this bug yet checked into nightly, so you are reporting on something else.

Any chance you are using a proxy that does integrated windows authoriztion? and any chance you are using a osx or linux copy of firefox?
Comment 22 Georg Koppen 2012-09-10 05:31:14 PDT
I guess he/she meant https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mcmanus@ducksong.com-79701d341e04/ which are indeed shown as nightlies to the user.
Comment 23 Patrick McManus [:mcmanus] 2012-09-10 05:33:40 PDT
(In reply to Georg Koppen from comment #22)
> I guess he/she meant
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mcmanus@ducksong.
> com-79701d341e04/ which are indeed shown as nightlies to the user.

OK :)

in any event, this bug shouldn't be tangled up in something that is just running too slow (100s!).. that's a different kettle of fish.
Comment 24 Laurent THALER 2012-09-10 05:56:35 PDT
sorry but it's a proxy/autoconfig pb
pages loads quickly with IE and the same proxy, or with the same version of firefox (windows) without proxy
Comment 25 Patrick McManus [:mcmanus] 2012-09-10 06:03:22 PDT
thanks for the reply.

(In reply to Laurent THALER from comment #24)
> sorry but it's a proxy/autoconfig pb

I don't know what pb is.

> pages loads quickly with IE and the same proxy, or with the same version of
> firefox (windows) without proxy


can you open a different bug with an HTTP log attached to it? cc me.
https://developer.mozilla.org/en-US/docs/HTTP_Logging

thanks.
Comment 26 Vladimir Vukicevic [:vlad] [:vladv] 2012-09-10 08:22:56 PDT
Had my dad test the builds here -- they fixed the problem for him.
Comment 27 Christian :Biesinger (don't email me, ping me on IRC) 2012-09-10 16:25:44 PDT
(pb=problem, I assume)
Comment 28 Laszlo Ersek 2012-09-11 04:24:39 PDT
Filed a corresponding entry in the Red Hat bugzilla <https://bugzilla.redhat.com/show_bug.cgi?id=856170>.

Once done, please consider backporting the fix to Firefox ESR. Thank you.
Comment 29 Patrick McManus [:mcmanus] 2012-09-11 13:13:03 PDT
> 
> @@ +512,5 @@
> > +    delete mJSRuntime;
> > +    mJSRuntime = nullptr;
> > +  }
> > +
> > +  if (mPACScript.IsEmpty())
> 
> you can probably make this an NS_ABORT_IF_FALSE because due to pacUtils,
> this should never be empty.
> 

mPACScript is the concatenation of sPACUtils and the custom PAC - so it isn't static. And it gets truncated after feeding it into the JS engine.

>
> 
> ::: netwerk/base/src/nsProtocolProxyService.cpp
> 
> I'm confused by this change; mStatus is not changed since the if two lines
> up. Did you mean to assign the result of ProcessPACString to mStatus?
>

that was actually just a merge mistake on my part. 
 
> 
> ::: netwerk/protocol/http/nsHttpHandler.cpp
> @@ +1431,5 @@
> >      } else {
> >          httpChannel = new nsHttpChannel();
> >      }
> >  
> > +    uint8_t caps = mCapabilities;
> 
> why did you make this change?

to allow a later binding on the channel of the capabilities if the proxy is assigned after async open. The only difference between mCapabilities and mProxyCapabilities was the allow-pipeline bit, and that has been replaced with the mProxyPipelining bool which is looked at after he proxy is fully resolved.
Comment 30 Patrick McManus [:mcmanus] 2012-09-11 13:16:01 PDT
Created attachment 660203 [details] [diff] [review]
patch 1

christian, jdm - thanks. That made it stronger.

josh - christian has already had a look and is looking at the latest version.. need another set of eyes for sr on the idls too.
Comment 31 Christian :Biesinger (don't email me, ping me on IRC) 2012-09-11 13:43:12 PDT
Comment on attachment 660203 [details] [diff] [review]
patch 1

just skimming the patch, but there's a few things I noticed:

+  alertMessage.SetLength(32 + message.Length());
+  alertMessage += NS_LITERAL_STRING("PAC-alert: ");
+  alertMessage += message;

I'm pretty sure you want SetCapacity here instead of SetLength (or just not call it), because otherwise you'd get 32 bytes of "something" in the beginning (null? garbage? I'm not actually sure...).

+  hostName.init(cx, arg1);

feels like you ought to check the return value here and in the other DependentJSString users, unless this is in fact infallible?

+  if (!PACResolve(NS_LossyConvertUTF16toASCII(hostName), dottedDecimal))

This should be UTF-8, not lossy ASCII, so that IDN gets handled correctly (the DNS service will do the punycode conversion)
Comment 32 Patrick McManus [:mcmanus] 2012-09-11 14:23:38 PDT
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #31)
> Comment on attachment 660203 [details] [diff] [review]
> patch 1
> 
> just skimming the patch, but there's a few things I noticed:
> 
> +  alertMessage.SetLength(32 + message.Length());
> +  alertMessage += NS_LITERAL_STRING("PAC-alert: ");
> +  alertMessage += message;
> 
> I'm pretty sure you want SetCapacity here instead of SetLength 

embarrasing!
Comment 33 Patrick McManus [:mcmanus] 2012-09-11 14:28:18 PDT
Created attachment 660225 [details] [diff] [review]
patch 2

update for comment 31 .. also make sure PR_SetCurrentThreadName works when the proxy thread is only used for System-Based resolution instead of PAC execution.
Comment 34 Patrick McManus [:mcmanus] 2012-09-11 14:32:56 PDT
some os x and linux try results here https://tbpl.mozilla.org/?tree=Try&rev=82e87046936f

and windows try results here https://tbpl.mozilla.org/?tree=Try&rev=da8b78c8aa00 

they reflect patch 1 (the os x and linux ones are missing a trivial prototype issue that caused the windows build failure)
Comment 35 Christian :Biesinger (don't email me, ping me on IRC) 2012-09-11 15:05:05 PDT
as for ESR: this is actually a fairly complex patch, I don't know if it's appropriate to backport to ESR, even if it were easy to merge (probably not - I assume it depends on various trunk-only changes)
Comment 36 Kevin Brosnan [:kbrosnan] 2012-09-11 15:13:33 PDT
"Maintenance of each ESR, through point releases, is limited to high-risk/high-impact security vulnerabilities and in rare cases may also include off-schedule releases that address live security vulnerabilities. Backports of any functional enhancements and/or stability fixes are not in scope."

https://www.mozilla.org/en-US/firefox/organizations/faq/

If this fix were uplifted to Aurora (Fx 17) it will be part of the next Firefox 17 ESR channel.
Comment 37 Christian :Biesinger (don't email me, ping me on IRC) 2012-09-12 12:51:22 PDT
Comment on attachment 660225 [details] [diff] [review]
patch 2

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

::: netwerk/base/public/nsIProxiedProtocolHandler.idl
@@ +14,5 @@
>  {
>      /** Create a new channel with the given proxyInfo
>       *
> +     * @param uri the channel uri
> +     * @param proxyInfo any proxy information that has already been determined

", or null if channel should later determine the proxy on its own using proxyResolveFlags/proxyURI"? (Is that correct?)

@@ +16,5 @@
>       *
> +     * @param uri the channel uri
> +     * @param proxyInfo any proxy information that has already been determined
> +     * @param proxyResolveFlags used if the proxy is later determined
> +     *        from nsIProxyProtocolService::asyncResolve

typo: the interface is called nsIProtocolProxyService

@@ +18,5 @@
> +     * @param proxyInfo any proxy information that has already been determined
> +     * @param proxyResolveFlags used if the proxy is later determined
> +     *        from nsIProxyProtocolService::asyncResolve
> +     * @param proxyURI used if the proxy is later determined from
> +     *        nsIProxyProtocolService::asyncResolve with this as the proxyURI name.

So that function doesn't have a proxyURI argument, just aURI, so this comment needs clarification. This is only different from uri in case of proxies, right? Should probably mention that usually it is not different.

::: netwerk/base/public/nsISystemProxySettings.idl
@@ +18,5 @@
>  {
>      /**
> +     * Whether or not it is appropriate to execute getProxyForURI off the main thread.
> +     * If that method can block (e.g. for WPAD as windows does) then it must be
> +     * threadSafe to avoid creating main thread jank. The main thread only option is

must be threadSafe -> must not be mainThreadOnly

BTW, are you sure that dbus is not threadsafe? http://dbus.freedesktop.org/doc/api/html/group__DBusThreads.html#ga33b6cf3b8f1e41bad5508f84758818a7 seems to indicate otherwise. Also http://comments.gmane.org/gmane.comp.freedesktop.dbus/14013, which quotes http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#details

::: netwerk/base/src/ProxyAutoConfig.cpp
@@ +372,5 @@
> +
> +  nsDependentJSString message;
> +  if (!message.init(cx, arg1))
> +    return false;
> +  

no trailing whitespace, please

@@ +566,5 @@
> +  nsresult rv = NS_ERROR_FAILURE;
> +  JSString *uriString =
> +    JS_NewStringCopyZ(mJSRuntime->Context(), aTestURI.get());
> +  JSString *hostString =
> +    JS_NewStringCopyZ(mJSRuntime->Context(), aTestHost.get());

Is it possible that this function call triggers a garbage collection, collecting uriString? In other words, do you have to protect the first string from GC somehow?

(RootedString may be what's needed here. Check with some js engine person...)

@@ +575,5 @@
> +
> +  if (ok && rval.isString()) {
> +      nsDependentJSString pacString;
> +      if (pacString.init(mJSRuntime->Context(), rval.toString())) {
> +        result = NS_ConvertUTF16toUTF8(pacString);

CopyUTF16toUTF8(pacString, result);

::: netwerk/base/src/ProxyAutoConfig.h
@@ +36,5 @@
> +
> +  /**
> +   * Get the proxy string for the specified URI.  The proxy string is
> +   * given by the following:
> +   *   

trailing ws

::: netwerk/base/src/nsPACMan.cpp
@@ +22,5 @@
> +using namespace mozilla::net;
> +
> +// The PAC thread does evaluations of both PAC files and
> +// nsISystemProxySettings because they can both block the calling thread and we
> +// dont want that on the main thread

don't

@@ +157,5 @@
> +
> +  void SetupPAC (const char *text, uint32_t datalen, nsCString &pacURI)
> +  {
> +    mSetupPAC = true;
> +    mSetupPACData = nsCString(text, datalen);

I think .Assign(text, datalen) is what you want

@@ +302,5 @@
> +  if (mShutdown)
> +    return NS_ERROR_NOT_AVAILABLE;
> +
> +  // Maybe Reload PAC
> +  if (mPACURI && (PR_Now() > mScheduledReload))

hm, what about jdm's suggestion to use Timestamp::Now?

::: netwerk/base/src/nsProtocolProxyService.cpp
@@ +854,5 @@
> +    nsresult rv;
> +    if (mSystemProxySettings &&
> +        NS_SUCCEEDED(mSystemProxySettings->GetMainThreadOnly(&mainThreadOnly)) &&
> +        !mainThreadOnly)
> +        rv = mPACMan->Init(mSystemProxySettings);

please use {} around the then and else block, if your condition is multiline like here

@@ +1502,3 @@
>              return NS_OK;
>          }
> +        

lots of trailing whitespace in this file
Comment 38 Patrick McManus [:mcmanus] 2012-09-13 07:13:54 PDT
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #37)
> Comment on attachment 660225 [details] [diff] [review]

> ", or null if channel should later determine the proxy on its own using
> proxyResolveFlags/proxyURI"? (Is that correct?)

yes.

> BTW, are you sure that dbus is not threadsafe?

our DBUS implementation used a gconf event loop that got very unhappy when I tried to run it on the proxy resolution thread.
 
> 
> (RootedString may be what's needed here. Check with some js engine person...)

bholley gave me a great reply to this inquiry.. we need both rootedstring and rootedvalue to deal with upcoming generational gc changes.

> 
> hm, what about jdm's suggestion to use Timestamp::Now?
> 

I avoided this just because that pr_now() wasn't really my code - I just rearranged it - and thought I'd introduced enough regression risk already :).. but I've now yielded to peer pressure and made the change to TimeStamp
Comment 39 Patrick McManus [:mcmanus] 2012-09-13 08:51:09 PDT
Created attachment 660858 [details] [diff] [review]
patch 3

fyi.
Comment 41 Ed Morley [:emorley] 2012-09-13 14:48:57 PDT
Push backed out for m-oth permaorange in browser_463205.js:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e59b9b887c25
Comment 42 Patrick McManus [:mcmanus] 2012-09-14 13:50:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/828f91de7143
Comment 43 Marco Castelluccio [:marco] 2012-09-14 14:03:44 PDT
*** Bug 235853 has been marked as a duplicate of this bug. ***
Comment 44 Ryan VanderMeulen [:RyanVM] 2012-09-14 18:16:00 PDT
https://hg.mozilla.org/mozilla-central/rev/828f91de7143
Comment 45 Philip Chee 2012-09-14 22:29:57 PDT
comm-central appears broken:
e:/builds/slave/comm-cen-trunk-w32-dbg/build/mailnews/base/util/nsMsgUtils.cpp(2057) : error C2039: 'Resolve' : is not a member of 'nsIProtocolProxyService'

and

../../../../mailnews/base/util/nsMsgUtils.cpp:2057:19: error: ‘class nsIProtocolProxyService’ has no member named ‘Resolve’
Comment 46 Patrick McManus [:mcmanus] 2012-09-15 05:46:39 PDT
(In reply to Philip Chee from comment #45)
> comm-central appears broken:
> e:/builds/slave/comm-cen-trunk-w32-dbg/build/mailnews/base/util/nsMsgUtils.
> cpp(2057) : error C2039: 'Resolve' : is not a member of
> 'nsIProtocolProxyService'
> 
> and
> 
> ../../../../mailnews/base/util/nsMsgUtils.cpp:2057:19: error: ‘class
> nsIProtocolProxyService’ has no member named ‘Resolve’

yes, you'll need to use nsIProtocolProxyService::AsyncResolve() .. The synchronous version cannot safely be implemented to be non-blocking so it was removed as a source of jank.
Comment 47 Philip Chee 2012-09-15 06:12:24 PDT
> yes, you'll need to use nsIProtocolProxyService::AsyncResolve()

Perhaps what I should have said is:

We in comm-central land would appreciate a heads-up (a c-c bug would be great) whenever something like this that impacts comm-central builds lands on mozilla-central, instead of letting us discover stuff when things fall over.
Comment 48 Mark Banner (:standard8) 2012-09-15 10:22:00 PDT
Has the functionality of DeprecatedBlockingResolve changed when compared to the Resolve functionality before this bug landed?

I'm just wondering if we can temporarily use DeprecatedBlockingResolve whilst we rework the various protocol set up routines to handle this in an async fashion.
Comment 49 Patrick McManus [:mcmanus] 2012-09-15 11:48:45 PDT
DeprecatedBlockingResolve and the former Resolve() are equivalent, but I expect to eliminate the former very shortly. (bug 778201)
Comment 50 Ed Morley [:emorley] 2012-09-17 09:40:30 PDT
I've had to back this and two other changesets in the same push out, for extremely frequent OS X 10.8 mochitest-other leaks of the form found in bug 773255:
https://tbpl.mozilla.org/php/getParsedLog.php?id=15272317&tree=Mozilla-Inbound

http://brasstacks.mozilla.com/orangefactor/?display=Bug&tree=trunk&startday=2012-09-01&endday=2012-09-17&bugid=773255

https://hg.mozilla.org/integration/mozilla-inbound/rev/161847c0ac46
Comment 51 Ed Morley [:emorley] 2012-09-17 12:22:02 PDT
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/161847c0ac46
Comment 52 Patrick McManus [:mcmanus] 2012-09-18 06:06:39 PDT
*** Bug 789877 has been marked as a duplicate of this bug. ***
Comment 53 Patrick McManus [:mcmanus] 2012-09-27 10:35:51 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcae72a1333c
Comment 54 Ryan VanderMeulen [:RyanVM] 2012-09-27 20:16:21 PDT
https://hg.mozilla.org/mozilla-central/rev/dcae72a1333c
Comment 55 Philip Chee 2012-09-27 21:28:31 PDT
> https://hg.mozilla.org/mozilla-central/rev/dcae72a1333c
Did you remember to push dependent c-c bug 791457 as well?
Comment 56 Mark Banner (:standard8) 2012-09-28 01:32:21 PDT
(In reply to Philip Chee from comment #55)
> > https://hg.mozilla.org/mozilla-central/rev/dcae72a1333c
> Did you remember to push dependent c-c bug 791457 as well?

I've dealt with it.
Comment 57 Georg Koppen 2012-09-28 05:45:48 PDT
Trying to adapt FoxyProxy to these changes I figured out that even the manual proxy settings are broken. I was under the impression that just the PAC / system proxy settings part needs to get rewritten. Am I wrong here? The reason for the observed behavior seems to be that applyFilter() of our registered nsIProtocolProxyFilter is never called. I am not sure why yet, though.
Comment 58 Patrick McManus [:mcmanus] 2012-09-28 06:08:31 PDT
(In reply to Georg Koppen from comment #57)
> Trying to adapt FoxyProxy to these changes I figured out that even the

awesome

> manual proxy settings are broken. I was under the impression that just the
> PAC / system proxy settings part needs to get rewritten. Am I wrong here?

there is no synchronous resolve method, but other than that I don't think you should need any changes.

> The reason for the observed behavior seems to be that applyFilter() of our
> registered nsIProtocolProxyFilter is never called. I am not sure why yet,
> though.

there are lots of tests of applyFilter in http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_protocolproxyservice.js .. so it isn't 100% broken. Can you file a separate bug with a reduced test case? You can send me mail too.
Comment 59 :Ms2ger (⌚ UTC+1/+2) 2012-09-28 09:03:29 PDT
Comment on attachment 660858 [details] [diff] [review]
patch 3

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

::: dom/plugins/base/nsPluginHost.cpp
@@ +750,5 @@
>    if (NS_FAILED(res) || !proxyService)
>      return res;
>  
> +  proxyService2 = do_QueryInterface(proxyService, &res);
> +  if (NS_FAILED(res) || !proxyService)

Did you mean 'proxyService2'?

::: netwerk/base/src/Makefile.in
@@ +70,3 @@
>  		$(NULL)
>  
> +LOCAL_INCLUDES	+= -I$(topsrcdir)/dom/base

Please file a followup to export nsJSUtils instead

::: netwerk/base/src/ProxyAutoConfig.cpp
@@ +311,5 @@
> +}
> +
> +// dnsResolve(host) javascript implementation
> +static
> +JSBool PACDnsResolve(JSContext *cx, unsigned int argc, jsval *vp)

Looks like you're throwing a lot of uncatchable exceptions, but I guess that doesn't matter here

::: toolkit/components/passwordmgr/test/test_prompt_async.html
@@ +84,5 @@
>              }
>  
> +            var mozproxy = "moz-proxy://" +
> +                           SpecialPowers.wrap(pi).host + ":" +
> +	                   SpecialPowers.wrap(pi).port;

I'm afraid a hard tab crept in here

@@ +131,5 @@
> +
> +        if (!interfaces.some( function(v) { return iid.equals(v) } ))
> +	  throw Components.results.NS_ERROR_NO_INTERFACE;
> +	  return this;
> +	},

Here, too
Comment 60 Patrick McManus [:mcmanus] 2012-09-29 13:33:10 PDT
(In reply to :Ms2ger from comment #59)

> > +  proxyService2 = do_QueryInterface(proxyService, &res);
> > +  if (NS_FAILED(res) || !proxyService)
> 
> Did you mean 'proxyService2'?
> 
yes - thank you!
Comment 61 Patrick McManus [:mcmanus] 2012-09-29 19:10:04 PDT
Created attachment 666267 [details] [diff] [review]
followup.0

fix the error path check in the plugins code (that's why josh is the reviewer) and some whitespace problems in the base patch.
Comment 62 Patrick McManus [:mcmanus] 2012-09-30 18:30:59 PDT
 https://hg.mozilla.org/integration/mozilla-inbound/rev/02f3213b154f
Comment 63 Ed Morley [:emorley] 2012-10-01 06:05:54 PDT
https://hg.mozilla.org/mozilla-central/rev/02f3213b154f
Comment 64 Nick 2012-10-19 10:51:01 PDT
Is there any reason this patch is scheduled for FF18?  Can't it instead go into FF17 Extended Service Release?  Seems like a big fix to miss the ESR.

(forgive me if that's a dumb question, I'm new here)
Comment 65 Dave Garrett 2012-10-19 11:10:41 PDT
(In reply to Nick from comment #64)
> Is there any reason this patch is scheduled for FF18?  Can't it instead go
> into FF17 Extended Service Release?  Seems like a big fix to miss the ESR.

It involves completely removing the sync proxy API. It probably wouldn't be a great idea to backport that removal to a beta at this point.
Comment 66 Patrick McManus [:mcmanus] 2012-10-20 17:20:34 PDT
(In reply to Nick from comment #64)
> Is there any reason this patch is scheduled for FF18?  Can't it instead go
> into FF17 Extended Service Release?  Seems like a big fix to miss the ESR.
> 
> (forgive me if that's a dumb question, I'm new here)

there will be another train.

If ESR users would like more frequent trains then they are running the wrong channel. We have several other choices with more frequent updates.
Comment 67 Cameron Kaiser [:spectre] 2013-01-14 21:43:11 PST
This broke OverbiteFF's proxy handling also (direct connections still work). I'm reading through the patch, but nothing jumps out about how I need to change my channel's newProxiedChannel function. SOCKS is the only method supported, if that helps. Did this get documented anywhere?
Comment 68 Patrick McManus [:mcmanus] 2013-01-15 05:48:09 PST
Cameron, I'll work with you offline to help get your extension going. mail me at pmcmanus@mozilla.com when you've got a specific lead on the problem.
Comment 69 Cameron Kaiser [:spectre] 2013-01-15 19:43:58 PST
I sent you a message. Thanks.
Comment 70 David Bruant 2013-05-13 17:08:36 PDT
I tried to document the nsIProtocolProxyService bit (resolve removal)
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/18#Interface_changes
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIProtocolProxyService

Can you please tell me if the changes are erroneous and tell me if there is something else to document?
It's bitten some people apparently:
https://forums.mozilla.org/addons/viewtopic.php?p=25477&sid=175cc921bd80bf534be5600cda35a16f
https://github.com/moxie0/Convergence/issues/169

Thanks!
Comment 71 Rob Janssen 2013-09-21 03:07:47 PDT
Is it possible that Bug 903451 is caused by this change?
Is there any way the PAC code could cause a delay of exactly 3 seconds?
(maybe some default value for a timeout?)
Comment 72 Rob Janssen 2013-11-26 07:07:37 PST
Is it possible to have a workaround that disables the changes that this change caused to the
handling of the IMAP protocol?  I require a workaround that makes IMAP go back to the normal
startup performance (see bug 903451), but I do not require proxy support for IMAP, only for http(s).
So when it would be possible to write a small extension or some other way to work around this
problem I would really appreciate it.

Note You need to log in before you can comment on or make changes to this bug.