selenium fix(firefox): handle null args in FirefoxOptions.merge() (#15991)

User description

🔗 Related Issues

Fixes #15991

💥 What does this PR do?

Adds a null check to prevent a NullPointerException in FirefoxOptions.merge() when merging user-defined moz:firefoxOptions.

The issue occurred because .contains(...) was called on a null list when "args" was present but not properly initialized. This fix ensures the list is not null before using it.

🔧 Implementation Notes

Used a simple null check before iterating over arguments:

if (arguments != null) {
  ...
}

PR Type

Bug fix

Description

Fix NullPointerException in FirefoxOptions.merge() method
Add null checks before accessing existing arguments list
Prevent crashes when merging user-defined moz:firefoxOptions

Changes diagram

flowchart LR
  A["FirefoxOptions.merge()"] --> B["Check existing args"]
  B --> C["Add null check"]
  C --> D["Safe argument merging"]

Changes walkthrough 📝

Relevant files

Bug fix

FirefoxOptions.java

Add null safety to argument merging

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

Added null checks before accessing existing arguments list

Extracted list reference to variable for cleaner code

Applied fix to two locations in merge() method

+4/-2

Need help?
Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
Check out the documentation for more information.

Jul 12 '25 10:07 zidanmp

All committers have signed the CLA.

Jul 12 '25 10:07 CLAassistant

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Code Duplication

The same null check and variable extraction pattern is duplicated in two locations. Consider extracting this logic into a helper method to improve maintainability and reduce code duplication.

  List<String> existingArgs = (List<String>) newInstance.firefoxOptions.get(Keys.ARGS.key());
  if (existingArgs == null || !existingArgs.contains(arg)) {
    newInstance.addArguments(arg);
  }
});

Performance Impact

The firefoxOptions.get(Keys.ARGS.key()) call is now executed for every argument in the forEach loop instead of once before the loop. This could impact performance when processing many arguments.

          List<String> existingArgs = (List<String>) newInstance.firefoxOptions.get(Keys.ARGS.key());
          if (existingArgs == null || !existingArgs.contains(arg)) {
            newInstance.addArguments(arg);
          }
        });
  }

  if (name.equals(Keys.PREFS.key) && capabilities.getCapability(name) != null) {
    Map<String, Object> prefs = (Map<String, Object>) (capabilities.getCapability(("prefs")));
    prefs.forEach(newInstance::addPreference);
  }

  if (name.equals(Keys.PROFILE.key) && capabilities.getCapability(name) != null) {
    String rawProfile = (String) capabilities.getCapability("profile");
    try {
      newInstance.setProfile(FirefoxProfile.fromJson(rawProfile));
    } catch (IOException e) {
      throw new WebDriverException(e);
    }
  }

  if (name.equals(Keys.BINARY.key) && capabilities.getCapability(name) != null) {
    Object binary = capabilities.getCapability("binary");
    if (binary instanceof String) {
      newInstance.setBinary((String) binary);
    } else if (binary instanceof Path) {
      newInstance.setBinary((Path) binary);
    }
  }

  if (name.equals(Keys.LOG.key) && capabilities.getCapability(name) != null) {
    Map<String, Object> logLevelMap = (Map<String, Object>) capabilities.getCapability("log");
    FirefoxDriverLogLevel logLevel =
        FirefoxDriverLogLevel.fromString((String) logLevelMap.get("level"));
    if (logLevel != null) {
      newInstance.setLogLevel(logLevel);
    }
  }
}

if (capabilities instanceof FirefoxOptions) {
  newInstance.mirror((FirefoxOptions) capabilities);
} else {
  Object optionsValue = capabilities.getCapability(FIREFOX_OPTIONS);

  if (optionsValue instanceof Map) {
    @SuppressWarnings("unchecked")
    Map<String, Object> options = (Map<String, Object>) optionsValue;

    @SuppressWarnings("unchecked")
    List<String> arguments = (List<String>) (options.getOrDefault("args", new ArrayList<>()));
    @SuppressWarnings("unchecked")
    Map<String, Object> prefs =
        (Map<String, Object>) options.getOrDefault("prefs", new HashMap<>());
    String rawProfile = (String) options.get("profile");
    @SuppressWarnings("unchecked")
    Map<String, Object> logLevelMap =
        (Map<String, Object>) options.getOrDefault("log", new HashMap<>());
    FirefoxDriverLogLevel logLevel =
        FirefoxDriverLogLevel.fromString((String) logLevelMap.get("level"));

    arguments.forEach(
        arg -> {
          List<String> existingArgs = (List<String>) newInstance.firefoxOptions.get(Keys.ARGS.key());
          if (existingArgs == null || !existingArgs.contains(arg)) {
            newInstance.addArguments(arg);
          }

Jul 12 '25 10:07 qodo-code-review[bot]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add type safety check The unchecked cast to `List<String>` could fail if the stored value is not a List or contains non-String elements. Add a type check before casting to prevent ClassCastException. java/src/org/openqa/selenium/firefox/FirefoxOptions.java [308-311] `-List<String> existingArgs = (List<String>) newInstance.firefoxOptions.get(Keys.ARGS.key()); +Object argsObj = newInstance.firefoxOptions.get(Keys.ARGS.key()); +List<String> existingArgs = (argsObj instanceof List) ? (List<String>) argsObj : null; if (existingArgs == null \|\| !existingArgs.contains(arg)) { newInstance.addArguments(arg); }` [ ] Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that the unchecked cast could lead to a `ClassCastException` and proposes using an `instanceof` check to improve type safety and make the code more robust.	Medium
[ ] Update

Jul 12 '25 10:07 qodo-code-review[bot]

Can you please add a test?

Hi @diemol. Do you have plan to update the code base with additional tests?

Jul 24 '25 13:07 parovozby

@parovozby, not for now. It'd be nice if you can give us a hand.

Jul 24 '25 18:07 diemol