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

[zidanmp]

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"]

Loading

---

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.

[CLAassistant]

All committers have signed the CLA.

[qodo-merge-pro]

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); }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add type safety check The unchecked cast to List 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