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.

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.

BTW, this is something I'd like to finalize (and freeze) for 1.8.

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.

> 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!

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).

(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?

> 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.

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.

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 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

I'm posting a new patch. Please don't bother reading further until I do.

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 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 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.

Thanks for all the good review feedback. New patch and answers to your questions coming up.

> 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.

revised patch per biesi's review comments.

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.

Should those contracts be going into nsEmbedCID.h instead? Is nsNetCID.h public? If it is, should we be putting classids there too?

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! ;-)

bug 264002 will be fixed with this patch

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?

> (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!

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 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?

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.

(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.

> 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 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.

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 :-)

Boris: please see my responses to your review comments. I will post a revised patch tomorrow.

> 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.

> 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.

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?

> 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).

> 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.

>> * 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.

bz: please take a look at this file instead to see what changed.

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...)

> (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.

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.

Another tid-bit: nsIRequest has suspend and resume methods that would be tricky to implement for DNS and proxy queries.

(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 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

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.

fixed-on-trunk

I just filed bug 287646 and bug 287648 as followups to this bug.

this caused bug 287956

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.

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?