script: Prevent panic on multiple calls to MessagePort::close

[sabbCodes]

Converted ManagedMessagePort from a struct to an enum with Enabled and Disabled variants.

Testing: ran ./mach test-wpt tests/wpt/tests/webmessaging/message-channels/close.any.js result: ```web-platform-test

Ran 18 checks (16 subtests, 2 tests)
Expected results: 18
Unexpected results: 0
OK
```

Fixes #36765

[mukilan]

The changes look good to me, but I'll let @gterzian or someone else more familiar with message ports approve it, as I don't have the context to evaluate the correctness of the message in the code comment.

[jdm]

@sabbCodes Is it possible to write an automated test for this? We could add a crash test to ensure the browser stays running.

[sabbCodes]

I actually wrote a basic test earlier, I didn't commit it because it's not included in the scope. My PC is running low on power rn - Nigeria is happening to me - I'll look into it when I get my PC charged and complete my other assigned tasks

[gterzian]

Thanks for looking into this.

So the problem here is actually one of garbage collection.

Strictly speaking, you can call close as many times as you want in the same task, but after that the port is removed from the global because calling close sets the explicitly_closed flag, which is used when deciding to remove the port from the global.

So you could write two tests, and add them to https://github.com/servo/servo/blob/afa4b524ab75c28f5eb27872e6e8ba90f451c8b5/tests/wpt/tests/webmessaging/message-channels/close.any.js

One would be "close is idempotent within a task", where you just call close twice in a row, and the second one would be "close is idempotent across tasks".

For the second one, you'd have to keep a ref to the port and then queue a timer or something like that, and then call close a second time from within the subsequent task.

You can then confirm that what I say is correct, by removing the current changes(so leaving the panic), and confirming that only the second test panics.

---

As to the changes to be made to the code, I think we should take this one step further and do the following:

• Dom<MessagePort> should be turned into an enum, with an Enabled(Dom<MessagePort>) variant and another Disabled(WeakRef<MessagePort>) variant.
• The explicitly_closed should be removed, and when the port is closed, the dom should be downgraded to a weak ref.
• At that point JS should not be able to call Close unless it holds a ref, meaning that the panic can stay, but it should be turned into a debug_assert!

@jdm please confirm that what I say about using the weakref makes sense.

[jdm]

Yes, I think.that design is sensible!

[sabbCodes]

Hi @gterzian I've been trying to work on your suggestion, however I think I may need some guidance if I'm going to be able to finish this, right now, I've tried converting the ManagedMessagePort struct to an enum, deleted explicitly_closed bool, and tried updating perform_a_message_port_garbage_collection_checkpoint

here's how I updated my test;

    const channel = new MessageChannel();
    channel.port1.close();
    // Calling close() again in the same task must not throw/panic.
    channel.port1.close();
}, "close is idempotent within a task");

async_test(t => {
    const channel = new MessageChannel();
    channel.port1.close();
    // Queue a timer and call close() again from the next task.
    setTimeout(t.step_func_done(() => {
        channel.port1.close(); // must not panic or throw
    }), 0);
}, "close is idempotent across tasks");

below is the result I get on running ./mach test-wpt tests/wpt/tests/webmessaging/message-channels/close.any.js

Warning: could not load product 'firefox' for argument registration: No module named 'mozleak'
No build type specified so assuming `--dev`.
Running WPT tests with /home/sabb/servo/target/debug/servoshell
 0:59.94 wptserve INFO Starting http server on http://127.0.0.1:8001
 1:00.01 wptserve INFO Starting https server on https://127.0.0.1:8443
 1:00.51 wptserve INFO Starting http server on http://127.0.0.1:8000
 1:03.27 wptserve INFO Create socket on: (<AddressFamily.AF_INET: 2>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('127.0.0.1', 8888))
 1:03.27 wptserve INFO Bind on: (<AddressFamily.AF_INET: 2>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('127.0.0.1', 8888))
 1:03.27 wptserve INFO Listen on: (<AddressFamily.AF_INET: 2>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('127.0.0.1', 8888))
 1:04.23 wptserve INFO Starting http server on http://127.0.0.1:8002
 1:04.88 wptserve INFO Create socket on: (<AddressFamily.AF_INET: 2>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('127.0.0.1', 8889))
 1:05.06 wptserve INFO Bind on: (<AddressFamily.AF_INET: 2>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('127.0.0.1', 8889))
 1:05.06 wptserve INFO Listen on: (<AddressFamily.AF_INET: 2>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('127.0.0.1', 8889))
 1:05.12 wptserve INFO Starting https server on https://127.0.0.1:8446
 1:07.46 wptserve INFO Starting https server on https://127.0.0.1:8444
 1:07.73 wptserve INFO Starting https server on https://127.0.0.1:8445
 1:11.37 wptserve INFO Starting http server on http://127.0.0.1:8003
 1:11.54 wptserve INFO Starting http2 server on https://127.0.0.1:9000
 1:11.84 SUITE_START: web-platform-test - running 2 tests
 1:11.84 INFO Using 2 child processes
 1:11.92 WARNING Browser.__init__ kwargs: {'capabilities': None}
 1:11.96 INFO Starting WebDriver: /home/sabb/servo/target/debug/servoshell --webdriver=45589 --hard-fail --ignore-certificate-errors --enable-experimental-web-platform-features --window-size 800x600 about:blank --certificate-path /home/sabb/servo/tests/wpt/tests/tools/certs/cacert.pem --pref=network_http_proxy_uri= --pref=network_https_proxy_uri= --config-dir /tmp/servo-1ucfmqz3 --headless --prefs-file /home/sabb/servo/resources/wpt-prefs.json
 1:12.16 WARNING Browser.__init__ kwargs: {'capabilities': None}
 1:12.20 INFO Starting WebDriver: /home/sabb/servo/target/debug/servoshell --webdriver=37485 --hard-fail --ignore-certificate-errors --enable-experimental-web-platform-features --window-size 800x600 about:blank --certificate-path /home/sabb/servo/tests/wpt/tests/tools/certs/cacert.pem --pref=network_http_proxy_uri= --pref=network_https_proxy_uri= --config-dir /tmp/servo-1ucfmqz3 --headless --prefs-file /home/sabb/servo/resources/wpt-prefs.json
 1:16.54 INFO Webdriver started successfully.
 1:16.54 INFO Starting runner
 1:17.00 INFO Webdriver started successfully.
 1:17.00 INFO Starting runner
 1:27.38 TEST_START: /webmessaging/message-channels/close.any.html
 1:34.47 pid:49173 Full command: /home/sabb/servo/target/debug/servoshell --webdriver=45589 --hard-fail --ignore-certificate-errors --enable-experimental-web-platform-features --window-size 800x600 about:blank --certificate-path /home/sabb/servo/tests/wpt/tests/tools/certs/cacert.pem --pref=network_http_proxy_uri= --pref=network_https_proxy_uri= --config-dir /tmp/servo-1ucfmqz3 --headless --prefs-file /home/sabb/servo/resources/wpt-prefs.json
pid:49173 close_message_port called on an unknown port. (thread Script#3, at components/script/dom/globalscope.rs:1216)
 1:36.70 TEST_START: /webmessaging/message-channels/close.any.worker.html
 1:46.16 pid:49185 Full command: /home/sabb/servo/target/debug/servoshell --webdriver=37485 --hard-fail --ignore-certificate-errors --enable-experimental-web-platform-features --window-size 800x600 about:blank --certificate-path /home/sabb/servo/tests/wpt/tests/tools/certs/cacert.pem --pref=network_http_proxy_uri= --pref=network_https_proxy_uri= --config-dir /tmp/servo-1ucfmqz3 --headless --prefs-file /home/sabb/servo/resources/wpt-prefs.json
pid:49185 close_message_port called on an unknown port. (thread WW:web-platform.test:8000/webmessaging/message-channels/close.any.worker.js, at components/script/dom/globalscope.rs:1216)
 1:49.59 TEST_END: TIMEOUT, expected OK - Executor hit external timeout (this may indicate a hang)

 1:49.61 INFO No more tests
 1:51.72 INFO Trying to shut down gracefully by extension command
 1:52.81 TEST_END: Test TIMEOUT, expected OK. Subtests passed 6/8. Unexpected 2
TIMEOUT Message sent to closed port from transferred port should not arrive. - Test timed outTIMEOUT close is idempotent across tasks - Test timed outTIMEOUT /webmessaging/message-channels/close.any.worker.html
 1:52.82 INFO No more tests
 1:53.02 INFO Trying to shut down gracefully by extension command
 1:54.17 INFO Trying to shut down gracefully by extension command
 1:55.27 WARNING Max retry exceeded to normally shut down. Killing instead.
 1:55.34 INFO Trying to shut down gracefully by extension command
 1:56.51 INFO Trying to shut down gracefully by extension command
 1:49.65 INFO Closing logging queue
 1:49.67 INFO queue closed
 1:57.64 INFO Trying to shut down gracefully by extension command
 1:58.75 WARNING Max retry exceeded to normally shut down. Killing instead.
 1:52.87 INFO Closing logging queue
 1:52.91 INFO queue closed
 1:59.29 SUITE_END

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 10 checks (8 subtests, 2 tests)
Expected results: 6
Unexpected results: 4
  test: 2 (2 timeout)
  subtest: 2 (2 timeout)

Unexpected Results
------------------
/webmessaging/message-channels/close.any.html
  TIMEOUT /webmessaging/message-channels/close.any.html - Executor hit external timeout (this may indicate a hang)
/webmessaging/message-channels/close.any.worker.html
  TIMEOUT Message sent to closed port from transferred port should not arrive. - Test timed out
  TIMEOUT close is idempotent across tasks - Test timed out
  TIMEOUT /webmessaging/message-channels/close.any.worker.html
 1:59.30 INFO Got 2 unexpected results, with 0 unexpected passes
 1:59.33 wptserve INFO Stopped http server on 127.0.0.1:8003
 1:59.35 wptserve INFO Stopped http server on 127.0.0.1:8001
 1:59.35 wptserve INFO Stopped http server on 127.0.0.1:8002
 1:59.35 wptserve INFO Stopped http server on 127.0.0.1:8000
 1:59.35 wptserve INFO Stopped http server on 127.0.0.1:8443
 1:59.36 wptserve INFO Stopped http server on 127.0.0.1:8444
 1:59.38 wptserve INFO Stopped http server on 127.0.0.1:8445
 1:59.38 wptserve INFO Stopped http server on 127.0.0.1:8446
 1:59.41 wptserve INFO Stopped http server on 127.0.0.1:9000
 1:59.41 wptserve INFO Close on: (<AddressFamily.AF_INET: 2>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('127.0.0.1', 8889))
 1:59.48 wptserve INFO Close on: (<AddressFamily.AF_INET: 2>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('127.0.0.1', 8888))
 2:02.43 INFO Closing logging queue
 2:02.44 INFO queue closed

[eerii]

Could you upload your changes to this PR so that we can provide better feedback? :)

[sabbCodes]

Could you upload your changes to this PR so that we can provide better feedback? :)

My PC about to shutdown, pushing as soon as I recharge

[servo-wpt-sync]

🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#58886) with upstreamable changes.

[servo-wpt-sync]

✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#58886) title and body.

[servo-wpt-sync]

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#58886).

[sabbCodes]

Could you upload your changes to this PR so that we can provide better feedback? :)

I prevented it from panicking when cross-task messages are sent to a port that had already been closed and garbage collected, then it passed the tests.

[mukilan]

@sabbCodes

I've tried converting the ManagedMessagePort struct to an enum,

My understanding of @gterzian 's comments is that just the dom_port field inside ManagedMessagePort should be an enum, not the whole structure. Any reason you made the whole structure an enum? It would be good if you can share if you already tried the suggested approach and encountered any problems.

[sabbCodes]

@sabbCodes

I've tried converting the ManagedMessagePort struct to an enum,

My understanding of @gterzian 's comments is that just the dom_port field inside ManagedMessagePort should be an enum, not the whole structure. Any reason you made the whole structure an enum? It would be good if you can share if you already tried the suggested approach and encountered any problems.

These was actually part of my confusion, I thought the entire ManagedMessagePort was to be converted to a struct.

Lemme try converting just the domport

[servo-wpt-sync]

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#58886).

[sabbCodes]

Hi @mukilan if you can find the time to review, I have addressed @gterzian's previous suggestion