RFC: Proxy support for IcedTea Java Plugin

[Andrew John Hughes]

2009/3/16 Deepak Bhole <dbhole at redhat.com>:
> * Andrew John Hughes <gnu_andrew at member.fsf.org> [2009-03-13 19:07]:
>> 2009/3/13 Deepak Bhole <dbhole at redhat.com>:
>> > Hi,
>> >
>> > Attached patch adds proxy support to the IcedTea plugin.
>> >
>> > With this patch, the plugin can now read proxy settings from the browser
>> > and use those to connect to the remote server for fetching jars,
>> > applet communication (SOCKS proxy), etc. The patch also adds support
>> > for proxy (and web) servers that require http authentication by
>> > displaying a credential input dialog to the user.
>> >
>>
>> Haven't tested this so these comments are just from reading the code.
>> Someone else should (maybe the user who reported this issue?), and I
>> hope you have! Generally looks ok.  Main issue I can see is that the
>> password is being output several times to the console.  This should be
>> removed.
>
> That was only in inactive WIP code. I removed all print lines for
> credentials from live code. Since the unused code is gone, this is no
> longer an issue. Auth info is now printed from the Main method of
> PasswordAuthenticationDialog only, which is never called from plugin code.
> PasswordAuthenticationDialog::main() is a testing only function, so it is
> okay there.
>

Ah good.  I wasn't sure which bits were active and inactive before, so
the dead code was also making it a bit harder to review! Much easier
this time :)

>> Few other minor issues:
>> * Use a StringBuilder rather than a StringBuffer to construct the
>> String in CustomAuthenticator.  StringBuffer has unnecessary
>> synchronisation.
>
> This was in the unused part of the code, so it is gone now. Good to know
> about StringBuffer vs Builder though.. I'll use that in the future :)
>

I imagine HotSpot is clever enough to use escape analysis and get rid
of the synchronisation, but it's more reliable to just StringBuilder.

>> * A few lines are a bit too long and should have line breaks to make
>> the code easier to read.
>
> Very little of the plugin code is line wrapped and I didn't want
> to mix wrapped/unwrapped, so I did not wrap at all. Is the 80 char
> limit part of the general guidelines? I don't mind doing it... just
> didn't seem like something worth going out of the way for.
>

I don't think we have any official guidelines for IcedTea as such...
I tend to go by the GNU Classpath ones, which do include an 80 char limit.
But as I said, these are just minor things - I certainly wouldn't go
to the length of changing all the plugin code unless you have a lot of
time on your hands!
That said, making it worse doesn't help either and there were just one
or two places
in this code that had an issue.

>>
>> > Initially I also started adding support for fetching cached authentication
>> > information from mozilla directly, but then I decided to disable it as
>> > it felt like a security risk to pass usernames/passwords over a FIFO
>> > pipe. If the communication system is changed in the future, that code
>> > can be enabled again. It is about 80% done.
>> >
>>
>> Can you remove this dead code from the patch and keep it to one-side
>> separately?  It's a bad idea to have dead code hanging around, and
>> worse when it's potentially open for exploit like this.
>>
>
> Sure, I have removed it all.
>

Thanks!

>> > In addition, there are a few minor fixes here and there -- see ChangeLog
>> > diff for more details.
>> >
>>
>> Please do these in a separate patch.  It's easier to track down bugs
>> later if each changeset only makes one change.  They are trivial
>> enough to just go straight in.
>>
>
> Normally that is what I'd do, but in this case they are needed for
> proxy support to work properly (e.g. removing the time wait fix will
> cause the browser to hang if one cancels plugin auth dialog..).
>

That's fine, then.  That wasn't clear from your original mail.

>> As Andrew already mentioned, you don't include the ChangeLog in the
>> patch itself but rather in the email, as it generally won't apply
>> locally for other users.
>>
>
> Oops, will keep it in mind next time.
>

You'll get used to doing it ;)

> New patch (sans changelog and dead code) attached.
>

Looks ok, just spotted one minor issue:

+            Long now = new Date().getTime();
+
+            if (super.containsKey(key)) {
+                Long age = now - timeStamps.get(key);

Why Long and not long? This would seem to do an unnecessary amount of
boxing and unboxing:

Long now = Long.valueOf(new Date().getTime());

if (super.containsKey(key))
{
  Long age = Long.valueOf(now.longValue() - timeStamps.get(key).longValue());

whereas with a long you'd only be using longValue() on the return
value of timeStamps.get(key).

> Thanks for reviewing!
>

No problem! :)

> Cheers,
> Deepak
>

Cheers,
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8