Avoid using the NewSessionPayload directly

This simplifies creating ActiveSessions since we now know
exactly which capabilities we're matching against. This
also avoids the remote end attempting to read the payload
more than once from the underlying HttpRequest. If we ever
manage to not store the entire freaking payload in there,
this will avoid some hard to debug issues.

Author: shs96c

java/server/src/com/thoughtworks/selenium/webdriven/WebDriverBackedSeleniumServlet.java

@@ -21,17 +21,18 @@
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableSet;
 
 import com.thoughtworks.selenium.CommandProcessor;
 import com.thoughtworks.selenium.SeleniumException;
 
 import org.openqa.selenium.remote.DesiredCapabilities;
+import org.openqa.selenium.remote.Dialect;
 import org.openqa.selenium.remote.SessionId;
 import org.openqa.selenium.remote.server.ActiveSession;
 import org.openqa.selenium.remote.server.ActiveSessionFactory;
 import org.openqa.selenium.remote.server.ActiveSessionListener;
 import org.openqa.selenium.remote.server.ActiveSessions;
-import org.openqa.selenium.remote.server.NewSessionPayload;
 import org.openqa.selenium.remote.server.WebDriverServlet;
 
 import java.io.IOException;
@@ -211,11 +212,11 @@ private void startNewSession(
       }
 
       try {
-        try (NewSessionPayload payload = NewSessionPayload.create(caps)) {
-          ActiveSession session = new ActiveSessionFactory().createSession(payload);
-          sessions.put(session);
-          sessionId = session.getId();
-        }
+        ActiveSession session = new ActiveSessionFactory().createSession(
+            ImmutableSet.copyOf(Dialect.values()),
+            caps);
+        sessions.put(session);
+        sessionId = session.getId();
       } catch (Exception e) {
         log("Unable to start session", e);
         sendError(

java/server/src/org/openqa/selenium/remote/server/ActiveSessionFactory.java

@@ -33,12 +33,14 @@
 import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.SessionNotCreatedException;
 import org.openqa.selenium.WebDriver;
+import org.openqa.selenium.remote.Dialect;
 
 import java.io.IOException;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Objects;
 import java.util.ServiceLoader;
+import java.util.Set;
 import java.util.function.Function;
 import java.util.function.Predicate;
 import java.util.logging.Logger;
@@ -176,17 +178,15 @@ private static Predicate<Capabilities> containsKey(Pattern pattern) {
     return toCompare -> toCompare.asMap().keySet().stream().anyMatch(pattern.asPredicate());
   }
 
-  public ActiveSession createSession(NewSessionPayload newSessionPayload) throws IOException {
-    return newSessionPayload.stream()
-        .peek(caps -> LOG.info("Capabilities are: " + caps))
-        // Grab any factories that claim to be able to build each capability
-        .flatMap(caps -> factories.entrySet().stream()
-            .filter(e -> e.getKey().test(caps))
-            .peek(e -> LOG.info(String.format("%s matched %s", caps, e.getValue())))
-            .map(Map.Entry::getValue))
+  public ActiveSession createSession(Set<Dialect> downstreamDialects, Capabilities caps) {
+    LOG.info("Capabilities are: " + caps);
+    return factories.entrySet().stream()
+        .filter(e -> e.getKey().test(caps))
+        .peek(e -> LOG.info(String.format("%s matched %s", caps, e.getValue())))
+        .map(Map.Entry::getValue)
         .findFirst()
-        .map(factory -> factory.apply(newSessionPayload))
+        .map(factory -> factory.apply(downstreamDialects, caps))
         .orElseThrow(() -> new SessionNotCreatedException(
-            "Unable to create a new session because of no configuration."));
+            "Unable to create a new session because of no configuration. " + caps));
   }
 }

java/server/src/org/openqa/selenium/remote/server/InMemorySession.java

@@ -18,17 +18,13 @@
 package org.openqa.selenium.remote.server;
 
 
-import static java.nio.charset.StandardCharsets.UTF_8;
-
 import com.google.common.base.Preconditions;
 import com.google.common.base.StandardSystemProperty;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.io.CharStreams;
 
 import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.HasCapabilities;
-import org.openqa.selenium.ImmutableCapabilities;
 import org.openqa.selenium.SessionNotCreatedException;
 import org.openqa.selenium.WebDriver;
 import org.openqa.selenium.io.TemporaryFilesystem;
@@ -38,12 +34,8 @@
 import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.http.HttpResponse;
 
-import java.io.BufferedReader;
 import java.io.File;
 import java.io.IOException;
-import java.io.InputStream;
-import java.io.InputStreamReader;
-import java.io.Reader;
 import java.util.Map;
 import java.util.Set;
 import java.util.UUID;
@@ -138,32 +130,19 @@ public Factory(DriverProvider provider) {
     }
 
     @Override
-    public ActiveSession apply(NewSessionPayload payload) {
+    public ActiveSession apply(Set<Dialect> downstreamDialects, Capabilities caps) {
       // Assume the blob fits in the available memory.
-      try (
-          InputStream is = payload.getPayload().get();
-          Reader ir = new InputStreamReader(is, UTF_8);
-          Reader reader = new BufferedReader(ir)) {
-        Map<?, ?> raw = toBean.convert(Map.class, CharStreams.toString(reader));
-        Object desired = raw.get("desiredCapabilities");
-
-        if (!(desired instanceof Map)) {
-          return null;
-        }
-
-        @SuppressWarnings("unchecked") ImmutableCapabilities caps =
-            new ImmutableCapabilities((Map<String, ?>) desired);
-
+      try {
         if (!provider.canCreateDriverInstanceFor(caps)) {
           return null;
         }
 
         WebDriver driver = provider.newInstance(caps);
 
         // Prefer the OSS dialect.
-        Dialect downstream = payload.getDownstreamDialects().contains(Dialect.OSS) ?
+        Dialect downstream = downstreamDialects.contains(Dialect.OSS) ?
                              Dialect.OSS :
-                             payload.getDownstreamDialects().iterator().next();
+                             downstreamDialects.iterator().next();
         return new InMemorySession(driver, caps, downstream);
       } catch (IOException|IllegalStateException e) {
         throw new SessionNotCreatedException("Cannot establish new session", e);

java/server/src/org/openqa/selenium/remote/server/ServicedSession.java

@@ -23,15 +23,18 @@
 import com.google.common.base.Preconditions;
 import com.google.common.base.StandardSystemProperty;
 
+import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.ImmutableCapabilities;
 import org.openqa.selenium.SessionNotCreatedException;
 import org.openqa.selenium.WebDriver;
 import org.openqa.selenium.io.TemporaryFilesystem;
 import org.openqa.selenium.net.PortProber;
 import org.openqa.selenium.remote.Augmenter;
+import org.openqa.selenium.remote.Command;
 import org.openqa.selenium.remote.CommandCodec;
 import org.openqa.selenium.remote.CommandExecutor;
 import org.openqa.selenium.remote.Dialect;
+import org.openqa.selenium.remote.DriverCommand;
 import org.openqa.selenium.remote.ProtocolHandshake;
 import org.openqa.selenium.remote.RemoteWebDriver;
 import org.openqa.selenium.remote.Response;
@@ -50,10 +53,10 @@
 
 import java.io.File;
 import java.io.IOException;
-import java.io.InputStream;
 import java.lang.reflect.Method;
 import java.net.URL;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.Supplier;
 
 class ServicedSession implements ActiveSession {
@@ -169,10 +172,10 @@ public static class Factory implements SessionFactory {
     }
 
     @Override
-    public ActiveSession apply(NewSessionPayload payload) {
+    public ActiveSession apply(Set<Dialect> downstreamDialects, Capabilities capabilities) {
       DriverService service = createService.get();
 
-      try (InputStream in = payload.getPayload().get()) {
+      try {
         service.start();
 
         PortProber.waitForPortUp(service.getUrl().getPort(), 30, SECONDS);
@@ -181,18 +184,18 @@ public ActiveSession apply(NewSessionPayload payload) {
 
         HttpClient client = new ApacheHttpClient.Factory().createClient(url);
 
-        ProtocolHandshake.Result result = new ProtocolHandshake()
-            .createSession(client, in, payload.getPayloadSize())
-            .orElseThrow(() -> new SessionNotCreatedException("Unable to create session"));
+        Command command = new Command(null, DriverCommand.NEW_SESSION, capabilities.asMap());
+
+        ProtocolHandshake.Result result = new ProtocolHandshake().createSession(client, command);
 
         SessionCodec codec;
         Dialect upstream = result.getDialect();
         Dialect downstream;
-        if (payload.getDownstreamDialects().contains(result.getDialect())) {
+        if (downstreamDialects.contains(result.getDialect())) {
           codec = new Passthrough(url);
           downstream = upstream;
         } else {
-          downstream = payload.getDownstreamDialects().iterator().next();
+          downstream = downstreamDialects.iterator().next();
 
           codec = new ProtocolConverter(
               url,

java/server/src/org/openqa/selenium/remote/server/SessionFactory.java

@@ -18,6 +18,11 @@
 package org.openqa.selenium.remote.server;
 
 
+import org.openqa.selenium.Capabilities;
+import org.openqa.selenium.remote.Dialect;
+
+import java.util.Set;
+
 interface SessionFactory {
-  ActiveSession apply(NewSessionPayload payload);
+  ActiveSession apply(Set<Dialect> downstreamDialects, Capabilities capabilities);
 }

java/server/src/org/openqa/selenium/remote/server/commandhandler/BeginSession.java

@@ -40,6 +40,7 @@
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.Reader;
+import java.util.Objects;
 import java.util.logging.Level;
 
 public class BeginSession implements CommandHandler {
@@ -69,7 +70,19 @@ public void execute(HttpRequest req, HttpResponse resp) throws IOException {
         req.consumeContentStream(),
         req.getContentEncoding());
          NewSessionPayload payload = new NewSessionPayload(contentLength, reader)) {
-      session = sessionFactory.createSession(payload);
+      session = payload.stream()
+          .map(caps -> {
+            try {
+              return sessionFactory.createSession(payload.getDownstreamDialects(), caps);
+            } catch (SessionNotCreatedException e) {
+              // Do nothing. We'll complain at the end.
+              return null;
+            }
+          })
+          .filter(Objects::nonNull)
+          .findFirst()
+          .orElseThrow(
+              () -> new SessionNotCreatedException("Unable to create session: " + payload));
       allSessions.put(session);
     }
 

java/server/test/org/openqa/selenium/remote/server/ActiveSessionFactoryTest.java

@@ -28,6 +28,7 @@
 import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.ImmutableCapabilities;
 import org.openqa.selenium.WebDriver;
+import org.openqa.selenium.remote.Dialect;
 
 import java.io.IOException;
 import java.util.Map;
@@ -47,31 +48,24 @@ protected Iterable<DriverProvider> loadDriverProviders() {
       }
     };
 
-    try (NewSessionPayload payload = new NewSessionPayload(toPayload(caps.getBrowserName()))) {
-      ActiveSession session = sessionFactory.createSession(payload);
-      assertEquals(driver, session.getWrappedDriver());
-    }
+    ActiveSession session = sessionFactory.createSession(ImmutableSet.of(Dialect.W3C), caps);
+    assertEquals(driver, session.getWrappedDriver());
   }
 
   @Test
   public void canBindNewFactoriesAtRunTime() throws IOException {
     ActiveSession session = Mockito.mock(ActiveSession.class);
 
     ActiveSessionFactory sessionFactory = new ActiveSessionFactory()
-        .bind(caps -> "cheese".equals(caps.getBrowserName()), payload -> session);
+        .bind(caps -> "cheese".equals(caps.getBrowserName()), (dialects, caps) -> session);
 
-    try (NewSessionPayload payload = new NewSessionPayload(toPayload("cheese"))) {
-      ActiveSession created = sessionFactory.createSession(payload);
+    ActiveSession created = sessionFactory.createSession(ImmutableSet.copyOf(Dialect.values()), toPayload("cheese"));
 
-      assertSame(session, created);
-    }
+    assertSame(session, created);
   }
 
-  private Map<String, Object> toPayload(String browserName) {
-    return ImmutableMap.of(
-        "capabilities", ImmutableMap.of(
-            "alwaysMatch", ImmutableMap.of("browserName", browserName)),
-        "desiredCapabilities", ImmutableMap.of("browserName", browserName));
+  private Capabilities toPayload(String browserName) {
+    return new ImmutableCapabilities("browserName", browserName);
   }
 
   private static class StubbedProvider implements DriverProvider {