Closed Bug 1716963 Opened 3 years ago Closed 3 years ago

Marionette client doesn't use socket in a threadsafe way

Categories

(Remote Protocol :: Marionette, defect)

Default
defect

Tracking

(firefox91 fixed)

RESOLVED FIXED
91 Branch
Tracking Status
firefox91 --- fixed

People

(Reporter: jgraham, Assigned: jgraham)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

If we accidentially end up accessing the transport from multiple threads we can end up interleaving reads and so getting partial messages. It's probably reasonable to just fix this with a lock.

The previous function used early return in a way that made it hard to
follow, and made undocumented assumptions about the minimum possible
size of a message. This version is intended to have clearer control
flow and make the assumptions more explicit.

Assignee: nobody → james
Status: NEW → ASSIGNED

The GC is dangerous as we can end up calling the transport layer reentrantly,
resulting in corrupted packets. Since relying on gc for cleanup is often
unnecessary, put in an explicit cleanup argument that avoids doing work in the
destructor, and use it for wpt.

This should ensure that we can't end up with multiple threads
interleaving reads or writes on the socket.

This avoid the weirdness where we reach into the binary message data to
get the message type and then unconditionally deserialize as JSON anyway.

Attachment #9227642 - Attachment description: Bug 1716963 - Add explicit cleanup to marionette client, → Bug 1716963 - Don't call cleanup from __del__ if it already ran,
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/995c3b9b36dc
Rewrite the message recv function in marionette, r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/02641f631413
Don't call cleanup from __del__ if it already ran, r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/6b896d1cd3db
Guard access to marionette socket with a lock, r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/adb5fd73a3bc
Deserialize marionette message earlier, r=webdriver-reviewers,whimboo
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/29465 for changes under testing/web-platform/tests
Status: RESOLVED → REOPENED
Flags: needinfo?(james)
Resolution: FIXED → ---
Target Milestone: 91 Branch → ---
Upstream PR was closed without merging
Attachment #9229777 - Attachment is obsolete: true
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/9d5ec2a71554
Rewrite the message recv function in marionette, r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/6b8537e78d74
Don't call cleanup from __del__ if it already ran, r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/6dc6503a3664
Guard access to marionette socket with a lock, r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/ccd3c31983d2
Deserialize marionette message earlier, r=webdriver-reviewers,whimboo
Upstream PR merged by moz-wptsync-bot
Flags: needinfo?(james)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: