Open Bug 1776754 Opened 4 years ago Updated 2 years ago

Non-JSON data when handling an incoming message causes an internal JavaScript error and skips sending the response

Categories

(Remote Protocol :: Agent, defect, P3)

Product:
Remote Protocol
Component:
Agent
Type:
defect

Tracking

(Not tracked)

People

(Reporter: whimboo, Unassigned, Mentored)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [webdriver:backlog][lang=js])

Attachments

(3 files)

Python test
257 bytes, text/plain
Details
Bug 1776754 - Non-JSON data when handling an incoming message causes an internal JavaScript error and skips sending the response
48 bytes, text/x-phabricator-request
Details | Review
Bug 1776754 - updated packet handling
48 bytes, text/x-phabricator-request
Details | Review
Attached file Python test —

When a message is received via the WebSocket our WebSocketTransport.onMessage() will raise a JavaScript error if the data is not a JSON string. Sadly this skips sending the response to the client and as such causes a hang for WebDriver BiDi clients.

See the attached Python script as example.

Here the Javascript error that can be seen:

JavaScript error: chrome://remote/content/server/WebSocketTransport.jsm, line 89: SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data

As given by the spec we have to return with an invalid argument error:
https://w3c.github.io/webdriver-bidi/#handle-an-incoming-message

That means that parsing the JSON data needs to be done by the individual protocol so that protocol specific failures can be returned.

When moving JSON.parse() the packet event should then emit the data as received, and parsing the data needs to happen in both the WebDriver BiDi and CDP protocols:

https://searchfox.org/mozilla-central/rev/9379d512333cb143ed6ee5df5a40274f40d6ce87/remote/webdriver-bidi/WebDriverBiDiConnection.jsm#160
https://searchfox.org/mozilla-central/rev/9379d512333cb143ed6ee5df5a40274f40d6ce87/remote/cdp/CDPConnection.jsm#231

To verify the changes the following tests need to be run:

./mach run remote/cdp/test/browser
./mach run testing/web-platform/tests/webdriver/tests/bidi/
URL: https://firefox-source-docs.mozilla.o...
Mentor: hskupin
Priority: -- → P3
Whiteboard: [webdriver:backlog][lang=js]

@whimboo In other words, we need to remove the logic of JSON.parse() from WebSocketTransport and integrate to the WebDriverBiDiConnection.jsm and CDPConnection.jsm onPacket() method, right?

@sayuree, sorry for the late reply but I was off the last 3 weeks.

Yes, we will have to let each protocol actually parse the JSON data and return with the proper response. Only that way we can allow different protocols to use custom types eg. specific error messages.

Would you be interested to work on this bug? I'm happy to mentor you in whatever you need to get this implemented.

Severity: -- → S3
Flags: needinfo?(sabina.zaripova)

@whimboo I have added changes and wanted to run the tests for bidi using ./mach run testing/web-platform/tests/webdriver/tests/bidi/, but I cannot understand how does it work, can you please help with this part?

Flags: needinfo?(sabina.zaripova) → needinfo?(hskupin)

mach run is not supposed to run tests. You will have to use mach test or directly mach wpt (for web-platform-tests) instead. It will look like the following:

mach wpt --webdriver-arg=-vv --webdriver-binary=target/debug/geckodriver --setpref="remote.log.level=Trace" testing/web-platform/tests/webdriver/tests/bidi/

I assume that you are running an artifact build of Firefox? If yes you will also have to build geckodriver first. For that change into the testing/geckodriver directory and run cargo build.

Please note that we also might need a new test that explicitly checks that the correct response is sent by the WebSocket server.

Flags: needinfo?(hskupin)

Sayuree, I want to ask if my last comment was helpful to get the tests running and geckodriver built. Please let us know. Thanks!

Flags: needinfo?(sabina.zaripova)

I have built geckodriver and followed your directions, but still I keep getting an error on

mach wpt --webdriver-arg=-vv --webdriver-binary=target/debug/geckodriver --setpref="remote.log.level=Trace" testing/web-?>platform/tests/webdriver/tests/bidi/
The error message is as follows:
The details of the failure are as follows:

subprocess.CalledProcessError: Command '['git', 'diff-index', '--relative', '--no-renames', '--name-only', '-z', 'HEAD']' returned non-zero exit status 128.

Flags: needinfo?(sabina.zaripova) → needinfo?(hskupin)

(In reply to sayuree from comment #8)

I have built geckodriver and followed your directions, but still I keep getting an error on

mach wpt --webdriver-arg=-vv --webdriver-binary=target/debug/geckodriver --setpref="remote.log.level=Trace" testing/web-?>platform/tests/webdriver/tests/bidi/

The error message is as follows:

The details of the failure are as follows:

subprocess.CalledProcessError: Command '['git', 'diff-index', '--relative', '--no-renames', '--name-only', '-z', 'HEAD']' returned non-zero exit status 128.

Can you please attach the full log? Also is the extra question mark in web-?>platform expected?

Flags: needinfo?(hskupin) → needinfo?(sabina.zaripova)

Sabina, are you still interested to work on this bug?

Hey Henrik,
Can I work on this bug?

Flags: needinfo?(hskupin)

Sorry, but I was away last week. Sure, please get started and let me know if something isn't clear. As it looks like Sabina isn't going to continue on this bug. Thanks.

Flags: needinfo?(ssyed825)
Flags: needinfo?(sabina.zaripova)
Flags: needinfo?(hskupin)

Hi,

Sorry, but I was away last week.

No problem 😄

Sure, please get started and let me know if something isn't clear.

Thanks. Will start the work right away.

Flags: needinfo?(ssyed825)

Hi Scuzzy, I have seen that you ask a question on Element and I replied, but didn't get an answer yet. I wonder if you could share the status in where you currently are and if you need further help. Thanks.

Flags: needinfo?(ssyed825)

Hi Henrik,
I am facing issues with running the tests. I wanted to make sure that all the tests are passing before making any changes but there are some failing test cases. I have asked for help on this as well.
I am working on it actively and will keep you updated (on Element).

Flags: needinfo?(ssyed825)

Hi Henrik,
I won't be able to work on this bug anymore as I am occupied in some other activities.

Hi Henrik, can I take this up?

Is it still open for Contribution?

Flags: needinfo?(hskupin)

Yes, you can. As long as there is no patch attached the bugs are fee to get picked up. Thanks!

Flags: needinfo?(hskupin)

Yash, are you still interested to work on this bug? If yes are you blocked on something and where you would need help with? Please let us know. Thanks.

Flags: needinfo?(yshsaxena23)

Given that Yash doesn't seem to be interested this bug is open for others now. Thanks.

Flags: needinfo?(yshsaxena23)

I believe I can proceed with this?

(In reply to ofrazy from comment #23)

I believe I can proceed with this?

Yes, you are very welcome to have a look at this bug. Us usual please let me know if something is unclear. Thanks!

Flags: needinfo?(ofrazy)

Sorry for the inactivity, I was away for the past 2 weeks.
I believe I have enough context from the previous conversations from the chat. In case of any issues I will reach out via element.

Flags: needinfo?(ofrazy)

We briefly discussed intermittent steps to get to the final patch on Matrix. ofrazy, maybe you could give a quick update on where you are right now? Thanks a lot!

Flags: needinfo?(ofrazy)
Assignee: nobody → ofrazy
Status: NEW → ASSIGNED

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #26)

We briefly discussed intermittent steps to get to the final patch on Matrix. ofrazy, maybe you could give a quick update on where you are right now? Thanks a lot!

Flags: needinfo?(ofrazy)

Hi ofrazy, after close to a month when we have heard each other I want to ask again if you are still interested to finish up this particular bug. Thanks in advance.

Flags: needinfo?(ofrazy)

:whimboo will be on PTO for 3 weeks, in the meantime please reach out to me if there is anything I can do to help with the bug.
I see you were almost done with the patch here, it would be great to push it through the finish line :) Happy to help, don't hesitate to ping me.

I'm going to reset the assignment given that I haven't heard from ofrazy anymore within the last 2 months. It means the bug is free to get picked-up by someone else.

Assignee: ofrazy → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(ofrazy)

Hi, I would like to work on this.

That sounds great Mitesh! Please have a look at the already attached patch which gives you a starting point and that you do not have to start from zero. Lets me know if you face issues.

Assignee: nobody → miteshgulecha
Status: NEW → ASSIGNED

Hi, sorry I totally forgot about this, I already pushed the code on Phabricator along with the tests. Please let me know if it needs any other changes.

Hi Mitesh, as recently discussed we can ignore the automated tests for the changes of the CDP implementation. But we would still need those tests for WebDriver BiDi. Overall that should hopefully give us a boost forward to get the needed changes done.

Please let me know if you still want to work on it and also have the time. Thanks!

Flags: needinfo?(miteshgulecha)

Hi Mitesh, while we had a conversion outside of Bugzilla on Matrix I wanted to ask here again if you had the time to make some progress on this issue. If you don't have time right now don't worry. Thanks.

Hi sorry for delaying, this weekend, I will be working dedicatedly on this.

Flags: needinfo?(miteshgulecha)

After talking to Mitesh we are going to unassign for now so that someone else could take a look if wanted.

Assignee: miteshgulecha → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: