Last Comment Bug 282442 - Provide interface for configuring proxies
: Provide interface for configuring proxies
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: P1 enhancement (vote)
: mozilla1.8beta2
Assigned To: Darin Fisher
: benc
Mentors:
Depends on: 287690
Blocks: 76111 235853 264002 287646 287648
  Show dependency treegraph
 
Reported: 2005-02-16 00:14 PST by Darin Fisher
Modified: 2005-03-29 13:07 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 patch : incomplete stubbed out interfaces (47.78 KB, patch)
2005-02-28 19:43 PST, Darin Fisher
no flags Details | Diff | Review
v0.1 patch (169.08 KB, patch)
2005-03-04 18:29 PST, Darin Fisher
no flags Details | Diff | Review
v1 patch (173.25 KB, patch)
2005-03-10 20:12 PST, Darin Fisher
cbiesinger: review-
Details | Diff | Review
v1.1 (180.81 KB, patch)
2005-03-14 14:49 PST, Darin Fisher
no flags Details | Diff | Review
interdiff between v1 and v1.1 patches (25.30 KB, patch)
2005-03-14 14:50 PST, Darin Fisher
no flags Details | Diff | Review
v1.2 (181.71 KB, patch)
2005-03-15 12:09 PST, Darin Fisher
cbiesinger: review+
bzbarsky: superreview-
Details | Diff | Review
interdiff between v1 and v1.2 patches (25.61 KB, patch)
2005-03-15 12:10 PST, Darin Fisher
no flags Details | Diff | Review
v1.3 (182.90 KB, patch)
2005-03-23 17:21 PST, Darin Fisher
bzbarsky: superreview+
Details | Diff | Review
interdiff between v1.2 and v1.3 patches (11.02 KB, patch)
2005-03-23 17:25 PST, Darin Fisher
no flags Details | Diff | Review

Description Darin Fisher 2005-02-16 00:14:35 PST
Provide interface for configuring proxies.

I'd like to provide an interface so that an extension or embedder can easily
override the proxy configuration without affecting user visible preferences.

When nsProtocolProxyService::ExamineForProxy determines, using its existing
logic, that it should go direct, it would query this new interface to check if
another proxy should be used.  The new interface, or proxy provider, would
return a PAC string.

We may want to support multiple proxy providers, using some sort of ordering. 
Perhaps we'd query them all and concatenate the result, allowing proxy failover
logic to come into play.  Or perhaps we'd stop when the first proxy provider
returned a non-direct result.  I'm not sure which of those I prefer, but I am
concerned about performance.
Comment 1 Darin Fisher 2005-02-28 19:43:44 PST
Created attachment 175887 [details] [diff] [review]
v0 patch : incomplete stubbed out interfaces

This is an incomplete patch, but I'm posting it here for feedback and as a
place for safe keeping.  The design is as follows:

Allow filters to be run just before ExamineForProxy returns its result.  The
filters are allowed to freely modify the linked list of nsIProxyInfo objects.

This means freezing:
  nsIProtocolProxyFilter  - new interface with applyFilter method
  nsIProxyInfo		  - converted to a sane form
  nsIProtocolProxyService - so filters can call NewProxyInfo if desired

Filters are registered with an integer valued position that is used to build a
sorted list of filters.  The filters are chained together, with each filter
being applied to the output of the previous.

For example, this could be used to implement an extension that inserts a proxy
filter that replaces 'direct' with something more interesting (even when the
user has a PAC file configured).  This API could also be used to completely
take over the proxy settings.
Comment 2 Darin Fisher 2005-02-28 19:53:46 PST
BTW, this is something I'd like to finalize (and freeze) for 1.8.
Comment 3 Christian :Biesinger (don't email me, ping me on IRC) 2005-03-01 05:58:36 PST
nsIProtocolProxyService will need a new UUID...

what happens if two filters register for the same position?

nsIProtocolProxyFilter
+   * @param aProxy
+   *        The proxy (or list of proxies) that would be used by default for
+   *        the given URI.

So... if multiple filters are registered, then won't this be the proxy list from
the previous filter, instead of the default?

nsIProtocolProxyService
+     * @param aFailoverTimeout
+     *        Specifies the length of time to ignore this proxy if this proxy
+     *        fails.  Pass PR_UINT32_MAX to specify the default timeout value.

this should probably mention the unit (seconds?)


These interfaces will be called each time examineForProxy is called, right?
(I.e. not only for "direct" like comment 0 mentions). What will be passed as
proxyInfo when the default would be "direct"? newProxyInfo should probably
mention that the next proxy info parameter can be null.
Comment 4 Darin Fisher 2005-03-01 09:05:32 PST
> nsIProtocolProxyService will need a new UUID...

yup.. TODO


> what happens if two filters register for the same position?

the first to register for a position gets queried first (in other words, the
last one to register gets to trump the first one).


> nsIProtocolProxyFilter
> +   * @param aProxy
> +   *        The proxy (or list of proxies) that would be used by default for
> +   *        the given URI.
> 
> So... if multiple filters are registered, then won't this be the proxy list from
> the previous filter, instead of the default?

yes, that is correct.  i need to clarify that in the comments.


> nsIProtocolProxyService
> +     * @param aFailoverTimeout
> +     *        Specifies the length of time to ignore this proxy if this proxy
> +     *        fails.  Pass PR_UINT32_MAX to specify the default timeout value.
> 
> this should probably mention the unit (seconds?)

yup... seconds... TODO


> These interfaces will be called each time examineForProxy is called, right?
> (I.e. not only for "direct" like comment 0 mentions). What will be passed as
> proxyInfo when the default would be "direct"? newProxyInfo should probably
> mention that the next proxy info parameter can be null.

yes, these are called each time examineForProxy is called (provided the protocol
handler has the ALLOW_PROXY flag set), and the ALLOW_HTTP_PROXY flag will
probably still have to be applied to the filter results as a final step to
ensure that the filters don't try to violate those protocol flags.

the proxyinfo can be null in pretty much any context: as the parameter to
applyFilter, and as the parameter to newProxyInfo.  i need to clarify that point
in the comments.


thanks for the great feedback!
Comment 5 Darin Fisher 2005-03-01 09:19:55 PST
We probably want to include an async examineForProxy method to
nsIProtocolProxyService before freezing it since that'll be helpful when we try
to solve bug 136789, bug 100022, and bug 235853 (or bugs like it).
Comment 6 Christian :Biesinger (don't email me, ping me on IRC) 2005-03-01 09:38:31 PST
(In reply to comment #4)
> the proxyinfo can be null in pretty much any context: as the parameter to
> applyFilter, and as the parameter to newProxyInfo.  i need to clarify that point
> in the comments.

Ah, one more case: It can also be null when returned from applyFilter, right?
(to indicate a direct connection)

what about exceptions thrown from applyFilter? are they ignored (and the input
proxyInfo will be used for the next filter/for connecting)? something else?
Comment 7 Darin Fisher 2005-03-01 13:44:26 PST
> Ah, one more case: It can also be null when returned from applyFilter, right?
> (to indicate a direct connection)

yes.


> what about exceptions thrown from applyFilter? are they ignored (and the input
> proxyInfo will be used for the next filter/for connecting)? something else?

yeah, i think exceptions should be ignored, meaning that the filter is not
applied and that the input is then applied to the next filter.  I might make the
filter take an inout nsIProxyInfo parameter, but that can be a pain to work with.
Comment 8 Darin Fisher 2005-03-04 18:29:27 PST
Created attachment 176340 [details] [diff] [review]
v0.1 patch

OK, this patch is much further along.  I still need to do more testing to make
it final, and I may need to touch up some other files.

I haven't made HTTP use the new async proxy resolution API, but that's coming.

This patch introduces a PAC thread for processing the FindProxyForURL calls. 
So, if PAC decides to issue a DNS lookup, it will happily hang our background
thread instead of the UI thread :-)  That said, the synchronous proxy
resolution API still blocks on a monitor waiting for the PAC result, so we
still have to work to avoid calling that API.

Comments on this patch would be most appreciated.
Comment 9 Darin Fisher 2005-03-04 18:30:43 PST
Oh, btw... part of this patch includes a unit testing framework for Necko.  I'll
probably land that independently, but I've included it here so you can see the
new APIs in action.
Comment 10 Christian :Biesinger (don't email me, ping me on IRC) 2005-03-10 17:46:47 PST
Comment on attachment 176340 [details] [diff] [review]
v0.1 patch

netwerk/base/public/nsIProtocolProxyFilter.idl
should probably mention that returning null is ok

netwerk/base/public/nsIProtocolProxyService.idl
+    void cancelAsyncResolve(in nsISupports aContext);

Hm... I wonder if it might not be better to make this similar to
nsIDNSService::resolve, i.e. return something that has a cancel method. (maybe
a renamed nsIDNSRequest).

+     * NOTE: If a nsIProtocolHandler disallowes all proxying, then filters
will

"disallows"

+    void unregisterFilter(in nsIProtocolProxyFilter aFilter);

maybe the behaviour when trying to register a not registered filter should be
specified?

netwerk/base/public/nsIProxyAutoConfig.idl
+     * This method initializes the object.  This method may be called multiple
+     * times.
[...]
+    void init(in ACString aPACURI, in ACString aPACScript);

it's unclear to me what this init method does... why does it need a URI, given
it has the text?

Also, what about non-ascii characters? scripts might use IDN proxies, should
the text be AString?

+    ACString getProxyForURI(in ACString aTestURI, in ACString aTestHost);

So this will be the %-escaped URI, and the punycode-encoded host?

why this change?

netwerk/base/public/nsIProxyInfo.idl
+  readonly attribute unsigned long failoverTimeout;

is it worth documenting what PR_UINT32_MAX here means?

stopped at 
netwerk/base/src/nsPACMan.cpp, more tomorrow
Comment 11 Darin Fisher 2005-03-10 18:23:41 PST
I'm posting a new patch.  Please don't bother reading further until I do.
Comment 12 Darin Fisher 2005-03-10 20:12:37 PST
Created attachment 177098 [details] [diff] [review]
v1 patch

OK, here it is.  I scaled back the scope of this patch somewhat.  I am not
attempting to execute PAC on a background thread.  Instead, I pre-populate
the DNS cache before executing PAC.  This allows the DNS cache to hopefully
catch any synchronous DNS queries generated by the execution of the PAC
script.

This code also ensures that async PAC requests get queued up until the PAC
file is loaded.  This is an important part of ensuring that the user's
homepage is loaded properly at startup time.

Of course, this patch also includes the proxy filter API that was the 
original impetus for this bug.	Remember: my goal here is to freeze these
interfaces for Gecko 1.8, so I am very interested in getting the rest of
the API right as well.

After this patch goes in, we'll be able to start using async proxy queries
for HTTP loads, and that should go a long way to improving performance when
PAC is enabled.

Please let me know if you are not going to be able to review these changes
in a timely fashion.  I know it's a lot to ask, but I'd like to get these
changes on the trunk ASAP.  Thanks!!

BTW, biesi: I was thinking of introducing nsIDNSService::cancelAsyncResolve
as well since it eliminates an interface.  Thoughts?
Comment 13 Christian :Biesinger (don't email me, ping me on IRC) 2005-03-13 08:51:01 PST
Comment on attachment 177098 [details] [diff] [review]
v1 patch

netwerk/base/src/nsPACMan.cpp
+  if (uriSpec.IsEmpty()) {
+    Shutdown();
+    mShutdown = PR_FALSE;
+    return NS_OK;
+  }

OK... so an empty PAC uri makes the resolve methods return failure, and the
protocol proxy service will warn (and try DIRECT). seems this will only happen
if the user enters an empty url, so this seems ok. but, is this special-casing
needed? couldn't you just let NewChannel fail?

actually... independent from that, if NewChannel fails, should you null out
mPAC? if a user enters an invalid URL, I don't think we should reuse the
previous pac file...

+    nsCOMPtr<nsIRequest> request;
+    mLoader->GetRequest(getter_AddRefs(request));
+    if (request)
+      request->Cancel(NS_ERROR_ABORT);

I wonder if stream loaders should implement nsIRequest...

+      // We assume that the PAC text is ASCII.  We've had this assumption
+      // forever, so 

it seems something is missing at the end of this line?

+nsPACMan::GetInterface(const nsIID &iid, void **result)

hm... where are the notificationCallbacks set?

netwerk/base/src/nsPACMan.h
+private:
+  NS_DECL_NSISTREAMLOADEROBSERVER
+  NS_DECL_NSIINTERFACEREQUESTOR

should this also use private inheritance for these interfaces?


more later...
Comment 14 Christian :Biesinger (don't email me, ping me on IRC) 2005-03-13 15:29:46 PST
Comment on attachment 177098 [details] [diff] [review]
v1 patch

netwerk/base/src/nsProtocolProxyService.cpp
-	 if (NS_SUCCEEDED(rv) && (!reloadPAC || strcmp(tempString.get(),
mPACURI.get()))) 
+	 if (NS_SUCCEEDED(rv) && (reloadPAC || !tempString.Equals(mPACURI))) 

so the !reloadPAC condition was just wrong?


Hm... it seems to me that a proxyInfo's timeout is currently ignored. Shouldn't
DisableProxy use the proxyInfo's timeout, instead of always the global one?

+    link->next = nsnull;
+    link->position = position;
+    link->filter = filter;

maybe those should be constructor arguments to FilterLink?

+    for (FilterLink *iter = mFilters; iter; iter = iter->next, last = iter) {

Does that last part do the right thing? I don't know C++ enough to tell, but
wouldn't it make last equal to iter->next instead of iter?

+    PR_ASSERT(last);

did you choose this over NS_ASSERTION for a special reason?

+    if (2 == mUseProxy || 4 == mUseProxy) {

hm... some constants/#defines for these would be nice

+	 // remove any disabled proxies.

hm... the NS_RELEASE(iter) will set iter to nsnull, which means that the
enumeration will stop. so, at most one disabled proxy will be removed. that
sounds wrong...


Nice documentation in nsProtocolProxyService.h!


netwerk/base/src/nsProxyAutoConfig.js
 // ptr to eval'ed FindProxyForURL function
 var LocalFindProxyForURL = null;
 // sendbox in which we eval loaded autoconfig js file
 var ProxySandBox = null;

hm... I wonder why these are not member variables...
(and I notice "sendbox" is misspelled)


I think a testcase for NON_BLOCKING would be a good idea, too.
Comment 15 Darin Fisher 2005-03-13 16:12:53 PST
Thanks for all the good review feedback.  New patch and answers to your
questions coming up.
Comment 16 Darin Fisher 2005-03-14 12:16:54 PST
> OK... so an empty PAC uri makes the resolve methods return failure, and the
> protocol proxy service will warn (and try DIRECT). seems this will only happen
> if the user enters an empty url, so this seems ok. but, is this special-casing
> needed? couldn't you just let NewChannel fail?
> 
> actually... independent from that, if NewChannel fails, should you null out
> mPAC? if a user enters an invalid URL, I don't think we should reuse the
> previous pac file...

Yeah, you're on to something.  There are a couple ways that things could get
screwed up.  I spent some time and reworked the way LoadPACFromURI is handled. 
It's a shame that we have to deal with re-entrant GetService not being possible.


> I wonder if stream loaders should implement nsIRequest...

Well, there is that bug about making nsIStreamLoader be a nsIStreamListener ;-)


> +nsPACMan::GetInterface(const nsIID &iid, void **result)
> 
> hm... where are the notificationCallbacks set?

good catch!  the call to SetNotificationCallbacks got removed as I was reworking
some things.


> should this also use private inheritance for these interfaces?

nah, i want to expose nsISupports as public.
Comment 17 Darin Fisher 2005-03-14 14:49:14 PST
Created attachment 177427 [details] [diff] [review]
v1.1

revised patch per biesi's review comments.
Comment 18 Darin Fisher 2005-03-14 14:50:06 PST
Created attachment 177428 [details] [diff] [review]
interdiff between v1 and v1.1 patches
Comment 19 Darin Fisher 2005-03-15 12:09:04 PST
Created attachment 177522 [details] [diff] [review]
v1.2

whoops... that last patch was missing code to use the failover timeout stored
on the nsProxyInfo object :-(

This patch includes the one-line fix for that problem.
Comment 20 Darin Fisher 2005-03-15 12:10:52 PST
Created attachment 177523 [details] [diff] [review]
interdiff between v1 and v1.2 patches
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-03-15 12:48:56 PST
Should those contracts be going into nsEmbedCID.h instead?  Is nsNetCID.h public?

If it is, should we be putting classids there too?
Comment 22 Darin Fisher 2005-03-15 15:34:40 PST
No, I'm not interested in putting necko stuff in nsEmbedCID.h.  nsNetCID.h is
not frozen, but it is the current defacto place for these things.  I wouldn't
mind splitting it up into two header files at some point.

The reason for adding these default prompt contracts to necko is that necko
defines nsIPrompt and nsIAuthPrompt, and necko needs to use them.  it needs to
use this contract, but it doesn't know how to implement it.  it would be odd to
make necko depend on embedding.  that's why i didn't just code to
nsIWindowWatcher in the first place! ;-)
Comment 23 Darin Fisher 2005-03-15 16:21:30 PST
bug 264002 will be fixed with this patch
Comment 24 Christian :Biesinger (don't email me, ping me on IRC) 2005-03-15 16:32:18 PST
Comment on attachment 177522 [details] [diff] [review]
v1.2

netwerk/base/src/nsPACMan.cpp
+  nsCOMPtr<nsIIOService> ios = do_GetIOService();
+  if (ios) {
+    nsCOMPtr<nsIChannel> channel;
+    ios->NewChannel(mPACSpec, nsnull, nsnull, getter_AddRefs(channel));

nit: You could avoid one level of indentation by using NS_NewChannel here

netwerk/base/src/nsProtocolProxyService.cpp
+	     } else if (type > eProxyConfig_WPAD) {

nit: I wonder if a enum value like eProxyConfig_Last and >= comparison might be
better, in case a new type gets added at some point

(hm... are my reviews too nit-picky?)

I note that you are removing this comment:
-    // We diverge from the WPAD spec here in that we don't walk the hosts's
-    // FQDN, stripping components until we hit a TLD.	Doing so is dangerous
in
-    // the face of an incomplete list of TLDs, and TLDs get added over time. 
We
-    // could consider doing only a single substitution of the first component,
-    // if that proves to help compatibility.

Should you, instead, move it to where you AssignLiteral the wpad url?
Comment 25 Darin Fisher 2005-03-15 17:25:08 PST
> (hm... are my reviews too nit-picky?)

No, I appreciate your comments.


> Should you, instead, move it to where you AssignLiteral the wpad url?

yes, good idea.. i meant to do that, but forgot!
Comment 26 Darin Fisher 2005-03-15 17:36:09 PST
Not ideal to use NS_NewChannel since it expects a nsIURI, and I only have a
nsCString.  I could write a new version of NS_NewChannel that calls
nsIIOService::newChannel instead of newChannelFromURI, but I decided instead to
just use nsIIOService directly.
Comment 27 Christian :Biesinger (don't email me, ping me on IRC) 2005-03-18 06:58:28 PST
Comment on attachment 177522 [details] [diff] [review]
v1.2

so, one more thing:
nsNetCID.h
+#define NS_DEFAULTAUTHPROMPT_CID		      \

given that this is not implemented by necko, and that CIDs identify a certain
implementation, should a necko header really contain this CID?
Comment 28 Darin Fisher 2005-03-18 10:19:59 PST
That's a reasonable point.  I was just being consistent, but I could certainly
leave out the ClassIDs for those.  I've been thinking for a while now that I
should hide the ClassIDs for the other components as well.
Comment 29 Christian :Biesinger (don't email me, ping me on IRC) 2005-03-18 12:26:36 PST
(In reply to comment #28)
> I've been thinking for a while now that I
> should hide the ClassIDs for the other components as well.

sounds good. By the way, why is nsNetCID.h not in base/public? Being a public
necko header, it seems to me like it would belong there.

Comment 30 Darin Fisher 2005-03-19 12:38:01 PST
> sounds good. By the way, why is nsNetCID.h not in base/public? Being a public
> necko header, it seems to me like it would belong there.

I'm not sure it matters where it lives.  It is exported into dist/include/necko,
which is where all the other modules access it from.

Comment 31 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-03-22 21:33:25 PST
Comment on attachment 177522 [details] [diff] [review]
v1.2

>Index: netwerk/base/public/nsIProtocolProxyCallback.idl

>+   * @param aProxyInfo
>+   *        The resulting proxy info or null if there is no associated proxy
>+   *        info for aURI.  As with the result of examineForProxy, a null
>+   *        result implies that a direct connection should be used.

What's examineForProxy?  On which interface?  Doesn't this patch remove it on
the proxy service?  Fix this comment, please.

>Index: netwerk/base/public/nsIProtocolProxyService.idl
>+     * This is the default value for the aFlags parameter passed to resolve
>+     * and asyncResolve.

How is it the default if all callers have to pass it?  I'm not sure what this
comment is trying to say.

>      * NOTE: However, if the nsIProxyInfo type is "HTTP", then it means that

Isn't the type "http" (lowercase)?

>+     * from resolve/asyncResolve as well as from getFailoverProxy.
...
>     nsIProxyInfo getFailoverForProxy(in nsIProxyInfo aProxyInfo,

Make method name in comment match the code?

>+     * If two filters register for the same position, then the filters will be
>+     * visited in the order in which they were registered.

So I guess the use case for the position is so that if a given entity registers
more than one filter it can control their execution order, huh?

>Index: netwerk/base/public/nsNetUtil.h
>+                rv = pps->Resolve(uri, 0, proxyInfo);

nsIProtocolProxyService::RESOLVE_NORMAL, please.

>Index: netwerk/base/src/nsIOService.cpp
> nsIOService::NewChannelFromURI(nsIURI *aURI, nsIChannel 
>+        if (!mProxyService) {
>+            mProxyService = do_GetService(NS_PROTOCOLPROXYSERVICE_CONTRACTID);
>+            if (!mProxyService)
>+                NS_WARNING("failed to get protocol proxy service");

That should probably return whatever error do_GetService produces...  Since
we're doing this lazily, we could perfectly well fail to get the service (eg
OOM).

>+        if (mProxyService) {

And do we really want to keep going here if there is no proxy service?

>Index: netwerk/base/src/nsPACMan.cpp
>Index: netwerk/base/src/nsPACMan.h
>+class NS_NO_VTABLE nsPACManCallback : public nsISupports

Why NO_VTABLE?	Don't we need one for the nsISupports stuff?

I have to admit to mostly skimming things from here to nsNetCID.h...  They look
ok, but I didn't do a careful line-by-line review of all the PAC stuff, etc...

>Index: netwerk/build/nsNetCID.h

Please don't put classnames and CIDs for components not implemented by Necko in
this file.  Put them in the headers of the relevant concrete classes if the
module that implements them needs these defines.

I also completely skipped all the test code.

>Index: modules/plugin/base/src/nsPluginHostImpl.cpp

>+  res = proxyService->Resolve(uriIn, 0, getter_AddRefs(pi));

Use the named constant here, instead of just 0.

>Index: browser/components/preferences/connection.js
>Index: xpfe/components/prefwindow/resources/content/pref-proxies.js
>Index: mail/components/preferences/content/connection.js

I'm deeply unhappy about the use of nsPIProtocolProxyService here.  Unhappy
enough to mark sr- on this patch, in fact, though you're not really introducing
the problem, just modifying it slightly.

If this functionality is needed by our prefs UI, it's also needed by embeddors,
and should be exposed on an interface we plan to freeze.
nsIProtocolProxyService sounds like a good place; I don't quite see why it
wasn't just placed there...

>Index: mailnews/base/util/nsMsgProtocol.cpp
>+          rv = pps->Resolve(proxyUri, 0, getter_AddRefs(proxyInfo));

Please use the named constant, not 0.
Comment 32 Darin Fisher 2005-03-22 23:18:17 PST
Boris, thank you for taking the time to review this patch.  Now, let me try to
answer some of your questions and explain my reasoning for various decisions. 
If I skip something you mentioned, then assume that I've applied the change.


> >Index: netwerk/base/public/nsIProtocolProxyService.idl
> >+     * This is the default value for the aFlags parameter passed to resolve
> >+     * and asyncResolve.
> 
> How is it the default if all callers have to pass it?  I'm not sure what this
> comment is trying to say.

Yes, that could be worded better.  The aFlags parameter is used to modify the
"resolve" call.  RESOLVE_NORMAL is a placeholder which means "no modifications"
or "do the default thing".  I would almost rather remove RESOLVE_NORMAL and just
document that callers should pass 0 to specify no options.  I prefer that to the
more verbose RESOLVE_NORMAL, as can be seen by the callsites that never make use
of RESOLVE_NORMAL.


> >+     * If two filters register for the same position, then the filters will be
> >+     * visited in the order in which they were registered.
> 
> So I guess the use case for the position is so that if a given entity registers
> more than one filter it can control their execution order, huh?

No, the idea is that this gives distinct entities the ability to apply proxy
filtering with some kind of relative priority.  Obviously, there is no good way
for distinct entities to agree on what values constitute "first" and "second"
position in the filter list.  That's a problem, so if you have any suggestions
about how to deal with that problem, I'd love to hear them.  The use case is:
one extension wishes to make a proxy be used in place of direct whenever the
user preferences select default, and another extension wishes to prevent
proxying for certain URLs.  The first extension might decide that it is not a
big deal if it is overriden, so it may select a low position value.  The second
extension on the other hand may select a higher position value to "ensure" that
it gets to trump other filters.  Yes, this has obvious problems, but it was the
best I could come up with that would allow more than one extension to play with
proxy filters.  Maybe an extension could be created to provide UI to order proxy
filters if such a problem ever really came up.


> >Index: netwerk/base/src/nsIOService.cpp
> > nsIOService::NewChannelFromURI(nsIURI *aURI, nsIChannel 
> >+        if (!mProxyService) {
> >+            mProxyService = do_GetService(NS_PROTOCOLPROXYSERVICE_CONTRACTID);
> >+            if (!mProxyService)
> >+                NS_WARNING("failed to get protocol proxy service");
> 
> That should probably return whatever error do_GetService produces...  Since
> we're doing this lazily, we could perfectly well fail to get the service (eg
> OOM).

Hmm... my feeling has always been that the proxy service is not a required
service.  If it does not exist, then we can carry on, so why not?  I prefer to
build Necko so that it degrades gracefully when various component services
cannot be accessed (e.g., failure to access the pref services is not a fatal
error in most parts of necko).


> >+class NS_NO_VTABLE nsPACManCallback : public nsISupports
> 
> Why NO_VTABLE?	Don't we need one for the nsISupports stuff?
> 
> I have to admit to mostly skimming things from here to nsNetCID.h...  They look
> ok, but I didn't do a careful line-by-line review of all the PAC stuff, etc...

Please read the comments in nscore.h for NS_NO_VTABLE.  You might also take a
look at the header files generated by xpidl.exe ;-)


> >Index: netwerk/build/nsNetCID.h
> 
> Please don't put classnames and CIDs for components not implemented by Necko in
> this file.  Put them in the headers of the relevant concrete classes if the
> module that implements them needs these defines.

Please see the comment #22 where I already answered this.  It is correct for the
ContractIDs to be defined by Necko.  Necko defines the contract of a "default"
nsIPrompt/nsIAuthPrompt implementation.  It is an implementation detail that it
does not provide the implementation of that contract.  Moreover, Necko is a
consumer of these default prompts, and it should not have a REQUIRES dependency
on embedding.  Otherwise, why not just use windowwatcher directly?


> >Index: browser/components/preferences/connection.js
> >Index: xpfe/components/prefwindow/resources/content/pref-proxies.js
> >Index: mail/components/preferences/content/connection.js
> 
> I'm deeply unhappy about the use of nsPIProtocolProxyService here.  Unhappy
> enough to mark sr- on this patch, in fact, though you're not really introducing
> the problem, just modifying it slightly.
> 
> If this functionality is needed by our prefs UI, it's also needed by embeddors,
> and should be exposed on an interface we plan to freeze.
> nsIProtocolProxyService sounds like a good place; I don't quite see why it
> wasn't just placed there...

OK, take a moment to study the behavior of ConfigureFromPAC.  Notice that it
changes the value of mPACURI for nsProtocolProxyService in a manner that takes
it out of sync with the preferences settings.  It is possible in fact to go into
prefs, change the value of the PAC URL, press reload, and then cancel out of the
prefs dialog.  As a result, the PAC file would be loaded and it would not
necessarily correspond to the PAC URL specified in the preferences.  That's a
bug.  It's also a bug that this functionality is needed.

It should be enough to toggle necko offline/online to cause the PAC file to be
reloaded.  PAC should maybe also refresh periodically, and we should certainly
reload the PAC file whenever the OS changes its network settings.

In short, ConfigureFromPAC should be considered dangerous.  It is definitely not
something embedders should be playing with.  They can achieve the very same
effect by toggling preferences.  That would almost be a better solution for our
pref panel as well (since our pref panel could be smart about rolling back pref
changes on cancel).

So, I am strongly against putting ConfigureFromPAC on nsIProtocolProxyService. 
By the way, what about ConfigureFromWPAD?  How does one refresh the WPAD PAC
file in our prefs?  Oh, our pref panel for proxy selection is so broken :-)
Comment 33 Darin Fisher 2005-03-22 23:22:59 PST
Boris: please see my responses to your review comments.  I will post a revised
patch tomorrow.
Comment 34 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-03-22 23:42:57 PST
> That's a problem, so if you have any suggestions
> about how to deal with that problem, I'd love to hear them.

No good ideas here (esp. if two extensions both want to be the "last one" to
look at the list -- then we have a race).  Perhaps at least indicate in the IDL
what values of the position correspond to "low priority", "medium priority",
"high priority" or something?  Or would those just be 0, PR_UINT32_MAX/2,
PR_UINT32_MAX?

> Please read the comments in nscore.h for NS_NO_VTABLE. 

I did that as part of my review.  I also read the bug they reference.  I'm still
not following -- the comments all refer to pure virtual classes, etc.  What lets
you use this on a concrete class?  The fact that there is no constructor?  If
so, please document that; that's not clear from the nscore comments or the bug
that added NS_NO_VTABLE.

> It is correct for the ContractIDs to be defined by Necko.

Agreed.  But it's not correct for the classnames and classids to be defined by
Necko, which is what my comment was about.  Necko does not need these, and
neither does anyone else except the code that actually registers the components.
  Hence they belong in the module that registers for the relevant contract.

> since our pref panel could be smart about rolling back pref changes on cancel

Erm... It IS smart about that.  Or rather the preferences dialog is plenty smart
about it and provides hooks to actually only change pref values when "OK" is
pressed.

Sounds like we just need to file a bug on the pref panel to stop using this
method and just toggle the preference setting, eh?  And then once that's fixed
remove this api altogether, perhaps?  In the meantime, I'd like to see some
clear comments on the nsPIProtocolProxyService api saying that it's use is
considered harmful.
Comment 35 Darin Fisher 2005-03-23 01:14:19 PST
> No good ideas here (esp. if two extensions both want to be the "last one" to
> look at the list -- then we have a race).  Perhaps at least indicate in the IDL
> what values of the position correspond to "low priority", "medium priority",
> "high priority" or something?  Or would those just be 0, PR_UINT32_MAX/2,
> PR_UINT32_MAX?

Yes, I'll indicate this in the IDL.


> > Please read the comments in nscore.h for NS_NO_VTABLE. 
> 
> I did that as part of my review.  I also read the bug they reference.  I'm still
> not following -- the comments all refer to pure virtual classes, etc.  What lets
> you use this on a concrete class?  The fact that there is no constructor?  If
> so, please document that; that's not clear from the nscore comments or the bug
> that added NS_NO_VTABLE.

From the MSDN link in comment #0 of bug 49416:

"This form of _declspec can be applied to any class declaration, but should only
be applied to pure interface classes, that is classes that will never be
instantiated on their own. The _declspec stops the compiler from generating code
to initialize the vfptr in the constructor(s) and destructor of the class. In
many cases, this removes the only references to the vtable that are associated
with the class and, thus, the linker will remove it. Using this form of
_declspec can result in a significant reduction in code size."

If you'll notice, nsPACManCallback only defines an additional pure virtual
method, so nsPACManCallback is just like any other XPIDL generated interface, so
marking it with NS_NO_VTABLE seems appropriate to me.


> > It is correct for the ContractIDs to be defined by Necko.
> 
> Agreed.  But it's not correct for the classnames and classids to be defined by
> Necko, which is what my comment was about.

Oh, right.  I read your comment too quickly.  Please see my earlier comment #28
(in response to biesi), in which I said that I intend to remove the classnames
and classids for these from the header file.  I'm sorry, I should have posted a
revised patch before asking you to review.


> Sounds like we just need to file a bug on the pref panel to stop using this
> method and just toggle the preference setting, eh?  And then once that's fixed
> remove this api altogether, perhaps?

Yes, indeed.  Part of my plan is to do this work in stages.  There are many
small steps to be taken after this patch lands.  For example, I'd like to hook
up HTTP to actually take advantage of the asyncResolve method.  Removing
configureFromPAC and nsPIProtocolProxyService is another next step.


> In the meantime, I'd like to see some
> clear comments on the nsPIProtocolProxyService api saying that it's use is
> considered harmful.

Bah... it is a private interface.  That's what the "nsPI" means.  But, sure... I
can add such a warning fwiw.
Comment 36 neil@parkwaycc.co.uk 2005-03-23 03:56:18 PST
So what you're saying is that any change to proxy preferences will reload the
PAC (assuming that PAC is now the current preference) thus the only use of the
reload PAC button was to reload the PAC without doing anything else?
Comment 37 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-03-23 08:39:48 PST
> Bah... it is a private interface. 

Yes, but it's a deprecated private interface (unlike private interfaces we have
that are really for use by our internal gecko code).
Comment 38 Darin Fisher 2005-03-23 10:41:01 PST
> So what you're saying is that any change to proxy preferences will reload the
> PAC (assuming that PAC is now the current preference) thus the only use of the
> reload PAC button was to reload the PAC without doing anything else?

Well, for example, ConfigureFromPAC could be achieved by toggling the value of
network.proxy.autoconfig_url from empty to the desired URL.  That would cause
the PAC file to be reloaded in almost exactly the same way as a direct call to
ConfigureFromPAC.  NOTE: it is important to roll-back any such pref changes when
the pref dialog is canceled.
Comment 39 Darin Fisher 2005-03-23 10:49:18 PST
>>      * NOTE: However, if the nsIProxyInfo type is "HTTP", then it means that
>
>Isn't the type "http" (lowercase)?

BTW, the documentation for the newProxyInfo method clearly states that the proxy
type is a case-insensitive string.  I will still change this to be consistent
with the convention used elsewhere of referring to proxy types in their
lowercased form.
Comment 40 Darin Fisher 2005-03-23 17:21:49 PST
Created attachment 178419 [details] [diff] [review]
v1.3
Comment 41 Darin Fisher 2005-03-23 17:25:48 PST
Created attachment 178423 [details] [diff] [review]
interdiff between v1.2 and v1.3 patches

bz: please take a look at this file instead to see what changed.
Comment 42 Christian :Biesinger (don't email me, ping me on IRC) 2005-03-23 17:50:25 PST
oh... I meant to comment on cancelAsyncResolve

So, nsIDNSRequest has the advantage that it's not just an opaque cookie, and
that you can cancel it without needing other interfaces (without knowing where
it came from, maybe).
Clearly cancelAsyncResolve has the advantage that it avoids an interface. It
slightly bloats the nsIProtocolProxyService iface...

(random thought: Should PAC queries and the DNS queries implement nsIRequest?
also avoids a new interface ;-) somewhat heavyweight, probably. mmmm... dns
requests in a loadgroup...)
Comment 43 Darin Fisher 2005-03-23 18:06:46 PST
> (random thought: Should PAC queries and the DNS queries implement nsIRequest?
> also avoids a new interface ;-) somewhat heavyweight, probably. mmmm... dns
> requests in a loadgroup...)

I thought about that, and in fact it used to be that way.  But, nsIRequest
doesn't really fit well in that context.  Take a look at how nsIRequest is used
today:

 1- instantiate an object that implements nsIRequest
 2- set some request attributes
 3- call some method (e.g., nsIChannel::asyncOpen) to actually start the request

Now, constrast that the these async operations:

 1- call some method that requests a "request" object.

In the latter case, the only chance to do the configuring of the request is via
the method that is used to both instantiate and start the request.  That doesn't
mesh well with the host of options on nsIRequest.

NOTE: it also doesn't work for loadgroups because the behavior of loadgroups is
that the request is added to the loadgroup if starting the async request
succeeds.  In order for that to work here, we'd have to pass a nsILoadGroup to
asyncResolve.  This is what we do for imgILoader::loadImage, and as a result
that method has a bazillion parameters :-(

If we want to return a nsIRequest then we should have separate methods to
intantiate the nsIRequest and start the nsIRequest.  This is how
imgILoader::loadImage (IMO) should be re-cast.  DNS and Proxy resolution are
slightly different beasts (far simpler), and the API would be somewhat
cumbersome if consumers had to allocate some object, set some attributes on it,
and then call a final method to start the request.

So, I'm not sure where I want to take this, but I think we should use the same
solution for async proxy resolution and async DNS resolution.

Biesi raised exactly this issue earlier on, and I mentioned that I'm leaning
toward eliminating nsIDNSReqeust in favor of a cancel method on nsIDNSService.

NOTE: my intentions are to make changes to nsIDNSService (removing init and
shutdown methods at least) for Gecko 1.8.
Comment 44 Darin Fisher 2005-03-23 18:08:21 PST
Man, I should have proof-read that better.  When I wrote

>  1- call some method that requests a "request" object.

I really meant:

 1- call some method that _returns_ a "request" object.
Comment 45 Darin Fisher 2005-03-23 18:10:30 PST
Another tid-bit: nsIRequest has suspend and resume methods that would be tricky
to implement for DNS and proxy queries.
Comment 46 Christian :Biesinger (don't email me, ping me on IRC) 2005-03-23 18:55:07 PST
(In reply to comment #43)
> Biesi raised exactly this issue earlier on, and I mentioned that I'm leaning
> toward eliminating nsIDNSReqeust in favor of a cancel method on nsIDNSService.

Yes; what I said was kinda in response to:
(In reply to comment #12)
> BTW, biesi: I was thinking of introducing nsIDNSService::cancelAsyncResolve
> as well since it eliminates an interface.  Thoughts?
Comment 47 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-03-24 17:40:57 PST
Comment on attachment 178419 [details] [diff] [review]
v1.3

>Index: netwerk/base/public/nsIProtocolProxyCallback.idl
>+   *        info for aURI.  As with the result of nsIProtocolProxyService::
>+   *        resolve, a null result implies that a direct connection 

It's a little weird to have a linebreak in the middle of the method name like
that, but either way.

sr=bzbarsky
Comment 48 Darin Fisher 2005-03-24 19:24:02 PST
I changed the text to:

   *        The resulting proxy info or null if there is no associated proxy
   *        info for aURI.  As with the result of nsIProtocolProxyService's
   *        resolve method, a null result implies that a direct connection
   *        should be used.
Comment 49 Darin Fisher 2005-03-24 19:42:46 PST
fixed-on-trunk
Comment 50 Darin Fisher 2005-03-24 19:54:07 PST
I just filed bug 287646 and bug 287648 as followups to this bug.
Comment 51 Christian :Biesinger (don't email me, ping me on IRC) 2005-03-28 04:26:43 PST
this caused bug 287956
Comment 52 Ian Thomas ('thelem') 2005-03-29 12:43:32 PST
Is this really such a good idea in the current hostile climate that we have?

I can see two potential risks, which I'll admit exist with the current
implementations but are harder.

1. Somebody changes your proxy without your knowledge, adding adverts to all
pages, preventing access to pages they don't want you to see and replacing
server or file not found errors with their own advertising covered pages or
search engines. A similar thing was attempted by Verisign (I think) a couple of
years ago, when all .com and .net addresses that had not been registered would
point to a page of their advertising. It is also commonly seen on domains that
have recently been purchased or recently expired (and been purchased by a
different organisation).

2. Phishers setup a proxy without your knowledge and route all your traffic
normally, EXCEPT requests to bank logon screen etc which they will serve from
their own pages. This way, even typing the bank's url into the address bar would
not protect you.
Comment 53 Darin Fisher 2005-03-29 13:07:20 PST
Ian,

If an attacker can install software on your machine then all bets are off.  This
API is no more risky than XPCOM itself.  Are you suggesting that Mozilla should
stop using XPCOM?

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