76921 - nsPluginHostImpl::FindProxyForURL() messed up by PAC service.

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[edburns]

Using Trunk build from Friday 0100 hours I find that visiting java.sun.com on 
win32 and linux causes the applets to never show up.  The grey boxes appear, 
and action appears to take place in them, but the applets never show up.

This is a really bad bug.

Comment 1

[edburns]

Xiaobin, can you please try to duplicate this on your machine?

Comment 2

[Xiaobin Lu]

Yes, I can reproduce it in my systems (WINNT + jre1.3). The control panel looks 
like some classes is not loaded. I think it is the side effect of lazy loading 
problem.


load: class jug2 not found.
java.lang.ClassNotFoundException: java.net.ConnectException: Operation timed 
out: connect
	at java.net.PlainSocketImpl.socketConnect(Native Method)
	at java.net.PlainSocketImpl.doConnect(Unknown Source)
	at java.net.PlainSocketImpl.connectToAddress(Unknown Source)
	at java.net.PlainSocketImpl.connect(Unknown Source)
	at java.net.Socket.<init>(Unknown Source)
	at java.net.Socket.<init>(Unknown Source)
	at sun.net.NetworkClient.doConnect(Unknown Source)
	at sun.plugin.protocol.jdk12.http.HttpClient.doConnect
(HttpClient.java:97)
	at sun.net.www.http.HttpClient.openServer(Unknown Source)
	at sun.net.www.http.HttpClient.openServer(Unknown Source)
	at sun.net.www.http.HttpClient.<init>(Unknown Source)
	at sun.net.www.http.HttpClient.<init>(Unknown Source)
	at sun.plugin.protocol.jdk12.http.HttpClient.<init>(HttpClient.java:57)
	at sun.plugin.protocol.jdk12.http.HttpClient.New(HttpClient.java:69)
	at sun.plugin.protocol.jdk12.http.HttpURLConnection.privBlock
(HttpURLConnection.java:113)
	at 
sun.plugin.protocol.jdk12.http.HttpURLConnection$PrivilegedBlockAction.run
(HttpURLConnection.java:402)
	at java.security.AccessController.doPrivileged(Native Method)
	at sun.plugin.protocol.jdk12.http.HttpURLConnection.connect
(HttpURLConnection.java:148)
	at sun.plugin.protocol.jdk12.http.HttpURLConnection.getInputStream
(HttpURLConnection.java:321)
	at java.net.HttpURLConnection.getResponseCode(Unknown Source)
	at sun.applet.AppletClassLoader.getBytes(Unknown Source)
	at sun.applet.AppletClassLoader.access$100(Unknown Source)
	at sun.applet.AppletClassLoader$1.run(Unknown Source)
	at java.security.AccessController.doPrivileged(Native Method)
	at sun.applet.AppletClassLoader.findClass(Unknown Source)
	at sun.plugin.security.PluginClassLoader.findClass
(PluginClassLoader.java:239)
	at java.lang.ClassLoader.loadClass(Unknown Source)
	at sun.applet.AppletClassLoader.loadClass(Unknown Source)
	at java.lang.ClassLoader.loadClass(Unknown Source)
	at sun.applet.AppletClassLoader.loadCode(Unknown Source)
	at sun.applet.AppletPanel.createApplet(Unknown Source)
	at sun.plugin.AppletViewer.createApplet(AppletViewer.java:1178)
	at sun.applet.AppletPanel.runLoader(Unknown Source)
	at sun.applet.AppletPanel.run(Unknown Source)
	at java.lang.Thread.run(Unknown Source)

Comment 3

[Xiaobin Lu]

   For Linux debug build of mozilla without JRE installed, go to java.sun.com 
will make the browser crash.  

Comment 4

[edburns]

I'm adding Beard.  He was key in solving the other problem having to due with 
lazy loading, bug 76677.  

Patrick, do you have any clues about why this would be happening?

Comment 5

[edburns]

I see this on Linux, I've opened bug 77201 and made this bug depend on that.

Comment 6

[edburns]

Bugzilla doesn't update dependencies when a bug is marked dup.

Comment 7

[edburns]

Well, I don't think this is a problem with external applets.  If I go to 
<http://webmirror.eng.sun.com>, which is the internal mirror of the external 
site <http://java.sun.com> I get the same result.  

I've tried this with JDK1.3.0_01 and JDK1.3.1 rc 1, same result.  Here's the 
java console output:

Opening http://192.18.97.80/javanews/classes/news.jar

Connecting http://192.18.97.80/javanews/classes/news.jar with no proxy

Connecting http://192.18.97.80/javanews/classes/news.jar with no proxy

Opening http://192.18.97.137/testdev/javanews/jug2.class

Connecting http://192.18.97.137/testdev/javanews/jug2.class with no proxy

Opening http://192.18.97.80/javanews/classes/news.zip

Connecting http://192.18.97.80/javanews/classes/news.zip with no proxy

load: class jug2 not found.

java.lang.ClassNotFoundException: java.net.NoRouteToHostException: Operation 
timed out: no further information

	at java.net.PlainSocketImpl.socketConnect(Native Method)

	at java.net.PlainSocketImpl.doConnect(Unknown Source)

	at java.net.PlainSocketImpl.connectToAddress(Unknown Source)

	at java.net.PlainSocketImpl.connect(Unknown Source)

	at java.net.Socket.<init>(Unknown Source)

	at java.net.Socket.<init>(Unknown Source)

	at sun.net.NetworkClient.doConnect(Unknown Source)

	at sun.plugin.protocol.jdk12.http.HttpClient.doConnect(Unknown Source)

	at sun.net.www.http.HttpClient.openServer(Unknown Source)

	at sun.net.www.http.HttpClient.openServer(Unknown Source)

	at sun.net.www.http.HttpClient.<init>(Unknown Source)

	at sun.net.www.http.HttpClient.<init>(Unknown Source)

	at sun.plugin.protocol.jdk12.http.HttpClient.<init>(Unknown Source)

	at sun.plugin.protocol.jdk12.http.HttpClient.New(Unknown Source)

	at sun.plugin.protocol.jdk12.http.HttpURLConnection.privBlock(Unknown 
Source)

	at 
sun.plugin.protocol.jdk12.http.HttpURLConnection$PrivilegedBlockAction.run
(Unknown Source)

	at java.security.AccessController.doPrivileged(Native Method)

	at sun.plugin.protocol.jdk12.http.HttpURLConnection.connectStep2
(Unknown Source)

	at sun.plugin.protocol.jdk12.http.HttpURLConnection.connect(Unknown 
Source)

	at sun.plugin.protocol.jdk12.http.HttpURLConnection.getInputStream
(Unknown Source)

	at java.net.HttpURLConnection.getResponseCode(Unknown Source)

	at sun.applet.AppletClassLoader.getBytes(Unknown Source)

	at sun.applet.AppletClassLoader.access$100(Unknown Source)

	at sun.applet.AppletClassLoader$1.run(Unknown Source)

	at java.security.AccessController.doPrivileged(Native Method)

	at sun.applet.AppletClassLoader.findClass(Unknown Source)

	at sun.plugin.security.PluginClassLoader.findClass(Unknown Source)

	at java.lang.ClassLoader.loadClass(Unknown Source)

	at sun.applet.AppletClassLoader.loadClass(Unknown Source)

	at java.lang.ClassLoader.loadClass(Unknown Source)

	at sun.applet.AppletClassLoader.loadCode(Unknown Source)

	at sun.applet.AppletPanel.createApplet(Unknown Source)

	at sun.plugin.AppletViewer.createApplet(Unknown Source)

	at sun.applet.AppletPanel.runLoader(Unknown Source)

	at sun.applet.AppletPanel.run(Unknown Source)

	at java.lang.Thread.run(Unknown Source)

Connecting http://192.18.97.80/javanews/classes/news.zip with no proxy

Comment 8

[edburns]

It seems the problem is in the Java Plugin's ability to load class files.  
Stanley, I'm stuck on this, can you please help?

Comment 9

[Xiaobin Lu]

   I believe that there are something wrong with resolving java class. I also 
got the same ClassNotFound exception with some other applets used to work.

Comment 10

[stanley.ho]

The errir occurs because the plug-in is not able to connect 192.18.97.80 
without proxy. This is either a DNS problem, or incorrect proxy is used.

Comment 11

[edburns]

It turns out that ProxyAutoConfig (PAC) is to blame for this bug.  It turns out 
that The ProxyAutoConfig service says that URL 
<http://192.18.97.137/testdev/javanews/jug2.class> should get DIRECT treatment, 
even when the http proxy is set in the prefs.

Comment 12

[Bradley Baetz]

I talked about this on IRC with edburns, and from what he said, I think that
there are two problems:

1. The js autoproxy stuff doesn't appear to return an error. It probably should.

2. However, that code could probably be using the protocol proxy service (see
nsIProtocolProxyService.idl). (Just because someone has a valid pac in their
prefs doesn't mean that it is enabled.) That way you get all the pref behaviour
(and any future changes, I suppose) for free. You can probably do this without
changing the ABI of nsPluginHostImpl.cpp by just creating a local class which
implements nsIProxy, and use that for the callbacks.

I did this for gopher in nsGopherHandler.cpp, (although I used the channel as
the nsIProxy, not a separate class) but the proxy stuff I did was basically a
copy of the ftp stuff.

Then use the results of that, and return "PROXY hostname:port" based on what the
service returns, or "DIRECT", or do whatever socks proxies are meant to report.

You'll only get one proxy server that way, but thats all that the current plugin
code does (according to the comment), and all that the current PAC code handles
at the moment.

From the cvs logs, it looks like this code was written before the protocol proxy
service was present, so I don't know if there is a reason not to use it. You
should probably check with some necko people though.

Comment 13

[drapeau]

Copying some folks who worked on and are interested in bug 53080, since recent 
comments claim that 53080 causes this bug.  Also, setting this bug dependent on 
53080; please remove the dependency if I'm just wrong about that.

Comment 14

[Bradley Baetz]

I suspect that before this, the pac service just always failed.

Also, this bug is only for network applets which have to go through a proxy not
configured by pac, right?

Comment 15

[edburns]

It's for any plugin that wants to find out the proxy for a url.  This bug is 
not specific to applets.  Changing component to Plug-ins.

Comment 16

[edburns]

Change Summary.

Comment 17

[edburns]

Attachment: cvs diff -u of Approach 1: non-invasive easy fix, first iteration

Comment 18

[edburns]

Attachment: cvs diff -u of Approach 2: more invasive, right fix (bbaetz: uses nsIProxyProtocolService), first iteration

Comment 19

[edburns]

I have attached two fixes to this bug.  

Approach 1: minimal code modification, fix is to check for proxyType ==
2 for PAC.

Approach 2: new class nsPluginProxyImpl extends nsIProxy.  Use
nsIProtocolProxyService.

I've tested both approaches on win32 and Linux.  I prefer approach 2.

Ed

Comment 20

[Denis Antrushin]

Stupid question: does do_GetService load fresh component even if there's 
already one? Why I'm asking is because PAC component return DIRECT upon
first call (when it sees that it hasn't loaded PAC yet).
PAC component usually get loaded at startup time, so at the time you can
ask for any URL, proxy stuff is already there. I wonder, why this is not the
case with plugins. (Sorry, I don't know much about mozilla internals :-))

Comment 21

[edburns]

I have r=peterczynski for approach #2, the new class that implements nsIProxy.

Comment 22

[edburns]

bbaetz pointed out Approach 2 first iteration leaks proxyHost if the host get 
succeeds but the port get fails.  I'll fix this in a subsequent patch.

Comment 23

[edburns]

<bbaetz> So:
<bbaetz> +  if (NS_SUCCEEDED(protocolProxy.GetProxyHost(&proxyHost))) {
<bbaetz> becauses:
<bbaetz> er, becomes:
<bbaetz> nsXPIDLCString proxyHost;
<bbaetz> if (NS_SUCCEEDED(protocolProxy.GetProxyHost(getter_Copies(proxyHost))) 
{

Comment 24

[edburns]

<edburns> bbaetz: can you give r= as well?
<edburns> Assuming I fix the string leak of course.
<bbaetz> edburns: yeah, r=bbaetz with those changes I mentioned

Comment 25

[Darin Fisher]

sr=darin

however, you ought to replace PR_smprintf("DIRECT") with PL_strdup("DIRECT")

Comment 26

[Terry Noyes]

Attachment: plugin.mcp (Mac) which builds plugin.shlb using new nsPluginProxyImpl.cpp

Comment 27

[edburns]

FIx checked in.

Comment 28

[edburns]

From bug 76936:

------- Additional Comments From Asa Dotzler 2001-05-02 20:56 -------

a= [e-mail] for checkin to both this bug and 76921

Comment 29

[Dawn Endico]

Blizzard found that patch is causing crash on exit on installations that
have java installed. We're holding the milestone yet again for this. Should
this be backed out or fixed? Blizzard is investigating, but the more eyeballs,
the merrirer.


09:39:13] <blizzard> my crash is in /usr/lib/mozilla/components/libgkplugin.so
[09:39:34] <blizzard> #0  0x400d79d1 in nsCOMPtr_base::~nsCOMPtr_base () at
eval.c:41
[09:39:34] <blizzard> #1  0x41445a26 in NSGetModule ()
[09:39:34] <blizzard>    from /usr/lib/mozilla/components/libgkplugin.so
[09:39:48] <blizzard> NSGetModule() is bogus but it's definitely in there
[09:40:03] --- blizzard is now known as |
[09:40:05] <|> #0  0x400d79d1 in nsCOMPtr_base::~nsCOMPtr_base () at eval.c:41
[09:40:05] <|> #1  0x41445a26 in NSGetModule ()
[09:40:05] <|>    from /usr/lib/mozilla/components/libgkplugin.so
[09:40:05] <|> #2  0x4144580b in NSGetModule ()
[09:40:05] <|>    from /usr/lib/mozilla/components/libgkplugin.so
[09:40:06] <|> #3  0x41448d0a in NSGetModule ()
[09:40:08] <|>    from /usr/lib/mozilla/components/libgkplugin.so
[09:40:10] <|> #4  0x4144ddd3 in NSGetModule ()
[09:40:12] <|>    from /usr/lib/mozilla/components/libgkplugin.so
[09:40:14] <|> #5  0x400a2810 in nsObserverService::Notify () at eval.c:41
[09:40:16] <|> #6  0x400e223e in XPTC_InvokeByIndex () at eval.c:41
[09:40:18] <|> #7  0x4089242c in NSGetModule ()
[09:40:20] <|>    from /usr/lib/mozilla/components/libxpconnect.so
[09:40:22] <|> #8  0x408936a2 in NSGetModule ()
[09:40:24] <|>    from /usr/lib/mozilla/components/libxpconnect.so
[09:40:26] <|> #9  0x40142e7b in js_Invoke () at eval.c:41
[09:40:28] <|> #10 0x4014a537 in js_Interpret () at eval.c:41
[09:40:30] <|> #11 0x40142ed3 in js_Invoke () at eval.c:41



09:57:53] <brendan> those new files are boring and silly; only bug i see there
is that the Set methods taking strings blow away the old string before
strdup'ing the new -- if strdup fails, you're left with no string
[09:58:29] <blizzard> brendan: assuming the stack trace is worth anything it's
crashing in nsCOMPtr::~nsCOMPtr
[09:58:37] <blizzard> brendan: which means that he's releasing a bad pointer
somewhere
[10:03:23] <brendan> blizzard: right, it means refcount underflow, i think
[10:03:31] <brendan> or just corruption

Comment 30

[edburns]

Wow, I'll look at this top priority right now.

Comment 31

[edburns]

Attachment: cvs diff -u of fix for string free ordering, first iteration.

Comment 32

[Brendan Eich [:brendan]]

Better, but let me point out that you set rc (usually it's called rv, that's the
"canonical name") to NS_ERROR_FAILURE, then set it in two places to NS_OK.  It
seems better to me to set it optimistically to NS_OK, and then in only one place
(for a tiny win in code size) set it to NS_ERROR_FAILURE.

Shouldn't it really be NS_ERROR_OUT_OF_MEMORY?  I think so.

/be

Comment 33

[edburns]

Just FYI, the crash on exit doesn't appear in win32.

Comment 34

[edburns]

Attachment: cvs diff -u of string free ordering, second iteration.

Comment 35

[edburns]

For the record, lest someone else get confused.  This crash on exit has nothing 
to do with bug 76921.  

Comment 36

[Brendan Eich [:brendan]]

Please use NS_ERROR_OUT_OF_MEMORY (and optimal NS_OK) in the getters too.

Note also that you should overparenthesize assignment expressions nested in
conditions, to abate GCC warnings about possible == typos.  So

    if ((newProxyHost = nsCRT::strdup(aProxyHost))) {

rather than

    if (newProxyHost = nsCRT::strdup(aProxyHost)) {

Fix these things, and r/sr=[e-mail].

/be

Comment 37

[edburns]

I'd like to open another bug on this "crash on exit with java" problem.

Comment 38

[edburns]

For the record, rolling back this checkin will *not* solve the crash on exit 
problem.  I was observing this problem before my fix to this bug.

My $0.02 says that this crash on exit with java should not hold up 0.9.

Another $0.02 says that this crash on exit is related to Patrick Beard's lazy 
loading fix for bug 26516.  I started seeing the crash on exit around that time.

Comment 39

[edburns]

<edburns> Can I close this bug?
<blizzard> lol
<blizzard> yeah
<blizzard> go ahead
Turns out this isn't the problem.

re-closing.

Comment 40

[shrirang khanzode]

v