Skip to content

Commit 1b555e4

Browse files
shs96cshs96c
committed
Fix a problem with serializing FirefoxProfile to a remote server
1 parent 04b0789 commit 1b555e4

1 file changed

Lines changed: 99 additions & 73 deletions

File tree

‎java/client/src/org/openqa/selenium/firefox/FirefoxOptions.java‎

Lines changed: 99 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -33,23 +33,28 @@
3333

3434
import org.openqa.selenium.Capabilities;
3535
import org.openqa.selenium.SessionNotCreatedException;
36+
import org.openqa.selenium.WebDriver;
3637
import org.openqa.selenium.WebDriverException;
3738
import org.openqa.selenium.firefox.internal.ProfilesIni;
3839
import org.openqa.selenium.logging.LoggingPreferences;
3940
import org.openqa.selenium.remote.CapabilityType;
4041
import org.openqa.selenium.remote.DesiredCapabilities;
4142

43+
import sun.awt.image.ByteInterleavedRaster;
44+
4245
import java.io.File;
4346
import java.io.IOException;
4447
import java.nio.file.Path;
4548
import java.nio.file.Paths;
4649
import java.util.ArrayList;
4750
import java.util.Collection;
51+
import java.util.Collections;
4852
import java.util.HashMap;
4953
import java.util.List;
5054
import java.util.Map;
5155
import java.util.Optional;
5256
import java.util.logging.Level;
57+
import java.util.logging.Logger;
5358
import java.util.stream.Collectors;
5459
import java.util.stream.Stream;
5560

@@ -68,6 +73,7 @@
6873
public class FirefoxOptions {
6974

7075
public final static String FIREFOX_OPTIONS = "moz:firefoxOptions";
76+
private final static Logger LOG = Logger.getLogger(FirefoxOptions.class.getName());
7177

7278
private String binaryPath;
7379
private FirefoxBinary actualBinary;
@@ -159,18 +165,24 @@ public boolean isLegacy() {
159165
public FirefoxOptions setBinary(FirefoxBinary binary) {
160166
this.actualBinary = Preconditions.checkNotNull(binary);
161167
binary.amendOptions(this);
168+
desiredCapabilities.setCapability(BINARY, binary);
162169
this.binaryPath = null;
163170
return this;
164171
}
165172

166173
public FirefoxOptions setBinary(Path path) {
167174
// Default to UNIX-style paths, even on Windows.
168-
StringBuilder builder = new StringBuilder(path.isAbsolute() ? "/" : "");
169-
this.binaryPath = Joiner.on("/").appendTo(builder, path).toString();
175+
this.binaryPath = asUnixPath(path);
170176
this.actualBinary = null;
177+
desiredCapabilities.setCapability(BINARY, new FirefoxBinary(path.toFile()));
171178
return this;
172179
}
173180

181+
private String asUnixPath(Path path) {
182+
StringBuilder builder = new StringBuilder(path.isAbsolute() ? "/" : "");
183+
return Joiner.on("/").appendTo(builder, path).toString();
184+
}
185+
174186
public FirefoxOptions setBinary(String path) {
175187
return setBinary(Paths.get(checkNotNull(path)));
176188
}
@@ -228,6 +240,16 @@ private Optional<FirefoxBinary> determineBinaryFromCapabilities(Capabilities cap
228240

229241
public FirefoxOptions setProfile(FirefoxProfile profile) {
230242
this.profile = profile;
243+
244+
if (!booleanPrefs.isEmpty() || !intPrefs.isEmpty() || !stringPrefs.isEmpty()) {
245+
LOG.info("Will update profile with preferences from these options.");
246+
booleanPrefs.entrySet().forEach(e -> profile.setPreference(e.getKey(), e.getValue()));
247+
intPrefs.entrySet().forEach(e -> profile.setPreference(e.getKey(), e.getValue()));
248+
stringPrefs.entrySet().forEach(e -> profile.setPreference(e.getKey(), e.getValue()));
249+
}
250+
251+
desiredCapabilities.setCapability(PROFILE, profile);
252+
231253
return this;
232254
}
233255

@@ -289,14 +311,6 @@ private static void populateProfile(FirefoxProfile profile, Capabilities capabil
289311
}
290312
}
291313

292-
// Confusing API. Keeping package visible only
293-
FirefoxOptions setProfileSafely(FirefoxProfile profile) {
294-
if (profile == null) {
295-
return this;
296-
}
297-
return setProfile(profile);
298-
}
299-
300314
public FirefoxOptions addArguments(String... arguments) {
301315
addArguments(ImmutableList.copyOf(arguments));
302316
return this;
@@ -309,16 +323,25 @@ public FirefoxOptions addArguments(List<String> arguments) {
309323

310324
public FirefoxOptions addPreference(String key, boolean value) {
311325
booleanPrefs.put(checkNotNull(key), value);
326+
if (profile != null) {
327+
profile.setPreference(key, value);
328+
}
312329
return this;
313330
}
314331

315332
public FirefoxOptions addPreference(String key, int value) {
316333
intPrefs.put(checkNotNull(key), value);
334+
if (profile != null) {
335+
profile.setPreference(key, value);
336+
}
317337
return this;
318338
}
319339

320340
public FirefoxOptions addPreference(String key, String value) {
321341
stringPrefs.put(checkNotNull(key), checkNotNull(value));
342+
if (profile != null) {
343+
profile.setPreference(key, value);
344+
}
322345
return this;
323346
}
324347

@@ -328,28 +351,33 @@ public FirefoxOptions setLogLevel(Level logLevel) {
328351
}
329352

330353
public FirefoxOptions addDesiredCapabilities(Capabilities desiredCapabilities) {
331-
if (desiredCapabilities == null) {
354+
return validateAndAmendUsing(this.desiredCapabilities, desiredCapabilities);
355+
}
356+
357+
public FirefoxOptions addRequiredCapabilities(Capabilities requiredCapabilities) {
358+
return validateAndAmendUsing(this.requiredCapabilities, requiredCapabilities);
359+
}
360+
361+
private FirefoxOptions validateAndAmendUsing(DesiredCapabilities existing, Capabilities caps) {
362+
if (caps == null) {
332363
return this;
333364
}
334365

335-
this.desiredCapabilities.merge(desiredCapabilities);
366+
existing.merge(caps);
336367

337-
FirefoxProfile suggestedProfile = extractProfile(desiredCapabilities);
338-
if (suggestedProfile != null) {
339-
if (!booleanPrefs.isEmpty() || !intPrefs.isEmpty() || !stringPrefs.isEmpty()) {
340-
throw new IllegalStateException(
341-
"Unable to determine if preferences set on this option " +
342-
"are the same as the profile in the capabilities");
343-
}
344-
if (profile != null && !suggestedProfile.equals(profile)) {
345-
throw new IllegalStateException(
346-
"Profile has been set on both the capabilities and these options, but they're " +
347-
"different. Unable to determine which one you want to use.");
348-
}
349-
profile = suggestedProfile;
368+
FirefoxProfile newProfile = extractProfile(caps);
369+
if (profile != null && newProfile != null && !profile.equals(newProfile)) {
370+
LOG.info("Found a profile on these options and the capabilities. Will assume you " +
371+
"want the profile already set here. If you're seeing this in the logs of the " +
372+
"standalone server, we've probably just deserialized the same options twice and " +
373+
"it's likely that there's nothing to worry about.");
350374
}
351375

352-
Object binary = desiredCapabilities.getCapability(BINARY);
376+
if (newProfile != null) {
377+
setProfile(newProfile);
378+
}
379+
380+
Object binary = existing.getCapability(BINARY);
353381
if (binary != null) {
354382
if (binary instanceof File) {
355383
setBinary(((File) binary).toPath());
@@ -365,27 +393,6 @@ public FirefoxOptions addDesiredCapabilities(Capabilities desiredCapabilities) {
365393
return this;
366394
}
367395

368-
public FirefoxOptions addRequiredCapabilities(Capabilities requiredCapabilities) {
369-
this.requiredCapabilities.merge(requiredCapabilities);
370-
371-
FirefoxProfile suggestedProfile = extractProfile(desiredCapabilities);
372-
if (suggestedProfile != null) {
373-
if (!booleanPrefs.isEmpty() || !intPrefs.isEmpty() || !stringPrefs.isEmpty()) {
374-
throw new IllegalStateException(
375-
"Unable to determine if preferences set on this option " +
376-
"are the same as the profile in the capabilities");
377-
}
378-
if (profile != null && !suggestedProfile.equals(profile)) {
379-
throw new IllegalStateException(
380-
"Profile has been set on both the capabilities and these options, but they're " +
381-
"different. Unable to determine which one you want to use.");
382-
}
383-
profile = suggestedProfile;
384-
}
385-
386-
return this;
387-
}
388-
389396
private FirefoxProfile extractProfile(Capabilities capabilities) {
390397
if (capabilities == null) {
391398
return null;
@@ -409,7 +416,15 @@ private FirefoxProfile extractProfile(Capabilities capabilities) {
409416
}
410417

411418
public Capabilities toDesiredCapabilities() {
412-
DesiredCapabilities capabilities = new DesiredCapabilities(desiredCapabilities);
419+
return toCapabilities(desiredCapabilities);
420+
}
421+
422+
public Capabilities toRequiredCapabilities() {
423+
return toCapabilities(requiredCapabilities);
424+
}
425+
426+
private Capabilities toCapabilities(Capabilities source) {
427+
DesiredCapabilities capabilities = new DesiredCapabilities(source);
413428

414429
if (isLegacy()) {
415430
capabilities.setCapability(FirefoxDriver.MARIONETTE, false);
@@ -418,40 +433,54 @@ public Capabilities toDesiredCapabilities() {
418433
Object priorBinary = capabilities.getCapability(BINARY);
419434
if (priorBinary instanceof Path) {
420435
// Again, unix-style path
421-
priorBinary = Joiner.on("/").join((Path) priorBinary);
436+
priorBinary = asUnixPath((Path) priorBinary);
422437
}
423438
if (priorBinary instanceof String) {
424-
if (actualBinary != null || !priorBinary.equals(binaryPath)) {
425-
throw new IllegalStateException(String.format(
426-
"Binary already set in capabilities, but is different from the one set here: %s, %s",
427-
priorBinary,
428-
binaryPath != null ? binaryPath : actualBinary));
429-
}
439+
priorBinary = asUnixPath(Paths.get((String) priorBinary));
430440
}
431441
if (priorBinary instanceof FirefoxBinary) {
432-
// Relies on instance equality, which is fragile.
433-
if (binaryPath != null || !priorBinary.equals(actualBinary)) {
434-
throw new IllegalStateException(String.format(
435-
"Binary already set in capabilities, but is different from the one set here: %s, %s",
436-
priorBinary,
437-
actualBinary != null ? actualBinary : binaryPath));
438-
}
442+
priorBinary = asUnixPath(((FirefoxBinary) priorBinary).getFile().toPath());
443+
}
444+
445+
if ((actualBinary != null && !actualBinary.getFile().toPath().equals(priorBinary)) ||
446+
(binaryPath != null && !binaryPath.equals(priorBinary))) {
447+
LOG.info(String.format(
448+
"Preferring the firefox binary in these options (%s rather than %s)",
449+
actualBinary != null ? actualBinary.getPath() : binaryPath,
450+
priorBinary));
451+
}
452+
if (actualBinary != null && binaryPath == null) {
453+
capabilities.setCapability(BINARY, actualBinary);
454+
} else if (binaryPath != null && actualBinary == null) {
455+
capabilities.setCapability(BINARY, new FirefoxBinary(new File(binaryPath)));
439456
}
440457

441458
Object priorProfile = capabilities.getCapability(PROFILE);
459+
if (priorProfile instanceof String) {
460+
try {
461+
priorProfile = FirefoxProfile.fromJson((String) priorProfile);
462+
} catch (IOException e) {
463+
throw new WebDriverException(e);
464+
}
465+
}
442466
if (priorProfile != null) {
443467
if (!booleanPrefs.isEmpty() || !intPrefs.isEmpty() || !stringPrefs.isEmpty()) {
444-
throw new IllegalStateException(
445-
"Unable to determine if preferences set on this option " +
446-
"are the same as the profile in the capabilities");
468+
LOG.info("Setting our our preferences on the existing profile");
447469
}
448470
if (profile != null && !priorProfile.equals(profile)) {
449-
throw new IllegalStateException(
450-
"Profile has been set on both the capabilities and these options, but they're " +
451-
"different. Unable to determine which one you want to use.");
471+
LOG.info("Found a profile on these options and the capabilities. Will assume you " +
472+
"want the profile already set here. If you're seeing this in the logs of the " +
473+
"standalone server, we've probably just deserialized the same options twice and " +
474+
"it's likely that there's nothing to worry about.");
475+
}
476+
if (profile == null) {
477+
if (priorProfile instanceof FirefoxProfile) {
478+
profile = (FirefoxProfile) priorProfile;
479+
} else {
480+
LOG.info("Unable to use profile: " + priorProfile.getClass());
481+
}
452482
}
453483
}
454-
455484
capabilities.setCapability(FIREFOX_OPTIONS, this);
456485

457486
if (actualBinary != null) {
@@ -469,12 +498,9 @@ public Capabilities toDesiredCapabilities() {
469498
return capabilities;
470499
}
471500

472-
public Capabilities toRequiredCapabilities() {
473-
return requiredCapabilities;
474-
}
475-
476501
public DesiredCapabilities addTo(DesiredCapabilities capabilities) {
477502
capabilities.merge(toDesiredCapabilities());
503+
capabilities.merge(toRequiredCapabilities());
478504
return capabilities;
479505
}
480506

0 commit comments

Comments
 (0)