Closed Bug 53080 Opened 24 years ago Closed 23 years ago

[CLOSED]PAC: did not work pre-Mozilla 0.9 (-> go to 79893)

Categories

(Core :: Networking, defect, P2)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: rich.burridge, Assigned: attinasi)

References

Details

(Whiteboard: attach #30744)

Attachments

(34 files)

1.58 KB, patch
Details | Diff | Splinter Review
671 bytes, patch
Details | Diff | Splinter Review
782 bytes, patch
Details | Diff | Splinter Review
1.54 KB, patch
Details | Diff | Splinter Review
14.55 KB, text/plain
Details
6.70 KB, patch
Details | Diff | Splinter Review
7.25 KB, patch
Details | Diff | Splinter Review
9.80 KB, patch
Details | Diff | Splinter Review
11.67 KB, patch
Details | Diff | Splinter Review
3.61 KB, text/plain
Details
7.94 KB, patch
Details | Diff | Splinter Review
8.46 KB, patch
Details | Diff | Splinter Review
5.01 KB, patch
Details | Diff | Splinter Review
11.36 KB, patch
Details | Diff | Splinter Review
3.27 KB, patch
Details | Diff | Splinter Review
3.79 KB, patch
Details | Diff | Splinter Review
3.63 KB, patch
Details | Diff | Splinter Review
11.68 KB, patch
Details | Diff | Splinter Review
18.70 KB, patch
Details | Diff | Splinter Review
19.30 KB, patch
Details | Diff | Splinter Review
318 bytes, text/plain
Details
22.95 KB, patch
Details | Diff | Splinter Review
25.99 KB, patch
Details | Diff | Splinter Review
25.68 KB, patch
Details | Diff | Splinter Review
25.67 KB, patch
Details | Diff | Splinter Review
25.67 KB, patch
Details | Diff | Splinter Review
25.67 KB, patch
Details | Diff | Splinter Review
25.30 KB, patch
Details | Diff | Splinter Review
2.95 KB, patch
Details | Diff | Splinter Review
4.06 KB, patch
Details | Diff | Splinter Review
4.39 KB, patch
Details | Diff | Splinter Review
682 bytes, patch
Details | Diff | Splinter Review
1.71 KB, patch
Details | Diff | Splinter Review
1.12 KB, patch
Details | Diff | Splinter Review
Cannot specify a "file:/" URL for the PAC configuration file. Currently the 
only way PAC works is if you copy over the contents of that PAC file to the
nsProxyAutoConfig.js file.

For successfully deployment of Netscape 6 internally here at Sun we need to
be able to correctly specify Automatic Proxy Configuration URLs of the type:

file:/usr/dist/share/netscape,v6.0/proxy_config/<domain>.pac

where <domain> can be one of sixty different domains.
Changing the priority of the bug to P2, so it has a chance of being
considered for PR3/RTM. Akhil, could you accept this bug, and add you
comments when ready please.
Priority: P3 → P2
Akhil: If you do plan to implement this, then here are the list of things that 
need to be done--

1. register for pref change in nsProtocolProxyService.cpp (for the pac.url pref)
2. on receiving a change, download that file (could be an http, ftp, file url) 
This code would be absolutely ignorant of the scheme type so it would work with 
all cases. 
3. patch this file with the nsProxyAutoConfig.js file (basically remove the 
sample and using that as a template append the downloaded file from step 2)
4. place that in the dist/components/ directory. Unregister and reregister the 
nsProxyAutoConfig service. 
5. Use it--It should all work! 

Let me know if you have any questions about this. Rich: thanks for filing this. 
I changed the summary to reflect the overall problem. 
Summary: Cannot specify "file:/" style URL's for Proxy Automatic Configuration (PAC) files. → Cannot specify any URL for Proxy Automatic Configuration (PAC) files.
the autoproxy stuff is currently broken. before i can tackle this bug, i need to
get autoproxy working again. here are the changes needed (debugged by Igor
Kushnirsky - idk@eng.sun.com). tested to work on solaris and linux, tested to
work with single and multiple PROXY hosts specified.

in mozilla/netwerk,
M base/src/nsProxyAutoConfig.js
M dns/src/nsDnsService.cpp

Index: nsDnsService.cpp
===================================================================
RCS file: /cvsroot/mozilla/netwerk/dns/src/nsDnsService.cpp,v
retrieving revision 1.66
diff -u -r1.66 nsDnsService.cpp
--- nsDnsService.cpp    2000/08/21 08:23:40     1.66
+++ nsDnsService.cpp    2000/09/20 21:29:18
@@ -1232,7 +1232,8 @@
                 if (NS_SUCCEEDED(Resolve(hostname, &mMyIPAddress)))
                 {
                     CRTFREEIF(hostname);
-                    return NS_OK;
+                    *o_ip = nsCRT::strdup(mMyIPAddress);
+                    return *o_ip ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
                 }
                 else
                 {

Index: nsProxyAutoConfig.js
===================================================================
RCS file: /cvsroot/mozilla/netwerk/base/src/nsProxyAutoConfig.js,v
retrieving revision 1.6
diff -u -r1.6 nsProxyAutoConfig.js
--- nsProxyAutoConfig.js        2000/09/13 23:54:11     1.6
+++ nsProxyAutoConfig.js        2000/09/20 21:37:40
@@ -31,7 +31,7 @@
 const kPAC_CID = Components.ID("{63ac8c66-1dd2-11b2-b070-84d00d3eaece}");
 const nsIProxyAutoConfig = Components.interfaces.nsIProxyAutoConfig;
 const nsIIOService = Components.interfaces['nsIIOService'];
-const nsIDNSService =
Components.interfaces['@mozilla.org/js/xpc/ID;1NSService'];
+const nsIDNSService = Components.interfaces.nsIDNSService;

 function debug(msg)
 {
@@ -44,7 +44,7 @@
 nsProxyAutoConfig.prototype = {

     ProxyForURL: function(url, host, port, type) {
-        uri = url.QueryInterface(Components.interfaces.nsIURI);
+        var uri = url.QueryInterface(Components.interfaces.nsIURI);
         // Call the original function-
         var proxy = FindProxyForURL(uri.spec, uri.host);
         debug("Proxy = " + proxy);
@@ -58,8 +58,8 @@
             // we ignore everything else past the first proxy.
             // we could theoretically check isResolvable now and continue
             // parsing. but for now...
-            var re = /PROXY (.+):(\d+);.*/;
-            hostport = proxy.match(re);
+            var re = /PROXY (.+):(\d+)/;
+            var hostport = proxy.split(';')[0].match(re);
             host.value = hostport[1];
             port.value = hostport[2];
             type.value = "http"; //proxy (http, socks, direct, etc)


next, are there any bits of code i can steal to see how a uri can be downloaded
and nsProxyAutoConfig can be unregistered/re-registered?
Status: NEW → ASSIGNED
Akhil: good catches! But I have a better fix for the nsDnsService. Just remove 
the return NS_OK line. It should fall thru and do the right thing. With that 
change r=gagan

As for your next step... just look around there should be plenty of sample 
cases in the code base. 
i guess i should explain my latest diff

-            b[i] = atoi(p) && 0xff;
+            b[i] = atoi(p) & 0xff;

the boolean && operator was mistakenly used instead of the bitwise & operator.
because of this, nsDnsService::IsInNet was succeeding almost always causing the
first proxy in the pac to be selected everytime.
r=gagan
Nominating this bug for inclusion into beta3. Needed if pr3 is going to be 
deployed internally at Sun (and at other sites which use PACs).
Keywords: nsbeta3
approving for beta3.
Whiteboard: [nsbeta3+]
-            var re = /PROXY (.+):(\d+);.*/;
-            hostport = proxy.match(re);
+            var re = /PROXY (.+):(\d+)/;
+            var hostport = proxy.split(';')[0].match(re);

The var fixes are good, but why call split?  How about this instead:

            var hostport = /^PROXY (.+):(\d+)/(proxy);

It's faster, shorter, sweeter, etc.

/be
unfortunately the faster, shorter, sweeter fix does not work. for example, from 

PROXY proxy-cup.Eng.Sun.COM:800;PROXY proxy-mpk.Eng.Sun.COM:800

we want 

proxy-cup.Eng.Sun.COM:800

i am not a js expert, but my original fix does work. may i check in my original
fix?
Oh, I forgot about the extra stuff after the ;.  The problem is that .+ is 
greedy, it will swallow every char except \n.  Try this:

            var hostport = /^PROXY ([^:]+):(\d+)/(proxy);

You can't have a : in the proxy hostname, but that's a non-starter anyway.

/be
I think it's worth getting the JS right (even if it takes me one stab) rather 
than hacking in gratuitous split calls.  There's no absolute deadline, right? 
(We already missed the branch, so this will have to go in trunk and branch in 
any event.)

/be
Hi Brendan: no, there's no absolute deadline, except that we would very much
like this to make PR3 as it's a show-stopper for Sun internal use.  Sun intends
to make Netscape 6 its default internal browser as soon as possible, but we are
a global company behind a firewall with many internal domains.  Auto proxy
configuration is the only way we can realistically set up proxies for all 37,000
Sun employees.  So the difference between getting an approval today and getting
one on Monday is no big deal, as long as you're still willing to approve this
fix.  The risk, from our standpoint, is that each day that passes makes it
harder and harder to get approval for a fix (since risk assessment tightens each
day).  That's why we're working for approval now.

If you'll still approve on Monday after Akhil has verified your
brand-spanking-new fix, that's cool.

I hope this explains Akhil's rationale, hope this is acceptable.
*** Bug 48584 has been marked as a duplicate of this bug. ***
Due to criticality of deployment at Sun, it is reasonable to call this a P2, and
I'm marking it PDTP2
Keywords: rtm
Whiteboard: [nsbeta3+] → [nsbeta3+][PDTP2]
Can I have her spam instead of the baked beans, then?
Fix in hand looks trivial and we want to help Sun deploy. nsbeta3++, but go 
quickly, since time is short. 
Whiteboard: [nsbeta3+][PDTP2] → [nsbeta3++][PDTP2]
Checked in phase 1 of the fix, the instructions at
http://www.mozilla.org/docs/netlib/pac.html will now work. This fix will allow
us to use auto proxies by concatenating all of our existing PAC files into one
giant PAC and deploying with this giant PAC pre-installed in the components
directory. Not pretty, but it will work.

starting work on phase 2, downloadable PAC, but this most likely would not make
it into pr3. thanks everyone.
Marking nsbeta3- since the remaining part won't make (and isn't needed for) PR3.
Whiteboard: [nsbeta3++][PDTP2] → [nsbeta3-][PDTP2]
Akhil, did you check this into the Netscape_20000922 branch and the trunk?  
Only things checked into the branch will get into PR3 or RTM.
i checked in the changes to the branch as well as the trunk. sorry, i should 
have clarified.
added cc edburns.
I'm sorry to say that I followed the steps in the mozilla.org proxy document, 
using Sun's eng.pac file, but I was still not able to get outside the firewall.

I tried paring down the nsProxyAutoConfig.js so the only appended text was:


function FindProxyForURL(url, host) {

    return "PROXY webcache-cup.Eng.Sun.COM:8080;" +
    "PROXY webcache-mpk-01.Eng.Sun.COM:8080";
}

And restarted the browser, but this still didn't work.
Jim, I just got this from Akhil.  After following Akhil's instructions,
I verify that proxy auto config works for the browser.  Haven't tested
for the plugin.

I've attached an nsProxyAutoConfig.js for eng.sun.com here
<http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16045>.  You
need to make this your mozilla/dist/bin/components/nsProxyAutoConfig.js
file.

Here's Akhil's additional instructions:

On 2 October 18:32:27, Akhil Arora wrote:
> I bet you are forgetting to set the user_pref("network.proxy.type", 2);
> preference... this is set when you select the Automatic proxy
> configuration URL: radio button in Advanced->Proxies. It does not matter
> what you type into that URL, that radio should be selected.
> 
Blocks: 55076
Whiteboard: [nsbeta3-][PDTP2] → [nsbeta3-][PDTP2] suntrak-n6-highp
Whiteboard: [nsbeta3-][PDTP2] suntrak-n6-highp → [nsbeta3-][PDTP2] suntrak-n6
For an additional example that still won't work, try setting automatic
proxy configuration, with url http://proxy.library.upenn.edu/pennproxy.pac
A few hundred (or thousand) Penn students and faculty use this.



www.infonet.ee/proxy.pac ilso does not work (www.infonet.ee users must use it to 
access the web outside Estonia).
will fixing this bug also include hooking up the prefs dialog under 
advanced>proxies?
or should i file a new bug to get this finaly working?
In answer to the last question about whether a new bug is required,
I think the answer is yes.  (I thought this was the same bug.)  Widespread
use of this browser at universities will not happen unless automatic
proxy configuration works as it does in Netscape 4.  This is the natural
way to make library resources available to off-campus users.  Jon
No, a separate bug is not required, if you apply my interim phase2 patch of 
10/5, a PAC URL that is typed into the auto proxy setting in the prefs dialog 
gets downloaded and saved to disk. Unfortunately, I am getting pulled into other 
higher-priority bugs since we have a workaround for this one. If anyone else 
wants to help, please start from my patch of 10/5.
*** Bug 55933 has been marked as a duplicate of this bug. ***
*** Bug 55664 has been marked as a duplicate of this bug. ***
*** Bug 47993 has been marked as a duplicate of this bug. ***
Is it true that automatic proxy configuration does not work and the manual
configuration works fine in the edit|preferences?
I could not do anything when I setup auto-configuration, but manual config works
fine for me.
If PAC URLs that are HTTP work, then the sun solution should be to find an 
internal webserver and distribute via a web server rather than NFS.

Is this possible for Sun? (only worked on sun campuses briefly).

Didn't even know anyone used "file" URLs for PAC...
BenC: we have a read-only problem with the current Mozilla/Netscape 6 
and PAC files at the moment. See bug 57134 which I've just opened.
The patch I just attached works for me on win32. It downloads PACs from http and
file URLs. To make this work, edit netwerk/base/src/nsProxyAutoConfig.js, delete
everything after the line that says "replace everything below with your existing
pac file" and copy the file to dist/bin as pac.template.js. Type in the PAC URL
in the Mozilla preferences and make sure that the Auto Proxy radio button is
selected. Restart the browser, it should work. pac.template.js is copied from
dist/bin to dist/bin/components as pac.js, the PAC URL is downloaded and
appended to this file and the component reloaded automatically. In case of
errors, examine pac.js in dist/bin/components.

I am having trouble with the code on Linux and Solairs. It goes into an endless
loop on the NewURI call in the LoadPAC function. Looks like some threading issue
within necko... if it works on win32 why not on linux and solaris? Gagan, need
your help!
Adding [rtm need info] to the status since you're working on a patch.

This bug should get minus though, we're not focusing on enterprise features and 
this patch is large.  Without some fast talking and a much smaller patch, the 
approval for this is gonna be pretty difficult.
Whiteboard: [nsbeta3-][PDTP2] suntrak-n6 → [nsbeta3-][PDTP2] suntrak-n6[rtm need info]
Whiteboard: [nsbeta3-][PDTP2] suntrak-n6[rtm need info] → [nsbeta3-][PDTP2] suntrak-n6-highp [rtm need info]
Adding keywords, nominating this bug for ns601 and mozilla0.9.
Keywords: mozilla0.9, ns601
Blocks: 41072
This causes loop becouse when crating http service, it needs to http
service to load pac-file, and it creates http service, which needs...

Loop stack looks like this:

LoadPAC
nsProtocolProxyService::PrefsChanged
nsProtocolProxyService::Init
nsProtocolProxyService::Create  
nsGenericFactory::CreateInstance
nsComponentManagerImpl::CreateInstance
nsComponentManager::CreateInstance
nsServiceManagerImpl::GetService
nsServiceManager::GetService 
nsGetServiceByCID::operator()
nsCOMPtr<nsIProtocolProxyService>::assign_from_helper
nsCOMPtr<nsIProtocolProxyService>::operator=
nsHTTPHandler::Init
nsHTTPHandlerConstructor
nsGenericFactory::CreateInstance
nsComponentManagerImpl::CreateInstance
nsComponentManager::CreateInstance
nsServiceManagerImpl::GetService
nsServiceManagerImpl::GetService
nsServiceManager::GetService   
nsIOService::GetProtocolHandler
nsIOService::NewURI
nsIOService::NewURI
LoadPAC
nsProtocolProxyService::PrefsChanged
nsProtocolProxyService::Init    
nsProtocolProxyService::Create  
nsGenericFactory::CreateInstance
.
.
.
Yes, that is what I am seeing on Linux and Solaris. The wierd thing is that this
does not happen on Windows. The fix should be to temporarily disable the use of
proxies until the PAC is downloaded, experimenting...
How about combining do_CreateInstance(NS_PROXY_AUTO_CONFIG_CONTRACTID, &rv)
and loading of pac-file?

At
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsProtocolProxyService.cpp#258
there is code:
  
       // later on put this in a member variable TODO
       nsCOMPtr<nsIProxyAutoConfig> pac = 
          do_CreateInstance(NS_PROXY_AUTO_CONFIG_CONTRACTID, &rv);

How about making that |nsCOMPtr<nsIProxyAutoConfig> pac| member variable,
initialized to NULL, and change that code to something like:

if(pac == NULL)  
   LoadPAC();

And add this to LoadPAC() or PACConsumer::OnStopRequest()?
pac = do_CreateInstance(NS_PROXY_AUTO_CONFIG_CONTRACTID, &rv);


This should fix that loop, and fix that performance hit by not calling
do_CreateInstance every time (does it really create that service
for every url loaded?).
Here is patch that sort of works on solaris. It loads pac-file ok,
but it writes it *over* js-template, it should append it. How do i do that?
thanks tomi, I will try out your suggestions, should have an answer by COB 
monday. to answer your question on how to append the downloaded pac instead of 
overwriting the template, look at the mungepac function in my patch of 10/18.
Attached patch patch that actually works — — Splinter Review
Now i get it actually work. There is one thing that may be wrong:
        // set this to null so that service is recreated
        mPacService = nsnull;

Maybe that js-modules should be unloaded some better way?
excellent idea and great execution, tomi. you beat me to it :-) i just tested 
your patch, there is one small problem, the template is getting inserted twice 
for me on solaris.
*** Bug 51357 has been marked as a duplicate of this bug. ***
I see 2 problems here:

1.
We can't write to componets dir, default that is owned by root on unix. pac
file should go to user's profile dir. Then all users can have own proxy
settings too. Also, when we realy on autoloading that proxy file from
components-dir, we cant use pac file at first run, becouse there wasnt
any file at startup time to define CID's.

2.
In my implementation first url loaded goes without proxy, that should block
while pac file is loaded and continue load after that.
See 57134 for user-specific component directories.

The PAC doubling was my error, i had the pac template prepended to the
downloadable pac. your patch is working for me now on solaris (except for the
default home page, which is not loaded at startup, one has to explicitly click
again on home). FWIW, you have my r=.

I think the right way of doing this would be to feed the downloaded js directly
to the js interpreter, without having to save to a file... but that is a design
change and it is probably too late for that to go in.
*** Bug 58711 has been marked as a duplicate of this bug. ***
Added self to CC.
Can we get this on the trunk, please?
Blocks: 18687
There's still time to get sr= and do some testing.  The patch is rather large
for this stage of the rtm builds.  A very small patch that simply prevents the
crash would be more likely to get into an if-we-respin build.  Is that even
possible?
changing status to [rtm-], no need to hold up rtm for this one
Whiteboard: [nsbeta3-][PDTP2] suntrak-n6-highp [rtm need info] → [nsbeta3-][PDTP2] suntrak-n6-highp [rtm-]
*** Bug 56773 has been marked as a duplicate of this bug. ***
Keywords: ns601
I just attached a working patch that downloads the pac into a js string and 
calls eval on it, just as brendan suggested. No need to save to disk anymore. 
Thanks to Paul Rank and Jeff Dyer for help in debugging JavaScript. So far 
tested only on Windows, starting on Solaris and Linux now. Let the reviews 
begin!
On my PC/Linux system, if I apply this patch to the 11/14/2000 trunk from CVS,
build, then set the autoconf URL to anything and try to view a page, Mozilla
crashes, printing

	###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().:
'mRawPtr != 0', file ../../../dist/include/nsCOMPtr.h, line 649
	###!!! Break: at file ../../../dist/include/nsCOMPtr.h, line 649

to the console.

Yikes!  Do I need to apply patches other than the one from 11/13/00?

After this, Mozilla will not start up at all until I manually edit my prefs.js
to disable proxy autoconf.

Anything useful I can do to help debug this?
If you see the message 'ERROR 80040154: Cannot load PAC js component' on your
console, it probably means that you forgot to copy
mozilla/netwerk/base/src/nsProxyAutoConfig.js to mozilla/dist/bin/components/.
This causes the null pointer crash later on (I will add a check for null there).
Having copied this file over, the patch works fine for me on linux. Now for
solaris... (The 11/13 patch is the only patch you need to apply).
update: the patch of 11/13 works for me on windows, linux and solaris, but only
for file urls, it crashes if pac files are specified with http urls :-(
OK, I copied nsProxyAutoConfig.js into the right place, and now both file: and
http: proxy autoconfig files are working correctly (!) when I configure them
from preferences.

However:

1. http: URLs prevent the browser from starting (although they work if I set
them from preferences and hit reload).

2. No feedback is given if an incorrect autoconf URL is specified.  NS4 and IE
both display a popup window if there is an error loading the specified URL, and
give the option to use the same autoconf that was used last time.

Also, no feedback is given if the proxy autoconf script returns gibberish,
although that's less problematic.
I rearranged code so that http urls work too, but then i lose first url
loaded :(

I think there is way to load pac url at startup time, but that might
not be good thing, eg. modem user may not want to open internet connection
every time he starts browser.

Maybe there is way to hack this so that when first url is loaded, that
url is mangled to pac url and after pac is fully loaded fist url
is requested again and loading it starts. (i think 4.x does this?)

Or there seems to be method Susped and Resume, maybe use them to
first url?
I think that always trying to fetch the autoconf file is appropriate behavior,
even for a modem user.  That's what both NS4 and IE do, I believe.

They stop this from being a huge pain by prompting you if it can't load the
autoconf, and asking if you'd like to use the previous one, or none at all.

At least IE does.  I think NS4 has the same behavior, but doesn't ask if you
want to use the old one, just uses it.

I just attached a patch that implements Tomi's idea of deferring the loading of
the pac until a url is first accessed. And like Tomi, it causes the default home
page to fail to load on startup (it works if one clicks on the home icon later).
But it causes http to work. I tried putting a mutex between ProxyForURL and
LoadPAC, but that causes the browser to hang. Igor Kushnirskiy has an idea... he
suggests posting an event into the main event queue during init and having the
main thread call us back. That might work, investigating...

Scott, this implementation does not save the downloaded pac to disk, so using
one from a previous session would be impossible. And I do not know xul, so
someone else will have to contribute the nice ui feedback on error conditions.
I made some modifications so that pac is reload if pref is changed. I also
tryed to suspend first url load, but that didnt work. Looks like Suspend()
is not fully implemented yet?
*** Bug 60383 has been marked as a duplicate of this bug. ***
Igor's idea worked. I added an event to the UI thread's queue during 
initialization, and then I load the PAC in the UI thread, and now everything 
works fine. Tested both file and http pac urls on solaris, linux and windows. 
Ready for review.
First, thanks to Sun and Tomi for beating this one. It is one of the most
requested features for Beonex.

But I am concerned about security. The downloaded PAC file gets the all-powerful
system principal, right? I think, it is neither anyhow obvious nor necessary to
give an external configuration file the power of an executable.
Is there no way to sandbox the downloaded JS, so it only gets the power web-JS
has? that way, you'd save the power of expression in the config, but greatly
reduce risk for users.
If that solution is not implemented, IMO, we need UI for a bold warning, even
for interim solutions.
this seems reasonable to me. I would still like someone like brendan to review
the JS part. I am not sure if the downloaded pac file needs to be evaluated each
time. Maybe there is way to retain the context and convey to subsequent calls.
Also cc'ing mccabe and jband. For the proxy part r=gagan
I don't have the background here, but does this configuration file get run as JS
from a file: or resource: URL, or is it run from a chrome: URL? Or none of the
above? In answer to Ben's question, just because a file is loaded from the local
drive does not give it the system principal, _unless_ it's in the chrome
directory and loaded with the chrome: protocol. Scripts loaded by the file:
protocol get whatever privileges the user has granted to all other files on the
local disk, which by default is no different than what's granted to a web page.
> does this configuration file get run as JS
> from a file: or resource: URL, or is it run from a chrome: URL? Or none of the
> above?

I'd say the latter (but my background is limited, too). How I understood it:
nsProxyAutoConfig.js is a JS *component*, i.e. it is a normal XPCOM component,
with full system powers, just that it happens to be written in JS. It never runs
through Gecko, so your capability checks don't apply to it. (Becuase of that,
maybe my statement about system /principal/ was inappropriate.)
nsProxyAutoConfig.js loads the proxy auto-config script from the net (usually
intranet, but not necessarily) and |eval()|s this, i.e. the config script
inherits full system powers (whatever powers the user has on the local machine).
Thanks, Tomi, I did'nt know one could do this in JavaScript. Your patch works 
fine for me too. 

Ben, you raise an important point, one that I raised internally a few weeks ago, 
but I would like to suggest that the security aspects be tackled in a separate 
bug. I would like to get the basic functionality checked in while I start 
learning about javascript security.
> I would like to suggest that the security aspects be tackled in a separate bug

I disagree, security is not an "additional feature". If the functionality is
there, it will be used. Do you want anybody in your local intranet be able to
read your mail?
From what Ben has described, this feature does sound insecure. Correct me if I'm
wrong, but it sounds like this is a JS component which downloads and runs JS
from a web address as your proxy configuration script. Can that script do
anything? Is it all-powerful? If so, you have given control of your system to
the owner of the server containing the proxy script, or anyone who can spoof
that server. Am I overreacting here, or is this what we're talking about? If so,
I don't want this functionality checked in without a thorough security review.
isn't this what proxy automatic config does in 4.x and 3.x and 2.x where it was 
invented by netscape and still isn't in 6.0?
I'm not saying the proxy config file shouldn't be able to configure your
proxy...it just shouldn't be able to do anything else. I'm not sure whether this
was the case in earlier versions. 
Yes this is big security hole, but i think this should be checked in (but NOT
ENABLE by default) now, and open new bug for security fix. After security fix
is in, then change makefiles so that nsProxyAutoConfig.js gets copied
to components dir.

Maybe that security could be get by using nsIScriptSecurityManager, there
is methods DisableCapability() and RevertCapability(), call those before
and after eval()?

http://lxr.mozilla.org/seamonkey/source/caps/idl/nsIScriptSecurityManager.idl
If the script is running as part of a JS component, it will always be
privileged, and the disableCapability() function won't work. Call this a design
flaw if you like. I'm not sure how to make the eval() run with normal
privileges, but there's a way, somehow, and I'll investigate. I would rather
this patch not be checked in at all until the security issue is resolved. I
agree with Ben that security is integral to any piece of functionality, not an
add-on.
We are willing to hold off trying to get this fix checked in until 
the security aspects of it are addressed, but please bear in mind that
we (Sun) need this fix before we can distribute our FCS/RTM release.
This is one of two "show-stoppers" for us, and then we are ready to
do our next release. In short, this is very high priority for us, and we
need a fix as soon as possible. We are very much dependant upon the
expertise of the appropriate Mozilla/Netscape engineers, as this 
part of the code is an unknown to us. Thanks in advance for your help! 
from Erik Olson on 11/15/2000 at http://wwww.macintouch.com/netscape6.html:

I updated from PR3 to shipping Netscape 6 last night
...
Still does nothing with Automatic Proxy Configuration settings. It doesn't even
try to connect. Useless behind a proxy firewall.
Keywords: nsmac2
*** Bug 49297 has been marked as a duplicate of this bug. ***
Rich Burridge,
note that us (me and mstoltz) blocking the checkin into the Mozilla tree doesn't
really change much for the Sun release - you can always ship with your own
patches applied (but don't forget to publish the source). Of course, I don't
suggest that.
Ben: understood, but we will do the right thing here. I too don't
believe we should ship a product with a gaping security hole in it.
*** Bug 61027 has been marked as a duplicate of this bug. ***
*** Bug 49297 has been marked as a duplicate of this bug. ***
FWIW, a heads-up, I was told by someone at the W3C AC meeting that there was an
activity being started in the IETF to redefine this automatic proxy mechanism.
can someone please mark 49297 as a duplicate of this bug?
*** Bug 49297 has been marked as a duplicate of this bug. ***
------- Additional Comments From Rich Burridge 2000-11-21 20:31 -------
> this is very high priority for us, and we
> need a fix as soon as possible. We are very much dependant upon the
> expertise of the appropriate Mozilla/Netscape engineers, as this
> part of the code is an unknown to us.

Who knows, how to sandbox JS execution, i.e. remove rights from a certain part
of JS code?
mstoltz? brendan or shaver?
This bug has way too many people on the cc list
Could someone please point me at an example of a pac file, and tell me how to
install it (with this patch checked in)? I want to see if that script really is
getting system privileges.
http://home.netscape.com/eng/mozilla/2.0/relnotes/demo/proxy-live.html
has the syntax.

www.usyd.edu.au/proxy.pac (ns4.x won't show it because of the proxy type, don't
know about mozilla, just use wget or lynx or something) is a complex example. A
lot of the actual functions aren't implemented yet in mozilla, so this example
won't actually work.

A simpler example (untested) would be:

function FindProxyForURL(url, host) {
   return "PROXY myproxy:8080";
}
Mitchell, once the patch of 11/17 is applied (with or without tomi's 11/20 
eval optimization), all you need to do is 1. enter the URL of the proxy in the 
preferences dialog 2. select the radio button for auto proxy (not direct or 
manual) and 3. restart the browser.
must also copy netwerk/base/src/nsProxyAutoConfig.js to
components/nsProxyAutoConfig.js 
*** Bug 61894 has been marked as a duplicate of this bug. ***
My suspicions are correct...code in the pac file has access to XPConnect, in
other words, full system access, when this patch is applied. I'll attach an
example. Apply the above patches, then point your pac URL at this file. When
run, it will launch c:\test.exe if this exists (edit the path= line in the
attached pac to change the exe that gets run). Obviosusly, a script should not
be able to launch executables on your system. This is bad.

The problem is how to limit what this file can do. the pac script gets eval'd
from privileged code, so the pac script is also privileged. There's no way right
now to revoke privileges from system code, but I will see about how to add this
feature.
*** Bug 62255 has been marked as a duplicate of this bug. ***
So, is this working on the trunk now?
Bijal,
  No, it has not been checked into the trunk yet because it opens a security
hole. I'm working on a solution.
My attached patch supports a notion of a sandbox to be used by JS components
(loaded by the JS Component Loader).

As a test... In the registration code of nsSample.js I added:

var obj = new Sandbox();
dump(obj+"\n");

obj.foo = "someFoo";

evalInSandbox(obj, "dump(foo+'\\n')", "http://foo.bar/baz.js");
evalInSandbox(obj, "dump(Components.interfaces+'\\n')","http://foo.bar/baz.js");
evalInSandbox(obj, "dump(Components.classes+'\\n')", "http://foo.bar/baz.js");

...This prints on startup...

[object Object]
someFoo
[xpconnect wrapped nsIXPCComponents_Interfaces]
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Access to XPConnect service denied. [nsIModule::registerSelf]"  n
sresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "<unknown>"  dat
a: no]
************************************************************

NOTE that xpconnect specifically allows access to Components.interfaces fron
untrusted code, but prohibits access to Components.classes from untrusted code.

I think this supplies what is needed.

The error handling is pretty iffy, though.

Someone please test this with the autoproxy code.
Fixed bug 63027 for the sandboxing. Adding it as blocker.
Depends on: 63027
I attached patch that uses new evalInSandbox() function to eval securely,
it also includes my previous perf patch.

Some js-guru should try with some evil proxy-js and see how it protects.

To test you need to apply 3 patch: 19422, 20793 and 20804. 
evalInSandbox requires three params (you only pass 2). It will fail otherwise. 
It needs:
1) the sandbox object
2) the text of the js code to run
3) a url string that it uses to get the codebase principals. This probably ought 
to be the url from which you loaded the js file; e.g. "http://foo/bar.js"
Please note that discussion of the security issues and my sandbox scheme got 
moved to bug 63027. The sandbox patch in this bug is not the last word on 
things. Please follow that bug if you care.
*** Bug 63301 has been marked as a duplicate of this bug. ***
Checking on the status of this bug and the related sandbox stuff, I started
wondering how NS4 handled security and the AutoConfig script.  I don't really
know anything about javascript and security, but does anybody know?

Is there a chance that all of the sandboxing that's been proposed (and
implemented) could break somebody's existing PAC scripts?  Or did NS4 run the
PAC script in a similarly restricted environment?

If there's a chance of breaking things, should there be an option to allow the
user to run an insecure PAC script?
AFAIK, the execution of the PAC script in 4x was sandboxed using the JS API,
much as we're proposing now. The major difference in the 6.0 world is that JS is
potentially much more powerful, because of XPConnect. In 4x, there is no wat
that I know of for JS, even "privileged" JS, to access arbitrary local files,
for example. With access to XPConnect, full local file access is trivial. I
don't think we have to worry about breaking existing PAC scripts, because the
operations we're trying to prevent simply did not exist in 4x.

I will try to verify Jband's patch by early next week, and we can check the
whole mess in.
Just added a patch which make use of 3rd argument of evalInSandbox().
Also joined patches 19422 and 20804 into single patch, so to test you'll
need apply this patch (22348) and jband's patch for evalInSandbox() (20793)
OK, It looks like we have a pretty big disconnect here. It has to be understood 
that if we are going to use this sandbox then the sandbox is the global object 
in which the pac file is evaluated. This means that if the pac file is going to 
need access to any additional global functions (and it does!) then you need to 
evaluate those functions in the sandbox - the pac file can't see them outside 
the sandbox.

I suggest that you bundle all the utility function (e.g. dateRange, dnsDomainIs, 
...) into a separate file and load them into the sandbox the same way you load 
the pac file. You can put that file in the same directory as 
nsProxyAutoConfig.js, but you would then need to use a file extension other than 
'.js' so that the JS Component loader will not try to load it as a JS Component. 
'.jsl' is good.

Another (perhaps much better) option is to convert that big block of function 
declarations into a quoted string (with escaped quotes for embedded quotes) and 
pass it as one big string to evalInSandbox. 

Like:

function foo() {
  return "bar";
}
function baz () {
}
becomes:

var THE_STUFF = 
 "function foo() {\n"
 "  return \"bar\";\n"
 "}\n"
 "function baz () {\n"
 "}\n";
 
It takes some fiddling, but it saves a file load.

Either way you'll need to use a codebase that is compatible with the one used to 
eval the pac file. This implies that the same sandbox probably can not be used 
for multiple pac files (do you have multiple pac files? It is unclear to me from 
the code).
*** Bug 65189 has been marked as a duplicate of this bug. ***
Hmm... things are getting worse.
Well, here's a patch which makes single string from downloaded pac file
and needed utilities (predefined funcs.) and evaluates them in sandbox.
NOTE: we need DNS-related functionality in predefined functions,
so I have to pass nsDNSService component to sandboxed code.
I wonder what security risks it imposes...
*** Bug 65034 has been marked as a duplicate of this bug. ***
+    nsCOMPtr<nsIURI> pURL = nsnull;
don't use nsnull here
+    nsCOMPtr<nsIURI> pURL;
[general] use this instead.

+var pac = null;
[general] i don't think you need to mark things as null in javascript at 
definition.

+    onDataAvailable: function(channel, ctxt, inStream, sourceOffset, count) {
this function doesn't care about embedded nulls in the stream, is it a risk?

+"    dump(\"nsProxyAutoConfig.js: dateRange function deprecated or not 
implemented\\n\");\n" +
[general] please use exceptions instead of dumps.

+"function convert_addr(ipchars) {\n"+
this doesn't look ipv6 friendly, am i right? :-(

+"function isInNet(ipaddr, pattern, maskstr) {\n"+
this _definitely_ doesn't look ipv6 friendly. :-(

+"    return (ip != null) ? true: false;\n" +
this should suffice:
+"    return (ip != null);\n" +
actually, |return ip;|. might work (although it would leak information), too 
bad for lose javascript typing.
> +var pac = null;
> [general] i don't think you need to mark things as null in javascript at 
> definition.
Is it bad?

> +"    dump(\"nsProxyAutoConfig.js: dateRange function deprecated or not 
> implemented\\n\");\n" +
> [general] please use exceptions instead of dumps.

Will |throw "dateRange deprecated ..."| suffice?
Actually, if remove dateRange at all, exception will be thrown:
"function not defined" (or something like that) 
Or it's better to throw exception by myself?
 
> +"function convert_addr(ipchars) {\n"+
> this doesn't look ipv6 friendly, am i right? :-(

> +"function isInNet(ipaddr, pattern, maskstr) {\n"+
> this _definitely_ doesn't look ipv6 friendly. :-(

This is how isInNet currently implemented.
I just rewrote this in JS.
I seen comments in the code saying it should be in JS,
so here is it :-)
Do you have suggestions how to make it ipv6 friendly?

*** Bug 66186 has been marked as a duplicate of this bug. ***
I was asked to review this. I see problems...

- First and foremost, Are there test cases? What are the requirements? I see a 
bunch of "dump("foo: deprecated or not implemented\n")" Which is it? deprecated 
or not implemented? To me 'deprecated' is a warning to not use something that 
*might* not be supported in the future. Once you remove the function it is past 
being deprecated. Isn't the unavailability of all this functionality going to 
break a lot of scripts? Please explain or point to a document that talks about 
why so many utility functions are no longer part of the autoproxy config JS 
environment.

- 'dump' is useless to end users (and most autoproxy script hackers). Throwing 
an exception allows for the caller to possibly catch the exception or for the 
exception to bubble up into the JS error console.

- JS does not support method overloading. You are adding multiple properties 
to the global object each with the same name. So "dateRange(dmy)" is shadowed by 
"dateRange(dmy1, dmy2)" which is shadowed by "dateRange(d1, m1, y1, d2, m2, 
y2)". The latter is the only one callable. You need to declare only one function 
with this name and decided dynamically what the parameters mean (i.e. which 
variant the caller was trying to get at) by checking arguments.length. Again, I 
wonder about tests.

- this is all dependent on bug 63027. It won't work untill that bug is 
resolved. I'll push again to try to get some opinions on that bug.

- I don't have much to say about the rest of the code. I've never messed with 
auto proxy stuff. I'd hope that one of the *numerous* people on this bug's cc 
list would review that code.
As for those numerous dump()'s, I just got them when I come here :).
Well, maybe I'll implement all those 'deprecated' functions in few days,
except shExpMatch - I don't think it has much sense since JS now supports
regexps by itself. Also, it's not clear for me what's status of all those 
deprecated function. All I found is that "Note that since its original
implementation in Mozilla/2.0 a significant set of api's for PAC has been
deprecated. " (see http://www.mozilla.org/docs/netlib/pac.html)
As for breaking existing PACs,  auto config doesn't work for now anyway ;-)))
I think someone from Netscape may clarify things here (gagan@netscape.com?)
When deprecated must become past deprecated?

But I still have a question for you, John: assuming your sanbox implementation
is good, do you have any comments on sanbox use here?

Also I see problems with myIPAddress() function, it's dummy :-(
Does someone on the cc: list have an idea how to implement it ?
Thanks for picking up this bug, Denis. On the topic of the myIpAddress, the 
current implementation is very inefficient, it does a DNS lookup of localhost 
for every URL. It would be preferable to cache the value.
Assignee: akhil.arora → adu
Status: ASSIGNED → NEW
erm. Shouldn't we cache the array of localhost ip's and their understanding of 
netmasks?  I have 3 ips assigned to my box, and i do occasionally use all of 
them.

Oh, and can anyone comment on the difficulty of supporting ipv6 ips?
Keywords: nsbeta3, rtmhelpwanted
Whiteboard: [nsbeta3-][PDTP2] suntrak-n6-highp [rtm-] → [PDTP2] suntrak-n6-highp
As for myIPAddress(), "localhost" is resolved into 127.0.0.1, i.e. loopback
interface, while we need a way to get IP address(es) of 'real' interfaces....
I suspect we can't get it from inside sandbox...

Regarding deprecated function, here is implementation of weekdayRange():
------------------------------------------------------
var date = new Date();

var wdays = new Array("SUN", "MON", "TUE", "WED", "THU", "FRI", "SAT");

function weekdayRange() {
    function getDay(weekday) {
        for (var i = 0; i < 6; i++) {
            if (weekday == wdays[i]) 
                return i;
        }
        return -1;
    }
    var argc = arguments.length;
    var wday;
    if (argc < 1)
        return false;
    if (arguments[argc - 1] == "GMT") {
        argc--;
        wday = date.getUTCDay();
    } else {
        wday = date.getDay();
    }
    var wd1 = getDay(arguments[0]);
    var wd2 = (argc == 2) ? getDay(arguments[1]) : wd1;
    return (wd1 == -1 || wd2 == -1) ? false
                                    : (wd1 <= wday && wday <= wd2);
    
}
------------------------------------------------------

I gonna put this into attachment as far as I implement all functions
Status: NEW → ASSIGNED
Denis: Please do implement "shExpMatch" - it may be "pointless" but I don't want 
to rewrite proxy.pac scripts that use it!
> Please do implement "shExpMatch" - it may be "pointless" but I don't want
> to rewrite proxy.pac scripts that use it!

Feel free to implement shExpMatch yourself.
OK, I got number of requests to implement shExpMatch, so here it is.
BUT! I'm quite busy now and have no much time to test it.
So if you need it, please help testing it.

---------------------------------------------------------
function shExpMatch(url, pattern) {
   pattern = pattern.replace(/\./g, "\\.");
   pattern = pattern.replace(/\*/g, ".*");
   pattern = pattern.replace(/\?/g, ".");

   var newRe = new RegExp(pattern);
   return newRe.test(url);
}
-----------------------------------------------------


 
I'm trying to verify that evaling PACs in the sandbox is in fact secure, but now
I can't get PACs to work at all. I've applied patches 20793 and 20804 from
above, but I think I'm doing something wrong. I'm seeing the "cannot load PAC JS
component" error in the console. Have I missed a step?

Can someone else give this a shot? Akhil, Dennis, whoever is actively working on
this, can you try to verify the security? I'll attach a security test. Chenge
the path=... line to the path to some executable on your system, then make this
file your PAC. Without sandboxing, when you visit a new page, the script runs
and starts the executable referred to in the script. If the fix works, the
executable will not be run. 
> I'm seeing the "cannot load PAC JS component" error in the console. 
> Have I missed a step?
Currently you have to manually copy nsProxyAutoConfig.js to dist/bin/components
directory to make it work...

>If the fix works, the executable will not be run. 
Hmmm... I successfully ran executable from PAC loaded through file: protocol
Exactly, I use sandbox this way:
	evalInSandbox(ProxySandBox, mypac, channel.URI.spec)
where channel.URI.spec is URL of PAC file.
 
If it's OK to give system privs to local files, fix is works, otherwise
we can use it with some dummy URL, like this:
	evalInSandbox(ProxySandBox, mypac, "http://dummy.com/dummy.pac");


Do we have an ETA for a patch?  I am trying to roll Mozilla out to 3000 users as
a pilot to a full corporate rollout within my comapny. We cannot go without
autoProxy  however.
*** Bug 68101 has been marked as a duplicate of this bug. ***
*** Bug 68369 has been marked as a duplicate of this bug. ***
Whew, here is implementation of dateRange()... It's quite straightforward,
so patches are welcome... Also I think it's overkill to call this function
on every url request...
--------------------------------------------------------------

var monthes = new Array("JAN", "FEB", "MAR", "APR", "MAY", "JUN", "JUL", "AUG",
"SEP", "OCT", "NOV", "DEC");

// JS engines I used for testing doesn't allow nested functions.
// But for mozilla it may be moved inside of dateRange()
    function getMonth(name) {
        for (var i = 0; i < 6; i++) {
            if (name == monthes[i])
                return i;
        }
        return -1;
    }
function dateRange() {
    var argc = arguments.length;

    if (argc < 1) {
        return false;
    }
    var isGMT = (arguments[argc - 1] == "GMT");

    if (isGMT) {
        argc--;
    }
   
    // function will work even without explict handling of this case
    if (argc == 1) {
        var tmp = parseInt(arguments[0]);
        if (isNaN(tmp)) {
            return ((isGMT ? date.getUTCMonth() : date.getMonth()) ==
getMonth(arguments[0]));
        } else if (tmp < 32) {
            return ((isGMT ? date.getUTCDate() : date.getDate()) == tmp);
        } else { 
            return ((isGMT ? date.getUTCFullYear() : date.getFullYear()) ==
tmp);
        }
    }
 
    
    var year = date.getFullYear();
    var date1, date2;
    date1 = new Date(year,  0,  1,  0,  0,  0);
    date2 = new Date(year, 11, 31, 23, 59, 59);
    var adjustMonth = false;
    for (var i = 0; i < (argc >> 1); i++) {
        var tmp = parseInt(arguments[i]);
        if (isNaN(tmp)) {
            var mon = getMonth(arguments[i]);
            date1.setMonth(mon);
        } else if (tmp < 32) {
            adjustMonth = (argc <= 2);
            date1.setDate(tmp);
        } else {
            date1.setFullYear(tmp);
        }
    }
    for (var i = (argc >> 1); i < argc; i++) {
        var tmp = parseInt(arguments[i]);
        if (isNaN(tmp)) {
            var mon = getMonth(arguments[i]);
            date2.setMonth(mon);
        } else if (tmp < 32) {
            date2.setDate(tmp);
        } else {
            date2.setFullYear(tmp);
        }
    }

    if (adjustMonth) {
        date1.setMonth(date.getMonth());
        date2.setMonth(date.getMonth());
    }

    if (isGMT) {
    var tmp = date;
        tmp.setFullYear(date.getUTCFullYear());
        tmp.setMonth(date.getUTCMonth());
        tmp.setDate(date.getUTCDate());
        tmp.setHours(date.getUTCHours());
        tmp.setMinutes(date.getUTCMinutes());
        tmp.setSeconds(date.getUTCSeconds());
        date = tmp;
    }
    return ((date1 <= date) && (date <= date2));

}
----------------------------------------------------------------
*** Bug 69305 has been marked as a duplicate of this bug. ***
I just created attachment with all predefined PAC functions implemented
(no more dump("not implemented")'s). There's still problem with
myIPAddress, however, but it's subject for another bug, probably ;-)

So what about next round of review? Seems Netscape guys don't want 
to bother with this and 63027 (our blocker) ;-)))
<+"    return (host.replace(/\\w*/, \"\") == domain);\n" +
>+"    return host.replace(/\\w*/, '') == domain;\n" +

in general please |return condition;| instead of |return (condition);|.
and instead of using \" please use ' where possible.

+"function convert_addr(ipchars) {\n"+
...
+"    var result = ((bytes[0] & 0xff) << 24) |\n"+
             this v^ doesn't line up.
+"                ((bytes[1] & 0xff) << 16) |\n"+
+"                ((bytes[2] & 0xff) <<  8) |\n"+
+"                 (bytes[3] & 0xff);\n"+

otherwise you can take this as an r=timeless for nsProxyAutoConfig.js
If someone really wants to pick nits, there are a bunch of cases of

+        rv = mPrefs->CopyCharPref("network.proxy.autoconfig_url", 
+                                  getter_Copies(tempString));
+        if (NS_SUCCEEDED(rv) && tempString && *tempString) {

where the tempString clause (which converts the nsXPIDLCString to (const char*))
is unnecessary -- unless there is a bug in CopyCharPref where it allocates an
out string param buffer but fails to return NS_ERROR_OUT_OF_MEMORY when the
allocation fails.  If there were such a bug, it should be fixed in nsPref.cpp,
not patched around at all callsites.

I realize this pattern is not new with the latest patch -- it's all over the
place in nsProtocolProxyService.cpp -- but no time like the present to clean up
(better than just before a milestone or major release deadline :-).

+            // create an event
+            PLEvent* event = new PLEvent;
+            // AddRef this because it is being placed in the PLEvent struct
+            // It will be Released when DestroyPACLoadEvent is called
+            NS_ADDREF_THIS();

Note addref here.

+            PL_InitEvent(event, 
+                         this,
+                         (PLHandleEventProc) 
+                         nsProtocolProxyService::HandlePACLoadEvent,
+                         (PLDestroyEventProc) 
+                         nsProtocolProxyService::DestroyPACLoadEvent);
+
+            // post the event into the ui event queue
+            rv = eq->PostEvent(event);

PostEvent returns a PRStatus, not an nsresult, so

+            if (NS_FAILED(rv)) {

this should really be rv == PR_FAILURE, although it happens to work (PR_SUCCESS
and NS_OK are 0; PR_FAILURE is -1 and all XPCOM error nsresults have the high
bit set, and NS_FAILED tests that bit).

+                printf("ERROR %x: %s ", rv, 
+                       "Failed to post PAC load event to UI EventQueue\n");
+                return;

Early return without NS_RELEASE_THIS or delete event?  I guess it is assumed
that the xpcom/threads code takes care of all that, but from all I can see, it
ain't so.

danm, what are the rules here?  Since the caller of PostEvent is handing off the
event and any data it holds, it seems to me that PostEvent should call the
event's destructor in any case where it's clear that the event won't make it
through a queue to some consumer on another thread who *will* do the clean-up.

denis: are there thread-safety issues here with the callbacks running on the UI
thread while the rest of the PAC code races on its own thread?  I don't see any
obvious trouble (either Necko interface methods are called, and they must be
thread-safe already; or pointers such as mPAC are used, which must be valid and
unchanging), but I thought I'd ask, since the latest patch adds concurrency.

+            }

The comment earlier, the one that says "loading it now, in the current thread,
results in a browser crash" should really say something about the callback doing
UI or other things implemented by single-threaded code.  The comment sounds like
a crash is being worked around rather than investigated, but I think we all
understand that Gecko and related code is mostly single-threaded by design.

Style nit: indentation is whacked below in several ways (tabs?):

+        if(!ProxySandBox) {
+           ProxySandBox = new Sandbox();
+        }
+       // add predefined functions to pac
+       var mypac = pacUtils + pac;
+        // evaluate loded js file

The code in the pacUtils var has else after return in lots of places, not a big
deal, but a non-sequitur that tends to over-indent successive cases.

Sorry I'm mostly picking nits and finding fault in other (PostEvent) code.  You
seem to have addressed jband's concerns. Can you get a necko guy to review this
code?  I'm off to look at bug 63027.

/be
> in general please |return condition;| instead of |return (condition);|.
> and instead of using \" please use ' where possible.

Reasons?
the first is syntax, both are readability, if you need better reasons i think 
brendan could provide them...
timeless: I think you need to worry about more substantive problems now and then
or people will get tired of such nit-picking -- I know when I cross that line,
and I go back to substance and repent.  Really, some of these things are matters
of taste.  Others have more substantial effects on readability and clarity (such
as that do-while(false) loop you pointed me at).  Others still have bad
practical and social effects (tabs in files being public enemy number 1).

Anyway, back to substance.

/be
*** Bug 69794 has been marked as a duplicate of this bug. ***
Attached patch Brendan's concerns addressed — — Splinter Review
Created attachment with Brendan's concerns addressed.
Anyone to review again? (Brendan, could you take a look at new attach?)
As for thread issues, is there gecko gurus on the Cc:? Could you review thread
safeness?

As for tabs and parens, is there mozilla coding style? 
Everything looks pretty good to me, except for all the printfs.  Consider using
NS_ASSERTION or NS_WARNING for conditions that you think "should not happen".

There is no one Mozilla coding style that mozilla.org enforces; the community is
too big.  mozilla.org abominates tabs, because they don't expand to the same
number of spaces on all platforms, and even on certain platforms (depending on
the tool used to browse or edit source).  mozilla.org looks for a *consistent*
style in a module -- or rather, the lack of it, which is a tell-tale of lack of
ownership, and generally indicates or predicts worse problems in the code.

Hit me with another patch that turns those printfs into something better, and
I'll sr=.

/be
Attached patch Wiped out printf()s — — Splinter Review
Attachment no. 26519 - no more printf's

Brendan: I hope I have your sr= now :-)

Gagan/Jband: could you please review JS part, please?

Anyone else who may give me r= : please review (I need r='s, right?)
Wow! Just realized that Gagan is NetLib module owner :-))
Gagan, could you please review the _whole_ stuff?
All looks good -- only nit is overindented code visible here:

+        if (NS_SUCCEEDED(rv = mPAC->ProxyForURL(aURI, 
+                                                getter_Copies(p_host),
+                                                &p_port, 
+                                                getter_Copies(p_type))))
             {
                 if (NS_FAILED(rv = aProxy->SetProxyHost(p_host))) 
                     return rv;
@@ -272,7 +381,6 @@
                     return rv;
                 return NS_OK;
             }
-        }
         return rv;

/be
Blocks: 54804
denis, brendan: it looks like we can roll this fix into the Netscape 6.01A
release if Denis can make the change Brendan suggests, and Brendan gives
approval for the change right away.

We're within a day, probably, of getting the rest of our show-stoppers in, and
we'd love to put this bug in (it'd be a huge win for Sun internally, as we
mostly use PAC files for browser proxy configs).

Denis, can you make the change?

Brendan, what more do we need to do to get approval to check in, besides your sr?
Note that this is still blocked by bug 63027. No one seems to want to review 
that bug. Shaver's comment on that bug was basically (and I paraphrase) "If 
this is not as secure as the name suggests, maybe you should change the name".

Is someone going to stand up and say that the security of the whole scheme here 
is sufficient?

I still haven't seen much talk on this bug of testcases or evidence that this 
works with a real-world mix of PAC scripts.
jband: We can provide Sun's real-world PAC file as an attachment if you wish.  I
don't know about other stakeholders in this bug, but I'm happy to pony up our
real-world test case.

Also, about the other bug (63027), it sounds like it's blocked waiting on you to
rename the functionality, that's it.  If you do that, then ask for review again,
it sounds to me from shaver's comments that it'll get approved.

About security concerns, I thought that was mstoltz's responsibility.  When
you're asking for somebody to stand up and say that the security of this scheme
is sufficient, is it Mitch you're asking?

Will you please make the change shaver describes to 63027 and get his approval,
then?  It sounds as if that's the primary blocker to getting this bug fix
approved for checkin.
> We can provide Sun's real-world PAC file as an attachment if you wish

I don't want to *run* the tests. I want someone to say how seriously they've 
tested it.

> Also, about the other bug (63027)

Shaver's opinion seemed to be to rename the function (and the constructor?) No 
one else gave an opinion. Changing the name does not make it any safer. I wrote 
that code so mstoltz would not have too. But, it exists to service this bug. 
Does anyone think it is the right solution?
Whiteboard: [PDTP2] suntrak-n6-highp
George, I made the changes, Brendan suggested, so its time to give/not to give
r= and a= :-)
BTW, don't we need to get those r= and a='s from module owner
(Gagan@netscape.com) or peers?
Also, note that we need simple change in Makefile to auto install PAC component
into dist/bin/components directory:

Index: Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/netwerk/base/src/Makefile.in,v
retrieving revision 1.40
diff -u -r1.40 Makefile.in
--- Makefile.in 2000/09/15 19:26:59     1.40
+++ Makefile.in 2001/03/05 13:35:28
@@ -63,3 +63,6 @@
 
 include $(topsrcdir)/config/rules.mk
 
+install::
+       $(INSTALL) $(srcdir)/nsProxyAutoConfig.js $(DIST)/bin/components
+
I see two r=gagan's on earlier versions of the patch.  As the patch is refined,
so long as it does not add substantive new code that gagan or another module
owner/peer should review, I don't see the need to resolicit r=.  If there *are*
substantive changes that I've missed (I didn't diff the earlier patches against
the later; I'm going by memory), then gagan or darin should re-review.  I'm
cc'ing darin so he has the opportunity.

I already gave my approval -- sorry if that was unclear. 
sr=brendan@mozilla.org.  We can handle remaining problems not caught by review
in followup bugs, I think.  We shouldn't put this off till the last minute of
0.8.1; we should get it in this week.

/be

/be

r=darin (denis, can you land this soon?  thx!)
53080 depends on bug 63027, that seems to have stalled. What still needs to 
be done before 63027 will be r=/a='ed and checked in?
My apologies for staying away from reviews for a while. This bug has really come
a long way and I am very glad to see the excellent progress and contributions
made by everyone. As brendan pointed out most of this stuff looks fine to me and
should land soon. I do have a concern which I will add here (concern, but not
critical enough to delay landing this)-

the default value on proxy ports as in all.js is zero. I had filed a bug long
time back but haven't seen much progress on it with shifting prefs-owners. This
is why the check for proxyPort>0 was required. Till that happens (and I might
just do that myself) it might be a good idea to leave the check for ports>0 when
we fetch the prefs.

Other than that this looks great! Get it checked-in soon...
I tried once again to verify the security of this feature, and once again I 
can't get the feature to work. Has anyone else applied this patch and tested it 
within the past week? I think it has been broken by the big Necko changes of 
last Wednesday. For one thing, channel.asyncRead at nsProxyAutoCOnfig.js, line 
93, needs to be changed to asyncOpen. I tried doing that, and using the PAC 
still fails because mPACURL at line 280 of the patched 
nsProtocolProxyService.cpp is an empty string. I don't know why this is. As soon 
as I can get a working test of the PAC feature I can evaluate its security, but 
right now the latest patch seems to have bitrotted.
Drat -- all our optimism and the Necko interface changed!

Denis, can you provide a working, up-to-date patch?

/be
My bad... I should have caught this before giving it an r= !
*** Bug 71166 has been marked as a duplicate of this bug. ***
Attached patch Adapted to Necko changes — — Splinter Review
Added attachment adapted to latest Necko changes. I verified that PAC is loaded
successfully, but I encountered wierd problem: now I'm failing in
evalInSandbox() :
---------------------------------------------------------------------------
JavaScript error: 
 line 0: uncaught exception: [Exception... "ServiceManager::GetService returned
failure code: 0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)"  nsresult:
"0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)"  location: "JS frame ::
chrome://communicator/content/tasksOverlay.js :: <TOP_LEVEL> :: line 25"  data:
no]
------------------------------------------------------------------
To test, you'll need attachments no. 27008 (this fix) and 20811 (sandbox
implementation). Also don't forget to copy nsProxyAutoConfig.js to
dist/bin/components directory.

I'll be offline for next three days and will get back only on Sunday.
If someone were look in the problem.... I'll be very grateful :-)
On Sunday I'll try to run this on fresh mozilla build I'll make myself
(today I used alien build); now leaving mozilla being updated.... 

I applied the latest patch, and the patch failed - I got a bunch of 'Hunk
Failed' errors. Could you please post a patch that's in sync with the current
trunk? I can't verify the security until I see a working patch.
Attached patch "hunk failed" fix — — Splinter Review
Mitchell, could you please send rejects to me? 
Well, I get only one 'hunk failed' when applied patch 27008.
I created new attachment (#27449) and tested it today against following
versions of files:
nsIProxyAutoConfig.idl		ver. 1.4
nsProtocolProxyService.h	ver. 1.7
nsProtocolProxyService.cpp	ver. 1.16
nsProxyAutoConfig.js		ver. 1.7

Patch is applied w/o any errors
I am trying to get this working for me.  Unfortunately, the autoproxy 
here uses SOCKS.  Now, there is a line in nsProxyAutoConfig.js that 
says "TODO warn about SOCKS".  What is there to warn about?  It 
obviously doesn't work (I get assertions).  Even when I change this 
line:
   var hostport = /^PROXY ([^:]+):(\d+)/(proxy);
to be this:
   var hostport = /^SOCKS ([^:]+):(\d+)/(proxy);
I get a warning from the SOCKS server that mozilla is treating it a an 
HTTP server.  What do we need to add to get SOCKS to work?
*** Bug 71986 has been marked as a duplicate of this bug. ***
Thease patches are starting to rot.. Please checkin soo, enable
them when reviews are done.

Now there is evalAsSandbox() line missing fron latest patch.. and
maybe it should be evalAsCodebase() as newer sandbox patch seems to define it.

onStopRequest should be like this:
    onStopRequest: function(request, ctxt, status, errorMsg) {
        this.done = true;
        if(!ProxySandBox) {
           ProxySandBox = new Sandbox();
        }
        // add predefined functions to pac
        var mypac = pacUtils + pac;
        // evaluate loded js file
        evalAsCodebase(ProxySandBox, mypac, request.name);
        ProxySandBox.setDNS(dns);
        LocalFindProxyForURL=ProxySandBox.FindProxyForURL;
    },
New patch is in sync with latest patch for sanbox (bug 63027).
BTW, in one in my latest patches I lost evalInSandbox() call
(now it's evalAsCodebase) ;-(. So it's really a good idea to
checkin the patch before new changes in necko or new looses
in patch itself ;-) (I don't have check-in rights)
note that after I land bug 72831 (hopefully tonight) the impl. of myIpAddress
should change to directly calling GetMyIPAddress on the dnsService. Without that
myIpAddress() would always return the loopback 127.0.0.1
We have a problem.  Cannot load Java applets over a proxy due to these 
lines:
+        /* If we're not done loading the pac yet, wait (ideally). For
+           now, just return DIRECT to avoid loops. A simple mutex
+           between ProxyForURL and LoadPACFromURL locks-up the
+           browser. */
+        if (!this.done) {
+            host.value = null;
+            type.value = "direct";
+            return;
+        }

The Java plugin asks for proxy info and gets back that it is a direct 
connection, resulting in exceptions in the Java applet because it 
cannot find the host.  Removing these lines fixed this problem.  How 
do we rewrite these lines the correct way?
Blocks: 30387
Well, sandbox is in the tree, so here is update to use checked-in version.
Note thought, implemented sandbox is ultra-secure now :-), 
is doesn't allow any XPConnect calls outside of it (At least I failed
to made them, got no answer from Shaver yet on possibility of doing this).
This has one drawback: we can't use DNSService component, so there is
no dnsResolve() anymore. If you feel that's too bad, fill the bug :-)
On the other side, myIpAddress() is now returning actual IP address 
(primary one?), thanks to Gagan!
Whiteboard: attach #30744
You mean that "is resolvable call"? Please don't put that back, it's a horrible
call that depends on a timeout. People use this when their internal DNS is done
wrong. One person partially responsible for it's invention said he coudln't
think of a single practical use for it.

Since all the code for this sounds like it is going to land soon, can we have a
discussion (in the NEWSGROUP) about what needs be done to test and verify this
bug? This defect is too long already...
I noticed that my suggestion about checking for proxyport>0 is not in this 
patch but we should not hit that case unless the default prefs are messed up. 
Anyway-- take it or leave it. r=gagan 

Get an sr and get this checked in soon!
sr=darin, but you'll probably want to fix the calling convention of
onStopRequest... it changed recently ;-)
Just applied this patch to a trunk build of this afternoon.  Tested it; it works
for me, and I'm inside a bona fide auto-proxy-config-type network.
Checking in the fix on behalf of Denis.  God bless us all, let's hope it sticks.
Marking as fixed.  Build on the Tinderbox looks good after testing.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
I notice the change to the makefile listed in the comment by Denis Antrushin at
2001-03-05 05:37 was not checked in.  The tinderboxes are giving the assertion
at nsProtocolProxyService.cpp, line 227 in their logs, presumably because of this.

We also need to make similar makefile changes for Windows and (project file
changes for) Mac.
r=blizzard on the last patch
Reopen. This is now causing an assertion on startup. This happens because the 
"network.proxy.autoconfig_url" is an empty string, so you try to load a URL 
that's an empty string.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Last attachment fixes the assertion on startup.
r/sr=jband on Simon's fix.
Should also remove debug() function from netwerk/base/src/nsProxyAutoConfig.js
becouse it dumps stuff in opt build too, it may annoy some people.
There seems to be a problem with PAC now: 
when starting build 2001041921 (Linux), I get on console :
PAC: Reloading PAC from http://support.free.fr/proxy-adsl.pac

but Mozilla is currently configured to use Manual proxy, not Automatic
(I no longer use PAC).

When trying to load some web pages (maybe linked to Java), I've got
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "[JavaScript Error: "dnsResolve is not defined" {file:
"http://support.free.fr/proxy-adsl.pac" line: 33}]
[nsIProxyAutoConfig::ProxyForURL]"  nsresult: "0x80570021
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************
In last nights build (2001041921) all prefs are broken, checking
that caused that has been backed out and new builds should be ok.
The autoproxy stuff appears to be working very well, except for one small
problem. Using Mozilla nightly Build ID: 2001042321, with the following code in
the proxy autoconfig file, I get the error below it, and no page is displayed. I
imagine this alert command would be used quite often in PAC files...

--- Code fragment from PAC file ---
      if (netcache_notify == 0) 
        {
        alert("BLAH, BLAH, BLAH bunch of warnings about acceptable use, etc...");
        netcache_notify = 1;
        }
      }

--- Error when code fragment is executed ---
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "[JavaScript Error: "alert is not defined" {file:
"http://foo.bar.com/" line: 302}] [nsIProxyAutoConfig::ProxyForURL]"  nsresult:
"0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame
:: chrome://communicator/content/utilityOverlay.js :: openTopWin :: line 186" 
data: yes]
************************************************************

 
Please fix the assertion! I'd like to propose an alternate fix: only (try to) load 
the PAC file when a user selects proxy-autoconfig. This will still assert on 
an empty URL, but only for those who should care about it (ie. not for 
Simon, John or me ;-)). I think that regardless of the assertion, this fix is 
the right thing to do anyway.
On the issue of "alert is not defined":

These pac files are not run in a window with all the nice builtin properties of 
'window' There is no alert available, no DOM, Just core JavaScript, the 
functions supplied as the autoproxy api, 'dump' which will send strings to the 
DOS console (or the platform equiv.), and 'debug' which does the same as 'dump', 
but only in a debug build of the browser.
Blocks: 76921
The assertion coming out of nsProtocolProxyService::HandlePACLoadEvent on 
EVERY STARTUP is extremely annoying - on some platforms it kills the process you 
know. Please consider eliminating the assertion by checking the argument you are 
passing to NEwURI. I'm attaching a patch that does just that. Please consider 
applying the patch or allowing me to. Thanks.
Well, so far we have 3 patches to fix the assertion.  I guess I like peterv's
the best (since it also avoids loading the PAC component when it's unnecessary),
so r=dbaron on that one (although it would be nice if the authors of the PAC
code commented).  Does anyone else have other preferences?
Shall we mark this one 'mostpatched'?

Peter's patch is the best, I agree. I vote for applying it since the owner seems
to be non-responsive. Maybe I'm just especially sensitive to this, but an
assertion at startup is really nasty for some platforms as it kills the app
(that is what assertions are traditionally suppossed to do). sr=attinasi on
Peter's patch (id=32009)

Adu, do you want me to take this over and take responsibility for the patch? I'd
be happy to...
> Adu, do you want me to take this over and take responsibility for the 
> patch? I'd be happy to...

Yes, please do. Andrew (adu) is in Russia, and doesn't even have CVS access
to Mozilla. We (Sun) took an interest in this bug as unfortunately Mozilla/
Netscape engineers have been busy on other things. It would make a lot more
sense for this to be owned by a Mozilla/Netscape engineer with CVS access.
Thanks for offering to do just that.
For me, Linux 2001042608 nightly, there is at least one serious problem.

There was an error in my original PAC file, so I uploaded a new one.
Mozilla ignored the new version, even after pressing Reload in the proxy prefs
dialog and restarting.
I renamed the fixed file, changed the entry in prefs, hit reload, restarted
Mozilla multiple times. No effect.

If I can do any more to test this, or if anyone knows a way (short of blowing
away my profile or mozilla) to reload the PAC please mail me.
If PAC is officially "WORKING" and this needs to be a new bug, you have only to
say the word.
Ruth, can you try a new profile (without blowing away your existing profile)?

I'm taking this bug to help shepard in the assertion fixes. I know nothing about
ProxyPAC issues, or the state of Mozilla with regard to it. Hopefully others
will chime in. The previous owner is not available anyway.
Assignee: adu → attinasi
Status: REOPENED → NEW
SPAM: Please shoot me. adu is Denis Antrushin, not Andrew <mumble>. My
apologies.
So, cna Denis fix the assertion???
Denis does not have CVS access. He would have to get somebody else with 
CVS access to check the code in. I thought you just volunteered to do this?
I'm happy to do it, I just wnated to make sure I'm not stepping on any toes here.

So, we have r= and sr= for Peter's patch. I'll get it in soon.
Status: NEW → ASSIGNED
r=gagan for last two patches.
I'm taking over QA of this bug.

If you put an http or file URL into prefs and it loads a one line .pac file that
works, I would like to close this bug out and force new problems into new bugs.

I've posted to the necko group to begin this discussion, but at the latest, I
will probably close this bug at the next mozilla or Netscape milestone.
QA Contact: tever → benc
Peter's patch is now checked in.

benc - could you please verify that this bug is fixed? My guess is that it is
since the patch will not change the behavior when a proxy PAC is specified...
Thanks!
Did this last patch land on the MOZILLA_0_9_BRANCH ?  Do we need it there for
PAC to work poperly?
I landed the patch on the TRUNK only. It simply removes an assertion, it is not
needed to make PAC to work properly as far as I know. The assertion is just
anoying, but it does not cause andy obviously deliterious effects.
It would be sort of nice if the mozilla 0.9 milestone release did not assert on 
startup. No?
I'm not sure we can convince drivers to accept this for the branch, but if
somebody does, I'll be happy to commit it to the branch too. My understanding is
that we are currently restricting the 0.9 branch for *critical* checkins (BWDIK?).
Are there really platforms which kill apps for assertions? if that's the case 
then i think that's a good enough arguement for committing to the branch.
attinasi: I don't know if the patch for this bug is involved, but certainly pac 
is.  Bug 76921 was created due to the check in of 53080.
marc: sorry for the delay. here's my general plan.

I'm going to wait for .9 to come out, verify against the comercial and the
mozilla release.

I'm going to use the minimal test I described above.

No matter what, I'm resolving this bug in some way, and breaking out every issue
into new bugs.
I have created bug #79057 for the absence of dnsResolve() because that's
definitely going to be the main thing blocking use of PAC where I am.

People following this bug (53080) because they intend to roll out Mozilla in an
environment where PAC is necessary should check whether they use isInNet() or
isResolvable() and consider voting for #79057 if they need it.
Keywords: nsenterprise
Blocks: 79244
Blocks: 79057
Blocks: 79248
Blocks: 79251
I have created bugs for the following PAC-related issues, many of these were
mentioned here:

79242
  IPv6 + PAC: isInNet() function needs to support new addressing
79057
  PAC: dnsResolve() (thank you ruth)
79244
  PAC: myIPAddress()
79246
  PAC: reload of PAC file fails
79248
  PAC: mail folders disappear w/ PAC failures
79251
  PAC: weekdayRange()
65034
  PAC: request for shExpMatch

I've tested a one line PAC file w/ Mozilla 0.9, and it works in Win32 and Linux,
but not in MacOS.

I'm going to move to verify this bug and create an new [meta] bug for m0.9.

I'll also create a new bug for Mac after I finish some other things.
No longer blocks: 79057, 79244, 79248, 79251
If you set automatic proxy configuration URL to
http://proxy.library.upenn.edu/pennproxy.pac
Mozilla 0.9 starts OK.
In http://www.library.upenn.edu/webbin5/resources/dbfast.cgi
some of the databases work.  You can tell because you get a
prompt asking for your Penn login information.  (If you get
that far, they work.)  E.g., ISI Citation Indices.
But most do not work, e.e., Medline.  They all work in
Netscape 4.7.  It is beyond my abilities to figure out what
the difference is between those that work and those that don't.
baron@cattell.psych.upenn.edu: Thats probably bug 65034 (although shExpMatch is
implemented from what I can see).

Whats the URL for a site that doesn't work? (please comment in 65034)

Can this bug be closed out?
Blocks: 65034
I am closing this bug for comments.

I'm working on getting this marked fixed and targeted for m0.9 milestone.

Basically:

IF YOU ARE USING M0.9, UPDATE BUG 79893.
IF YOU ARE NOT USING M0.9 OR LATER, UPGRADE.
Summary: Cannot specify any URL for Proxy Automatic Configuration (PAC) files. → [CLOSED]PAC: did not work pre-Mozilla 0.9 (-> go to 79893)
*** Bug 47223 has been marked as a duplicate of this bug. ***
Blocks: wpad
Who checked in the patch that actually hooked up the URL preference to the pac
loading mechanism?

You need to take ownership of this bug, mark it fixed, and ideally look at bug
80363, because it doesn't seem to work on MacOS.
> Who checked in the patch that actually hooked up the URL preference to the pac
> loading mechanism?

What do you mean? There was four checkins (or so):
1. PAC implementation itself
2. Makefile correction, so code builds on every platform
3. Load PAC only when neccessary and
4. Check for empty strings.
So, what exactly check-in you mentioned? If the first one, the checkin was
made for me, 'cause I'm don't have checkin permsissions myself.
If you did #1, you get credit. Please assign the bug to yourself and mark it
fixed. If you don't have permissions, I'll do it.

Whoever did #3, you need to look at bug 80885.
No longer blocks: 54804
25 dups and no mostfreq keyword (yes!). Marking.
Keywords: mostfreq
Jacek, you probably changed the keywords without reading the entire bug. That's
understandable, which is why there is no mostfreq keyword.

-helpwanted, -mostfreq.

To paraphrase the marketing people for the NBA on TNT:

"Upgrade or go home."
Keywords: helpwanted, mostfreq
I believ this bug should be closed. If you think it needs to be open still,
please assign to the corect engineer, it is certainly not me!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
So close it :-)
Fixed is good enough for me... 
VERIFIED:

We are outta here!

(if you know who checked in the majority of the functional code, please assign
the bug to them, so it is correct for the record).
Status: RESOLVED → VERIFIED
Blocks: 47993
Depends on: 398077
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: