[icedtea-web] RFC: add proxy auto config support
Omair Majid
omajid at redhat.com
Fri Feb 25 07:38:09 PST 2011
On 02/17/2011 05:16 PM, Dr Andrew John Hughes wrote:
> On 15:55 Wed 16 Feb , Omair Majid wrote:
>> On 02/16/2011 02:51 PM, Dr Andrew John Hughes wrote:
>>> On 14:21 Wed 16 Feb , Omair Majid wrote:
>>>> The attached patch adds support for reading, parsing and using proxy
>>>> auto config (PAC) files to icedtea-web.
>>>>
>>>> PAC files are written in javascript, so this patch also adds a
>>>> dependency on rhino for building. A dependency on jrunscript is also
>>>> added for testing.
>>>>
>>>> PAC files will be used by both plugin and netx if deployment.proxy.type
>>>> is 2 (PAC) and deployment.proxy.auto.config.url points to a valid PAC
>>>> file. PAC files may also be used by netx if deployment.proxy.type is 3
>>>> (browser) and the browser (firefox) is using a proxy auto config file
>>>> for resolving proxies.
>>>>
>>>> The PAC file is evaluated inside a sandbox - which only has permissions
>>>> to resolve urls. Because we dont want to grant createclassloader
>>>> permissions, I have had to set (in ProxyAutoConfig.java):
>>>> cx.setOptimizationLevel(-1);
>>>> which works without creating classes (and so works without
>>>> createClassLoader permissions). This option is not available by going
>>>> through the Java ScriptEngine apis. The patch therefore uses the rhino
>>>> apis directly.
>>>>
>>>> Since parsing a PAC file requires access to a set of additional
>>>> functions, the patch also implements them. Tests for these are included.
>>>>
>>>> make check should run these tests and end by printing a line like:
>>>> X of Y tests failed
>>>>
>>>> Any thoughts or comments?
>>>>
Some further changes: the attached patch now works with or without rhino
support. If rhino is not available, it always returns "DIRECT" as the proxy.
>>> There seems to be a completely broken assumption that rhino.jar is going
>>> to be in the JDK??? It should detect it as IcedTea does.
>>>
>>
>> I think I do. The check for sun.org.mozilla.javascript.Context that you
>> have pointed out later is a check for rhino. And rhino.jar is in the JDK
>> (at least it appears on icedtea6's default builds).
>
> It might be in IcedTea. It is not in any arbitrary JDK. We are trying to
> break ties to IcedTea, not introduce more.
>
> We should be using the system Rhino which has org.mozilla.javascript.Context.
>
Done. The attached patches uses the system rhino (if available).
>>
>>> What is jrunscript? I've never heard of this before.
>>>
>>
>> It's a tool under JDK_HOME/bin (in icedtea6 builds). It's a front end to
>> rhino's shell. The patch I posed uses it to run javascript though the shell.
>>
>
> Right, ok. Not heard of this before.
> Does Rhino not provide it itself? Or is it only in the JDK?
>
Hm... Rhino does provide a binary named rhino (in /usr/bin on my system)
that appears to be the same thing. In the attached patch though, I
create a simple wrapper script (also named jrunscript) that just invokes
the rhino Main class.
>>>> + if [ -e $(SYSTEM_JDK_DIR)/jre/lib/rhino.jar ] ; then \
>>>> + ln -s $(SYSTEM_JDK_DIR)/jre/lib/rhino.jar $(BOOT_DIR)/jre/lib ; \
>>>> + else \
>>>> + ln -s rt.jar $(BOOT_DIR)/jre/lib/rhino.jar ; \
>>>
>>> This is even more confusing. First you assume that rhino.jar is going to be in
>>> the JDK. If it isn't there, you then symlink to rt.jar???
>>>
>>
>> I do check for sun.org.mozilla.javascript.Context (that's one of the few
>> rhino classes we care about) in configure.
>>
>
> You want org.mozilla.javascript.Context. The sun prefixed version is some
> Oracle JDK specific thing.
>
>> What I wanted to express in the makefile/configure (and clearly failed
>> to) is this:
>> 1. rhino classes are required to build now (that's the configure check).
>
> Fine.
>
>> 2. rhino.jar should be included in the classpath if it exists. If
>> rhino.jar does not exist (but we found it's classes), it's probably
>> rt.jar. So symlink rhino.jar to it.
>
> No it's not probably rt.jar. Please just use the Rhino check from IcedTea.
>
Done.
>> 3. Use rhino.jar when trying to compile code.
>
> The problem with this is the hardcoded path, you need to know where the system
> rhino is and use that.
>
>>
>> Does that make sense?
>>
>
> It does, it's just going about it the wrong way.
>
Hopefully the attached patch does it the right way.
>>>> fi
>>>> ln -sf $(SYSTEM_JDK_DIR)/jre/lib/$(JRE_ARCH_DIR) \
>>>> $(BOOT_DIR)/jre/lib/&& \
>>>> diff -r cd1eda4f0d97 acinclude.m4
>>>> --- a/acinclude.m4 Tue Feb 15 11:01:01 2011 -0500
>>>> +++ b/acinclude.m4 Wed Feb 16 13:59:02 2011 -0500
>>>> @@ -250,6 +250,40 @@
>>>> AC_SUBST(ECJ_JAR)
>>>> ])
>>>>
>>>> +AC_DEFUN([FIND_JRUNSCRIPT],
>>>
>>> Missing prefix; should be IT_FIND_JRUNSCRIPT.
>>>
>>
>> I looked around acinclude.m4 and was slightly confused about the naming.
>> I will fix this and post an update.
>>
>
> The IT_ prefix is used to denote IcedTea macros.
> On second thoughts, this should really use ITW_ if it's IcedTea-Web-specific.
> The IT_ ones are imported from IcedTea6.
>
Well, the rhino check is from IT, so I have left it as IT_. The java
check is IT_ as it based on IT_FIND_JAVAC.
>>>> +[
>>>> + AC_MSG_CHECKING([for jrunscript])
>>>> + AC_ARG_WITH([jrunscript],
>>>> + [AS_HELP_STRING(--with-jrunscript,specify location of the jrunscript binary)],
>>>> + [
>>>> + if test -f "${withval}"; then
>>>> + JRUNSCRIPT="${withval}"
>>>> + fi
>>>> + ],
>>>
>>> You don't handle withval being yes or no, which is valid (generated by --with-jrunscript or
>>> --without-jrunscript).
>>>
>>
>> Thanks, I will fix this.
>>
The attached patch does not have --with-jrunscript.
>>>> + [
>>>> + JRUNSCRIPT=${SYSTEM_JDK_DIR}/bin/jrunscript
>>>> + ])
>>>> + if test -z "${JRUNSCRIPT}"; then
>>>> + for dir in /usr/lib/jvm/java-openjdk/bin /usr/lib/jvm/icedtea6/bin \
>>>> + /usr/lib/jvm/java-6-openjdk/bin /usr/lib/jvm/openjdk/bin \
>>>> + /usr/lib/jvm/java-icedtea/bin /usr/lib/jvm/java-gcj/bin \
>>>> + /usr/lib/jvm/gcj-jdk/bin ; do
>>>> + if test -e $dir/jrunscript; then
>>>> + JRUNSCRIPT=$dir/jrunscript
>>>> + break
>>>> + fi
>>>> + done
>>>> + if test -z "${JRUNSCRIPT}"; then
>>>> + JRUNSCRIPT=no
>>>> + fi
>>>> + fi
>>>> + AC_MSG_RESULT(${JRUNSCRIPT})
>>>> + if test "x${JRUNSCRIPT}" = "xno" ; then
>>>> + AC_MSG_WARN([cannot find a jrunscript, use --with-jrunscript])
>>>> + fi
>>>> + AC_SUBST(JRUNSCRIPT)
>>>> +])
>>>> +
>>>> AC_DEFUN_ONCE([IT_CHECK_PLUGIN],
>>>> [
>>>> AC_MSG_CHECKING([whether to build the browser plugin])
>>>> diff -r cd1eda4f0d97 configure.ac
>>>> --- a/configure.ac Tue Feb 15 11:01:01 2011 -0500
>>>> +++ b/configure.ac Wed Feb 16 13:59:02 2011 -0500
>>>> @@ -34,6 +34,7 @@
>>>> FIND_ECJ_JAR
>>>> IT_FIND_JAVADOC
>>>> AC_CONFIG_FILES([javac], [chmod +x javac])
>>>> +FIND_JRUNSCRIPT
>>>>
>>>> IT_SET_VERSION
>>>> IT_CHECK_XULRUNNER_VERSION
>>>> @@ -64,6 +65,7 @@
>>>> IT_CHECK_FOR_CLASS(JAVA_NET_COOKIEMANAGER, [java.net.CookieManager])
>>>> IT_CHECK_FOR_CLASS(JAVA_NET_HTTPCOOKIE, [java.net.HttpCookie])
>>>> IT_CHECK_FOR_CLASS(JAVA_NET_COOKIEHANDLER, [java.net.CookieHandler])
>>>> +IT_CHECK_FOR_CLASS(SUN_ORG_MOZILLA_JAVASCRIPT_CONTEXT, [sun.org.mozilla.javascript.Context])
>>>
>>> Ugh... we should be making this list shorter not adding to it.
>>> Why is this necessary? Why can't you use a standard class from the JDK or Rhino?
>>>
>>
>> This is rhino :) Or at least, rhino as icedtea6 ships it.
>
> Hacked up Rhino from the rewriter. Don't use.
>
Fixed to remove this check and use real rhino.
>>>> diff -r cd1eda4f0d97 netx/net/sourceforge/jnlp/resources/pac-funcs.js
>>>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>>> +++ b/netx/net/sourceforge/jnlp/resources/pac-funcs.js Wed Feb 16 13:59:02 2011 -0500
>>>> @@ -0,0 +1,830 @@
>>>> +/* pac-funcs.js
>>>> + Copyright (C) 2011 Red Hat, Inc.
>>>> +
>>>> +This file is part of IcedTea.
>>>> +
>>>> +IcedTea is free software; you can redistribute it and/or
>>>> +modify it under the terms of the GNU General Public License as published by
>>>> +the Free Software Foundation, version 2.
>>>> +
>>>
>>> Shouldn't these headers now say IcedTea-Web?
>>>
>>
>> I am just going with the flow. I can fix this one if you want. Do you
>> want me to fix all others too? /me shudders at the thought of a patch
>> that updates all headers
>
> Oracle love such header changes :-) It means all the backports break
> in really stupid ways...
>
> No, I don't think it's worth the hassle. It's still in the IcedTea superproject.
>
Ok, so I have left it unchanged.
>>>> +/*
>>>> + * These helper functions are required to be able to parse Proxy Auto Config
>>>> + * (PAC) files. PAC files will use these helper functions to decide the best
>>>> + * proxy for connecting to a host.
>>>> + *
>>>> + * This implementation is based on the description at:
>>>> + * http://web.archive.org/web/20060424005037/wp.netscape.com/eng/mozilla/2.0/relnotes/demo/proxy-live.html
>>>> + */
>>>
>>> Based on? To what extent? Is any of this code copied? Is it legal?
>>>
>>
>> There is no code provided at that url (well, there is some code that
>> shows how the functions might be used). Just function descriptions and
>> examples. I made sure not to use any of their examples. The javadocs
>> consist of descriptions of the functions in my own words.
>>
>> Also that url seems dead now :/ This works for me right now:
>> http://web.archive.org/web/20080406080210/http://wp.netscape.com/eng/mozilla/2.0/relnotes/demo/proxy-live.html
>>
>
> Ugh... Is there a more stable URL?
>
I havent been able to find one yet :/ Which is quite surprising given
that some current firefox documentation links to it.
>>>
>>>> +import sun.org.mozilla.javascript.Context;
>>>> +import sun.org.mozilla.javascript.Function;
>>>> +import sun.org.mozilla.javascript.Scriptable;
>>>
>>> Please don't add more sun.* dependencies.
>>>
>>
>> I only added it because it's rhino. If you are not comfortable having
>> rhino as a required dependency, I can try and make it optional.
>>
>
> The problem is, as mentioned above, you need to use proper Rhino not
> the hacked up version in IcedTea6.
>
Done. These dependencies are now org.mozilla.javascript.*.
>>>> diff -r cd1eda4f0d97 tests/netx/pac/pac-funcs-test.js
>>>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>>> +++ b/tests/netx/pac/pac-funcs-test.js Wed Feb 16 13:59:02 2011 -0500
>>>> @@ -0,0 +1,446 @@
>>>> +
>>>> +var ICEDTEA_CLASSPATH_ORG_IP = "208.78.240.231";
>>>> +var CLASSPATH_ORG_IP = "199.232.41.10";
>>>
>>> These should be able to be overridden. Same with others below.
>>>
>>
>> Do you mean make check should be setting the value of these constants?
>> Which others below are you talking about? I only see various 0 and 255
>> combinations below and they are netmasks.
>>
>
> Sorry I meant the use of 'icedtea.classpath.org'. It would be nice to pass
> these as arguments but it can wait until another patch.
>
I have left it unchanged for now.
>>>
>>> Needs work.
>>
>> Nice summary :) I posted this knowing it was (very likely) incomplete.
>> Thanks for your comments. They are always appreciated!
>>
>
> No worries, hope you don't mind my savaging of your code! ;-)
>
Savaging? I prefer to think of it as improving :)
Any thoughts on the attached patch? Also, do you have any links to good
documentation on AC_CONFIG_FILES and good practices for it? I would like
to use it if possible for build.properties but so far I dont have much
to go on.
Thanks,
Omair
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pac-support-07.patch
Type: text/x-patch
Size: 74909 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110225/d03483bc/pac-support-07.patch
More information about the distro-pkg-dev
mailing list