Ensure server preference order in ALPN

Motivation:
With the current implementation the client protocol preference list
takes precedence over the one of the server, since the select method
will return the first item, in the client list, that matches any of the
protocols supported by the server. This violates the recommendation of
http://tools.ietf.org/html/rfc7301#section-3.2.

It will also fail with the current implementation of Chrome, which
sends back Extension application_layer_protocol_negotiation, protocols:
[http/1.1, spdy/3.1, h2-14]

Modifications:
Changed the protocol negotiator to prefer server’s list. Added a test
case that demonstrates the issue and that is fixed with the
modifications of this commit.

Result:
Server’s preference list is used.

handler/src/main/java/io/netty/handler/ssl/JdkAlpnSslEngine.java

@@ -20,7 +20,7 @@ import io.netty.handler.ssl.JdkApplicationProtocolNegotiator.ProtocolSelectionLi
 import io.netty.handler.ssl.JdkApplicationProtocolNegotiator.ProtocolSelector;
 import io.netty.util.internal.PlatformDependent;
 
-import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
 
 import javax.net.ssl.SSLEngine;
@@ -64,7 +64,8 @@ final class JdkAlpnSslEngine extends JdkSslEngine {
 
         if (server) {
             final ProtocolSelector protocolSelector = checkNotNull(applicationNegotiator.protocolSelectorFactory()
-                    .newSelector(this, new HashSet<String>(applicationNegotiator.protocols())), "protocolSelector");
+                    .newSelector(this, new LinkedHashSet<String>(applicationNegotiator.protocols())),
+                    "protocolSelector");
             ALPN.put(engine, new ServerProvider() {
                 @Override
                 public String select(List<String> protocols) {

handler/src/main/java/io/netty/handler/ssl/JdkBaseApplicationProtocolNegotiator.java

@@ -142,9 +142,8 @@ class JdkBaseApplicationProtocolNegotiator implements JdkApplicationProtocolNego
 
         @Override
         public String select(List<String> protocols) throws Exception {
-            for (int i = 0; i < protocols.size(); ++i) {
+            for (String p : supportedProtocols) {
-                String p = protocols.get(i);
+                if (protocols.contains(p)) {
-                if (supportedProtocols.contains(p)) {
                     jettyWrapper.getSession().setApplicationProtocol(p);
                     return p;
                 }

handler/src/main/java/io/netty/handler/ssl/JdkNpnSslEngine.java

@@ -21,7 +21,7 @@ import io.netty.handler.ssl.JdkApplicationProtocolNegotiator.ProtocolSelectionLi
 import io.netty.handler.ssl.JdkApplicationProtocolNegotiator.ProtocolSelector;
 import io.netty.util.internal.PlatformDependent;
 
-import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
 
 import javax.net.ssl.SSLEngine;
@@ -88,7 +88,8 @@ final class JdkNpnSslEngine extends JdkSslEngine {
             });
         } else {
             final ProtocolSelector protocolSelector = checkNotNull(applicationNegotiator.protocolSelectorFactory()
-                    .newSelector(this, new HashSet<String>(applicationNegotiator.protocols())), "protocolSelector");
+                    .newSelector(this, new LinkedHashSet<String>(applicationNegotiator.protocols())),
+                    "protocolSelector");
             NextProtoNego.put(engine, new ClientProvider() {
                 @Override
                 public boolean supports() {

handler/src/test/java/io/netty/handler/ssl/JdkSslEngineTest.java

@@ -17,6 +17,7 @@ package io.netty.handler.ssl;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeNoException;
 import static org.mockito.Mockito.verify;
@@ -61,7 +62,8 @@ import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 
 public class JdkSslEngineTest {
-    private static final String APPLICATION_LEVEL_PROTOCOL = "my-protocol";
+    private static final String PREFERRED_APPLICATION_LEVEL_PROTOCOL = "my-protocol-http2";
+    private static final String FALLBACK_APPLICATION_LEVEL_PROTOCOL = "my-protocol-http1_1";
     private static final String APPLICATION_LEVEL_PROTOCOL_NOT_COMPATIBLE = "my-protocol-FOO";
 
     @Mock
@@ -123,6 +125,7 @@ public class JdkSslEngineTest {
         clientChannel = null;
         serverChannel = null;
         serverConnectedChannel = null;
+        serverException = null;
     }
 
     @Test
@@ -135,7 +138,7 @@ public class JdkSslEngineTest {
                 throw new RuntimeException("NPN not on classpath");
             }
             JdkApplicationProtocolNegotiator apn = new JdkNpnApplicationProtocolNegotiator(true, true,
-                    APPLICATION_LEVEL_PROTOCOL);
+                    PREFERRED_APPLICATION_LEVEL_PROTOCOL);
             mySetup(apn);
             runTest();
         } catch (RuntimeException e) {
@@ -155,7 +158,7 @@ public class JdkSslEngineTest {
                 throw new RuntimeException("NPN not on classpath");
             }
             JdkApplicationProtocolNegotiator clientApn = new JdkNpnApplicationProtocolNegotiator(false, false,
-                    APPLICATION_LEVEL_PROTOCOL);
+                    PREFERRED_APPLICATION_LEVEL_PROTOCOL);
             JdkApplicationProtocolNegotiator serverApn = new JdkNpnApplicationProtocolNegotiator(false, false,
                     APPLICATION_LEVEL_PROTOCOL_NOT_COMPATIBLE);
             mySetup(serverApn, clientApn);
@@ -177,7 +180,7 @@ public class JdkSslEngineTest {
                 throw new RuntimeException("NPN not on classpath");
             }
             JdkApplicationProtocolNegotiator clientApn = new JdkNpnApplicationProtocolNegotiator(true, true,
-                    APPLICATION_LEVEL_PROTOCOL);
+                    PREFERRED_APPLICATION_LEVEL_PROTOCOL);
             JdkApplicationProtocolNegotiator serverApn = new JdkNpnApplicationProtocolNegotiator(false, false,
                     APPLICATION_LEVEL_PROTOCOL_NOT_COMPATIBLE);
             mySetup(serverApn, clientApn);
@@ -200,7 +203,7 @@ public class JdkSslEngineTest {
                 throw new RuntimeException("NPN not on classpath");
             }
             JdkApplicationProtocolNegotiator clientApn = new JdkNpnApplicationProtocolNegotiator(false, false,
-                    APPLICATION_LEVEL_PROTOCOL);
+                    PREFERRED_APPLICATION_LEVEL_PROTOCOL);
             JdkApplicationProtocolNegotiator serverApn = new JdkNpnApplicationProtocolNegotiator(true, true,
                     APPLICATION_LEVEL_PROTOCOL_NOT_COMPATIBLE);
             mySetup(serverApn, clientApn);
@@ -223,7 +226,7 @@ public class JdkSslEngineTest {
                 throw new RuntimeException("ALPN not on classpath");
             }
             JdkApplicationProtocolNegotiator apn = new JdkAlpnApplicationProtocolNegotiator(true, true,
-                    APPLICATION_LEVEL_PROTOCOL);
+                    PREFERRED_APPLICATION_LEVEL_PROTOCOL);
             mySetup(apn);
             runTest();
         } catch (Exception e) {
@@ -243,7 +246,7 @@ public class JdkSslEngineTest {
                 throw new RuntimeException("ALPN not on classpath");
             }
             JdkApplicationProtocolNegotiator clientApn = new JdkAlpnApplicationProtocolNegotiator(false, false,
-                    APPLICATION_LEVEL_PROTOCOL);
+                    PREFERRED_APPLICATION_LEVEL_PROTOCOL);
             JdkApplicationProtocolNegotiator serverApn = new JdkAlpnApplicationProtocolNegotiator(false, false,
                     APPLICATION_LEVEL_PROTOCOL_NOT_COMPATIBLE);
             mySetup(serverApn, clientApn);
@@ -265,7 +268,7 @@ public class JdkSslEngineTest {
                 throw new RuntimeException("ALPN not on classpath");
             }
             JdkApplicationProtocolNegotiator clientApn = new JdkAlpnApplicationProtocolNegotiator(false, false,
-                    APPLICATION_LEVEL_PROTOCOL);
+                    PREFERRED_APPLICATION_LEVEL_PROTOCOL);
             JdkApplicationProtocolNegotiator serverApn = new JdkAlpnApplicationProtocolNegotiator(true, true,
                     APPLICATION_LEVEL_PROTOCOL_NOT_COMPATIBLE);
             mySetup(serverApn, clientApn);
@@ -278,6 +281,31 @@ public class JdkSslEngineTest {
         }
     }
 
+    @Test
+    public void testAlpnCompatibleProtocolsDifferentClientOrder() throws Exception {
+        try {
+            // Typical code will not have to check this, but will get a initialization error on class load.
+            // Check in this test just in case we have multiple tests that just the class and we already ignored the
+            // initialization error.
+            if (!JdkAlpnSslEngine.isAvailable()) {
+                throw new RuntimeException("ALPN not on classpath");
+            }
+            // Even the preferred application protocol appears second in the client's list, it will be picked
+            // because it's the first one on server's list.
+            JdkApplicationProtocolNegotiator clientApn = new JdkAlpnApplicationProtocolNegotiator(false, false,
+                FALLBACK_APPLICATION_LEVEL_PROTOCOL, PREFERRED_APPLICATION_LEVEL_PROTOCOL);
+            JdkApplicationProtocolNegotiator serverApn = new JdkAlpnApplicationProtocolNegotiator(true, true,
+                PREFERRED_APPLICATION_LEVEL_PROTOCOL, FALLBACK_APPLICATION_LEVEL_PROTOCOL);
+            mySetup(serverApn, clientApn);
+            assertNull(serverException);
+            runTest(PREFERRED_APPLICATION_LEVEL_PROTOCOL);
+        } catch (Exception e) {
+            // ALPN availability is dependent on the java version. If ALPN is not available because of
+            // java version incompatibility don't fail the test, but instead just skip the test
+            assumeNoException(e);
+        }
+    }
+
     @Test
     public void testAlpnNoCompatibleProtocolsClientHandshakeFailure() throws Exception {
         try {
@@ -288,7 +316,7 @@ public class JdkSslEngineTest {
                 throw new RuntimeException("ALPN not on classpath");
             }
             JdkApplicationProtocolNegotiator clientApn = new JdkAlpnApplicationProtocolNegotiator(true, true,
-                    APPLICATION_LEVEL_PROTOCOL);
+                    PREFERRED_APPLICATION_LEVEL_PROTOCOL);
             JdkApplicationProtocolNegotiator serverApn = new JdkAlpnApplicationProtocolNegotiator(
                     new ProtocolSelectorFactory() {
                         @Override
@@ -514,7 +542,7 @@ public class JdkSslEngineTest {
     }
 
     private void runTest() throws Exception {
-        runTest(APPLICATION_LEVEL_PROTOCOL);
+        runTest(PREFERRED_APPLICATION_LEVEL_PROTOCOL);
     }
 
     private void runTest(String expectedApplicationProtocol) throws Exception {