Closed Bug 1788861 Opened 5 months ago Closed 5 months ago

script.callFunction clones remote references after deserialization

Categories

(Remote Protocol :: WebDriver BiDi, defect, P1)

defect
Points:
2

Tracking

(firefox107 fixed)

RESOLVED FIXED
107 Branch
Tracking Status
firefox107 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Whiteboard: [webdriver:m5], [wptsync upstream])

Attachments

(2 files, 2 obsolete files)

script.callFunction uses Realm.executeInGlobalWithBindings which will force cloning "this" and "arguments" before evaluating the function declaration: https://searchfox.org/mozilla-central/rev/25ec642ed33ca83f25e88ec0f9f27e8ad8a29e24/remote/webdriver-bidi/Realm.jsm#223-240

  executeInGlobalWithBindings(
    functionDeclaration,
    functionArguments,
    thisParameter
  ) {
    const expression = `(${functionDeclaration}).apply(__bidi_this, __bidi_args)`;

    return this.#globalObjectReference.executeInGlobalWithBindings(
      expression,
      {
        __bidi_args: this.#cloneAsDebuggerObject(functionArguments),
        __bidi_this: this.#cloneAsDebuggerObject(thisParameter),
      },
      {
        url: this.#window.document.baseURI,
      }
    );
  }

This is a problem whenever you are trying to use remote references (ie objects coming from this Realm, identified by a handle) because you should be able to use the original objects, and not a cloned version.

The deserialization algorithm should be responsible for cloning objects "when necessary" into the realm, but should preserve original objects if they are remote references.

An example fix can be found at https://phabricator.services.mozilla.com/D156201

A small example that fails because of this is

    remote_value = await bidi_session.script.evaluate(
        expression="window.test = {}; window.test",
        await_promise=False,
        result_ownership="root",
        target=ContextTarget(top_context["context"]),
    )
    result = await bidi_session.script.call_function(
        function_declaration="obj => obj === window.test",
        await_promise=False,
        arguments=[remote_value],
        target=ContextTarget(top_context["context"]),
    )
    assert result == {"type": "boolean", "value": True}

This should pass but since the remote_value gets cloned when we use call_function, the obj === window.test check will return false.

Blocks: 1750540
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

Unassigning until triage.

Assignee: jdescottes → nobody
Status: ASSIGNED → NEW
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

We will have to get this fixed for script.callFunctionOn so that the original object is used and not a clone.

Blocks: 1778976
Points: --- → 2
Priority: -- → P1
Whiteboard: [webdriver:m4]

Depends on D156201

Simple autoformatting to avoid unrelated changes in the next changeset

Depends on D156530

Add tests for deserialization of both this and arguments parameters, with various data structures (object, array, map, set) plus an additional test case nesting all
together.

Attachment #9292792 - Attachment is obsolete: true
Attachment #9293539 - Attachment description: Bug 1788861 - [bidi] Deserialization of remote references should not create new objects (alternative approach) → Bug 1788861 - [bidi] Deserialization of remote references should not create new objects
Attachment #9293259 - Attachment is obsolete: true
Whiteboard: [webdriver:m4] → [webdriver:m5]

The severity field is not set for this bug.
:whimboo, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(hskupin)
Severity: -- → S3
Flags: needinfo?(hskupin)

This is a M5 bug and should not block the enabling of the script commands by default.

No longer blocks: 1778976
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/83a729f1fffd
[bidi] Deserialization of remote references should not create new objects r=Sasha,webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/149ee65a5a38
[wdspec] Add call_function test for deserialization of remote references r=webdriver-reviewers,Sasha,whimboo
Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.
Whiteboard: [webdriver:m5] → [webdriver:m5], [wptsync upstream error]

(In reply to Web Platform Test Sync Bot (Matrix: #interop:mozilla.org) from comment #10)

Failed to create upstream wpt PR due to merge conflicts. This requires fixup
from a wpt sync admin.

This basically happened because bug 1779231 isn't on mozilla-central yet and it's open PR hasn't been merged. James could you please take a look here? Thanks.

Depends on: 1779231
Flags: needinfo?(james)
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 107 Branch
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/36033 for changes under testing/web-platform/tests
Whiteboard: [webdriver:m5], [wptsync upstream error] → [webdriver:m5], [wptsync upstream]
Upstream PR merged by jgraham
Flags: needinfo?(james)
You need to log in before you can comment on or make changes to this bug.