Ensure new server sessions have the same session ID as the session th…

…ey represent.
babymechanic · May 4, 2017 · 75a0ca7 · 75a0ca7
1 parent e60b607
commit 75a0ca7
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 56 deletions.
diff --git a/java/server/src/org/openqa/selenium/remote/server/DefaultDriverSessions.java b/java/server/src/org/openqa/selenium/remote/server/DefaultDriverSessions.java
@@ -18,10 +18,12 @@
 package org.openqa.selenium.remote.server;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.io.Files;
 
 import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.Platform;
 import org.openqa.selenium.WebDriver;
+import org.openqa.selenium.io.TemporaryFilesystem;
 import org.openqa.selenium.remote.DesiredCapabilities;
 import org.openqa.selenium.remote.SessionId;
 
@@ -30,7 +32,6 @@
 import java.util.Map;
 import java.util.ServiceLoader;
 import java.util.Set;
-import java.util.UUID;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.logging.Logger;
 
@@ -127,12 +128,15 @@ public void registerDriver(Capabilities capabilities, Class<? extends WebDriver>
   }
 
   public SessionId newSession(Capabilities desiredCapabilities) throws Exception {
-    SessionId sessionId = new SessionId(UUID.randomUUID().toString());
-    Session session = DefaultSession.createSession(factory, clock, sessionId, desiredCapabilities);
+    Session session = DefaultSession.createSession(
+        factory,
+        TemporaryFilesystem.getTmpFsBasedOn(Files.createTempDir()),
+        clock,
+        desiredCapabilities);
 
-    sessionIdToDriver.put(sessionId, session);
+    sessionIdToDriver.put(session.getSessionId(), session);
 
-    return sessionId;
+    return session.getSessionId();
   }
 
   public Session get(SessionId sessionId) {

diff --git a/java/server/src/org/openqa/selenium/remote/server/DefaultSession.java b/java/server/src/org/openqa/selenium/remote/server/DefaultSession.java
@@ -17,16 +17,13 @@
 
 package org.openqa.selenium.remote.server;
 
-import com.google.common.annotations.VisibleForTesting;
-
 import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.HasCapabilities;
 import org.openqa.selenium.JavascriptExecutor;
 import org.openqa.selenium.Platform;
 import org.openqa.selenium.Rotatable;
 import org.openqa.selenium.TakesScreenshot;
 import org.openqa.selenium.WebDriver;
-import org.openqa.selenium.WebDriverException;
 import org.openqa.selenium.html5.ApplicationCache;
 import org.openqa.selenium.html5.LocationContext;
 import org.openqa.selenium.html5.WebStorage;
@@ -36,10 +33,11 @@
 import org.openqa.selenium.mobile.NetworkConnection;
 import org.openqa.selenium.remote.CapabilityType;
 import org.openqa.selenium.remote.DesiredCapabilities;
+import org.openqa.selenium.remote.RemoteWebDriver;
 import org.openqa.selenium.remote.SessionId;
 import org.openqa.selenium.support.events.EventFiringWebDriver;
 
-import java.io.File;
+import java.util.UUID;
 import java.util.concurrent.Callable;
 import java.util.concurrent.FutureTask;
 import java.util.concurrent.LinkedBlockingQueue;
@@ -77,43 +75,29 @@ public class DefaultSession implements Session {
   private volatile Thread inUseWithThread = null;
   private TemporaryFilesystem tempFs;
 
-  // This method is to avoid constructor escape of partially constructed session object
-  public static Session createSession(DriverFactory factory, SessionId sessionId,
-                                      Capabilities capabilities) throws Exception {
-    return createSession(factory, new SystemClock(), sessionId, capabilities);
-  }
-
-  // This method is to avoid constructor escape of partially constructed session object
-  public static Session createSession(DriverFactory factory, Clock clock, SessionId sessionId,
-                                      Capabilities capabilities) throws Exception {
-    File tmpDir = new File(System.getProperty("java.io.tmpdir"), sessionId.toString());
-    if (!tmpDir.mkdir()) {
-      throw new WebDriverException("Cannot create temp directory: " + tmpDir);
-    }
-    TemporaryFilesystem tempFs = TemporaryFilesystem.getTmpFsBasedOn(tmpDir);
-
-    return new DefaultSession(factory, tempFs, clock, sessionId, capabilities);
-  }
-
-  @VisibleForTesting
-  public static Session createSession(DriverFactory factory, TemporaryFilesystem tempFs, Clock clock,
-                                      SessionId sessionId, Capabilities capabilities)
+  public static Session createSession(
+      DriverFactory factory,
+      TemporaryFilesystem tempFs,
+      Clock clock,
+      Capabilities capabilities)
       throws Exception {
-    return new DefaultSession(factory, tempFs, clock, sessionId, capabilities);
+    return new DefaultSession(factory, tempFs, clock, capabilities);
   }
 
-  private DefaultSession(final DriverFactory factory, TemporaryFilesystem tempFs, Clock clock,
-                         SessionId sessionId, final Capabilities capabilities) throws Exception {
+  private DefaultSession(
+      final DriverFactory factory,
+      TemporaryFilesystem tempFs,
+      Clock clock,
+      final Capabilities capabilities) throws Exception {
     this.knownElements = new KnownElements();
-    this.sessionId = sessionId;
     this.tempFs = tempFs;
     this.clock = clock;
     final BrowserCreator browserCreator = new BrowserCreator(factory, capabilities);
     final FutureTask<EventFiringWebDriver> webDriverFutureTask =
         new FutureTask<>(browserCreator);
     executor = new ThreadPoolExecutor(1, 1,
                                       600L, TimeUnit.SECONDS,
-                                      new LinkedBlockingQueue<Runnable>());
+                                      new LinkedBlockingQueue<>());
 
     // Ensure that the browser is created on the single thread.
     EventFiringWebDriver initialDriver = execute(webDriverFutureTask);
@@ -125,10 +109,14 @@ private DefaultSession(final DriverFactory factory, TemporaryFilesystem tempFs,
 
     this.driver = initialDriver;
     this.capabilities = browserCreator.getCapabilityDescription();
+    this.sessionId = browserCreator.getSessionId();
+
     updateLastAccessTime();
   }
 
-  private static boolean isQuietModeEnabled(BrowserCreator browserCreator, Capabilities capabilities) {
+  private static boolean isQuietModeEnabled(
+      BrowserCreator browserCreator,
+      Capabilities capabilities) {
     if (browserCreator.isAndroid()) {
       return true;
     }
@@ -165,19 +153,14 @@ public void close() {
 
 
   public <X> X execute(final FutureTask<X> future) throws Exception {
-/*    if (executor.isShutdown()) {
-         throw new WebDriverException(sessionId + " is closed for further execution");
-    } */
-    executor.execute(new Runnable() {
-      public void run() {
-        inUseWithThread = Thread.currentThread();
-        inUseWithThread.setName("Session " + sessionId + " processing inside browser");
-        try {
-          future.run();
-        } finally {
-          inUseWithThread = null;
-          Thread.currentThread().setName("Session " + sessionId + " awaiting client");
-        }
+    executor.execute(() -> {
+      inUseWithThread = Thread.currentThread();
+      inUseWithThread.setName("Session " + sessionId + " processing inside browser");
+      try {
+        future.run();
+      } finally {
+        inUseWithThread = null;
+        Thread.currentThread().setName("Session " + sessionId + " awaiting client");
       }
     });
     return future.get();
@@ -211,6 +194,7 @@ private class BrowserCreator implements Callable<EventFiringWebDriver> {
     private final DriverFactory factory;
     private final Capabilities capabilities;
     private volatile Capabilities describedCapabilities;
+    private volatile SessionId sessionId;
     private volatile boolean isAndroid = false;
 
     BrowserCreator(DriverFactory factory, Capabilities capabilities) {
@@ -226,13 +210,22 @@ public EventFiringWebDriver call() throws Exception {
         isAndroid = actualCapabilities.getPlatform().is(Platform.ANDROID);
       }
       describedCapabilities = getDescription(rawDriver, actualCapabilities);
+      if (rawDriver instanceof RemoteWebDriver) {
+        sessionId = ((RemoteWebDriver) rawDriver).getSessionId();
+      } else {
+        sessionId = new SessionId(UUID.randomUUID().toString());
+      }
       return new EventFiringWebDriver(rawDriver);
     }
 
     public Capabilities getCapabilityDescription() {
       return describedCapabilities;
     }
 
+    public SessionId getSessionId() {
+      return sessionId;
+    }
+
     public boolean isAndroid() {
       return isAndroid;
     }

diff --git a/java/server/test/org/openqa/selenium/remote/server/DefaultSessionTest.java b/java/server/test/org/openqa/selenium/remote/server/DefaultSessionTest.java
@@ -39,7 +39,10 @@ public void shouldClearTempFsWhenSessionCloses() throws Exception {
     when(factory.newInstance(any(Capabilities.class))).thenReturn(mock(WebDriver.class));
     final TemporaryFilesystem tempFs = mock(TemporaryFilesystem.class);
 
-    Session session = DefaultSession.createSession(factory, tempFs, new SystemClock(), null, DesiredCapabilities.firefox());
+    Session session = DefaultSession.createSession(
+        factory, tempFs,
+        new SystemClock(),
+        DesiredCapabilities.firefox());
 
     session.close();
     verify(tempFs).deleteTemporaryFiles();

diff --git a/java/server/test/org/openqa/selenium/remote/server/handler/UploadFileTest.java b/java/server/test/org/openqa/selenium/remote/server/handler/UploadFileTest.java
@@ -38,7 +38,6 @@
 import org.openqa.selenium.io.TemporaryFilesystem;
 import org.openqa.selenium.io.Zip;
 import org.openqa.selenium.remote.DesiredCapabilities;
-import org.openqa.selenium.remote.SessionId;
 import org.openqa.selenium.remote.server.DefaultSession;
 import org.openqa.selenium.remote.server.DriverFactory;
 import org.openqa.selenium.remote.server.Session;
@@ -53,15 +52,13 @@ public class UploadFileTest {
 
   private DriverFactory driverFactory;
   private TemporaryFilesystem tempFs;
-  private SessionId sessionId;
   private File tempDir;
 
   @Before
   public void setUp() {
     driverFactory = mock(DriverFactory.class);
     when(driverFactory.newInstance(any(Capabilities.class))).thenReturn(mock(WebDriver.class));
     tempDir = Files.createTempDir();
-    sessionId = new SessionId("foo");
     tempFs = TemporaryFilesystem.getTmpFsBasedOn(tempDir);
   }
 
@@ -73,7 +70,11 @@ public void cleanUp() {
 
   @Test
   public void shouldWriteABase64EncodedZippedFileToDiskAndKeepName() throws Exception {
-    Session session = DefaultSession.createSession(driverFactory, tempFs, new SystemClock(), sessionId, DesiredCapabilities.firefox());
+    Session session = DefaultSession.createSession(
+        driverFactory,
+        tempFs,
+        new SystemClock(),
+        DesiredCapabilities.firefox());
 
     File tempFile = touch(null, "foo");
     String encoded = Zip.zip(tempFile);
@@ -89,15 +90,19 @@ public void shouldWriteABase64EncodedZippedFileToDiskAndKeepName() throws Except
 
   @Test
   public void shouldThrowAnExceptionIfMoreThanOneFileIsSent() throws Exception {
-    Session session = DefaultSession.createSession(driverFactory, tempFs, new SystemClock(), sessionId, DesiredCapabilities.firefox());
+    Session session = DefaultSession.createSession(
+        driverFactory,
+        tempFs,
+        new SystemClock(),
+        DesiredCapabilities.firefox());
     File baseDir = Files.createTempDir();
 
     touch(baseDir, "example");
     touch(baseDir, "unwanted");
     String encoded = Zip.zip(baseDir);
 
     UploadFile uploadFile = new UploadFile(session);
-    Map<String, Object> args = ImmutableMap.of("file", (Object) encoded);
+    Map<String, Object> args = ImmutableMap.of("file", encoded);
     uploadFile.setJsonParameters(args);
 
     try {