[🐛 Bug]: find-elements-fragment has an issue with JSON

[jlipps]

What happened?

I generated find-element-fragment.js and pasted it into a browser. One line (minified it looks like the only line in the fragment that includes ba.JSON.stringify) seems to host this bug: ba is undefined.

Reading the minified code as best I can, it looks like ba is either document or window.navigator. Neither of these host the JSON global (it is on window, I think).

Removing ba. from the line so that it references JSON as a pure global fixes the issue.

I'm not sure how to navigate the whole Google Closure Compiler situation. A quick grep of the codebase for a fixable line only showed up goog.global.JSON. Perhaps this issue is internal to the Compiler?

How can we reproduce the issue?

1. Generate the fragment with `bazel build //javascript/webdriver/atoms/inject/...`
2. Copy the contents of the built `find-element-fragment.js` file into a browser console
3. Invoke the pasted contents as an IIFE with some params (like `'css selector', 'body'`)
4. Observe the error

Relevant log output

VM368:1631 Uncaught TypeError: Cannot read properties of undefined (reading 'stringify')
    at Object.<anonymous> (<anonymous>:1631:26)
    at findElement (<anonymous>:2633:8)
    at <anonymous>:1:1

Operating System

macOS

Selenium version

4.11.0

What are the browser(s) and version(s) where you see this issue?

Chrome, Safari

What are the browser driver(s) and version(s) where you see this issue?

N/A

Are you using Selenium Grid?

N/A

[github-actions]

@jlipps, thank you for creating this issue. We will troubleshoot it as soon as we can.

---

Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

[titusfortner]

I'm not sure who really understands the atoms code at this point. @shs96c / @jimevans / @AutomatedTester ?

[jlipps]

The bug appears to be specifically related to wrapper in javascript/private/fragment.bzl. This causes the this of the fragment to be the following object:

{
        navigator: typeof window != 'undefined' ? window.navigator : null,
        document: typeof window != 'undefined' ? window.document : null
}

However, inside the fragment, we have the following assignment:

        var ba = this || self;

Presumably ba is minify-speak for a global or window object. We try to find lots of things on ba, like JSON, navigator, Range, etc... Clearly the code inside the fragment assumes the this object is a window, not whatever mongrel thing is actually applied.

I am unsure if the solution is to change the wrapper in fragment.bzl, or to change the initial assignment so that we wind up with something like var ba = self instead.

[jlipps]

This diff appears to fix the problem:

diff --git a/javascript/private/fragment.bzl b/javascript/private/fragment.bzl
index d5d6ed1b..920685ed 100644
--- a/javascript/private/fragment.bzl
+++ b/javascript/private/fragment.bzl
@@ -59,10 +59,7 @@ def closure_fragment(
     #     See http://code.google.com/p/selenium/issues/detail?id=1333
     wrapper = (
         "function(){" +
-        "return (function(){%output%; return this._.apply(null,arguments);}).apply({" +
-        "navigator:typeof window!='undefined'?window.navigator:null," +
-        "document:typeof window!='undefined'?window.document:null" +
-        "}, arguments);}"
+        "return (function(){%output%; return this._.apply(null,arguments);}).apply(window, arguments);}"
     )
 
     browser_defs = {

While also appearing to explicitly go against the intention in the comment above. It seems to reflect the intention of the actual wrapped code better though. Thoughts anyone?

[jlipps]

With the merge of #12704, this is a bug again