[9] Review request for 8032832: Applet/browser deadlocks, when IIS integrated authentication is used

Discussion:

Anton Litvinov

2014-03-20 14:08:35 UTC

Hello,

Could you please review the following fix for the bug, which consists in
a deadlock between 2 threads sharing the same instance of
"java.net.URLClassLoader" as the context class loader, calling
"sun.net.www.protocol.http.NegotiateAuthentication.isSupported" method
and loading a class file from the web server with the enabled
"Negotiate" authentication.

Bug: https://bugs.openjdk.java.net/browse/JDK-8032832
Webrev: http://cr.openjdk.java.net/~alitvinov/8032832/jdk9/webrev.00

The fix is based on supplementing the method
"sun.net.www.protocol.http.NegotiateAuthentication.isSupported" with the
code locking on the instance of the class loader before existing locking
on the class "sun.net.www.protocol.http.NegotiateAuthentication".

Thank you,
Anton

Chris Hegarty

2014-03-24 16:38:35 UTC

Permalink

Hi Anton,

I don't have any objections to this, but Max is the original author of
this code and may want to take a closer look.

-Chris.

Post by Anton Litvinov
Hello,
Could you please review the following fix for the bug, which consists in
a deadlock between 2 threads sharing the same instance of
"java.net.URLClassLoader" as the context class loader, calling
"sun.net.www.protocol.http.NegotiateAuthentication.isSupported" method
and loading a class file from the web server with the enabled
"Negotiate" authentication.
Bug: https://bugs.openjdk.java.net/browse/JDK-8032832
Webrev: http://cr.openjdk.java.net/~alitvinov/8032832/jdk9/webrev.00
The fix is based on supplementing the method
"sun.net.www.protocol.http.NegotiateAuthentication.isSupported" with the
code locking on the instance of the class loader before existing locking
on the class "sun.net.www.protocol.http.NegotiateAuthentication".
Thank you,
Anton

Anton Litvinov

2014-03-25 10:48:49 UTC

Permalink

Hello Chris,

Thank you very much for review of this fix and for addressing the review
request to the second possible reviewer. I am looking forward to
receiving any response from Weijun Wang. Also I would like to inform you
that jtreg regression tests specified below (both open and internal
proprietary) were run on all 4 supported platforms: MS Windows, Linux,
Solaris, OS X. The results of regression tests were almost identical on
on all the platforms, however, no negative changes introduced by the fix
were found.

Executed regression tests:
- "jdk/test/java/net"
- "jdk/test/sun/net"

Thank you,
Anton

Post by Chris Hegarty
Hi Anton,
I don't have any objections to this, but Max is the original author of
this code and may want to take a closer look.
-Chris.

Wang Weijun

2014-03-25 11:37:52 UTC

Permalink

The fix looks fine. You might want to remove the cache-related words from the comment of the new isSupported method.

--Max

Post by Anton Litvinov
Hello Chris,
Thank you very much for review of this fix and for addressing the review request to the second possible reviewer. I am looking forward to receiving any response from Weijun Wang. Also I would like to inform you that jtreg regression tests specified below (both open and internal proprietary) were run on all 4 supported platforms: MS Windows, Linux, Solaris, OS X. The results of regression tests were almost identical on on all the platforms, however, no negative changes introduced by the fix were found.
- "jdk/test/java/net"
- "jdk/test/sun/net"
Thank you,
Anton

Post by Chris Hegarty
Hi Anton,
I don't have any objections to this, but Max is the original author of this code and may want to take a closer look.
-Chris.

Anton Litvinov

2014-03-25 12:27:27 UTC

Permalink

Hello Max,

Thank you for review and approval of the fix. The sentence containing
cache-related words is removed from the comment of the new "isSupported"
method. The edited version of the fix is following.

Webrev: http://cr.openjdk.java.net/~alitvinov/8032832/jdk9/webrev.01

Thank you,
Anton

Post by Wang Weijun
The fix looks fine. You might want to remove the cache-related words from the comment of the new isSupported method.
--Max

Post by Chris Hegarty
Hi Anton,
I don't have any objections to this, but Max is the original author of this code and may want to take a closer look.
-Chris.