[🐛 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[bot]]

@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

[titusfortner]

@shs96c is the findElements atom we use for relative locators dependent on this fragment? I'm not sure who is actively using this one (other than Appium apparently)

[github-actions[bot]]

This issue is looking for contributors.

Please comment below or reach out to us through our IRC/Slack/Matrix channels if you are interested.

[titusfortner]

@AutomatedTester you've been in this code recently. It looks like some change recently has been causing issues for Appium users. Do you know what needs to be done here?

[KazuCocoa]

I wondered if one of the old JS compilers changed the behavior (based on the initial jlipps's guess). This xx.JSON.stringify error itself was not in find-elements-fragment according to some other atoms in https://github.com/appium/appium-remote-debugger/pull/384 (the PR itself was vanilla atoms by this selenium project with bazel build //javascript/webdriver/atoms etc). I made some changes directly in generated JS code to avoid xx.JSON.stringify in find-elements-fragment. Then another js file, such as click, had the same error but a different randomly generated string, such as ia.JSON.stringify.

So... I wondered if we could build atoms with https://github.com/SeleniumHQ/selenium/tree/selenium-3.141.59/third_party/closure closure compiler... I'm not familiar with Bazel build though.

navigator: typeof window != 'undefined' ? window.navigator : null... code existed in selenium v3 as well, so I wondered if we could compare diff with the same JS compiler version.