servo icon indicating copy to clipboard operation
servo copied to clipboard

Rewrite readable stream implementation without spidermonkey builtin object

Open jdm opened this issue 3 years ago • 14 comments

The next spidermonkey upgrade will bring in https://bugzilla.mozilla.org/show_bug.cgi?id=1795914, which removes the JS implementation. We will need to port https://bugzilla.mozilla.org/show_bug.cgi?id=1730589 and subsequent changes from https://searchfox.org/mozilla-central/source/dom/streams/ReadableStream.cpp before we can upgrade again.

jdm avatar Nov 21 '22 15:11 jdm

Relevant base code in servo is https://github.com/servo/servo/blob/master/components/script/dom/readablestream.rs

jdm avatar Nov 21 '22 15:11 jdm

Depends on https://github.com/servo/servo/pull/29079

jdm avatar Nov 21 '22 15:11 jdm

I'm working on this. It looks like it might be easier than I anticipated, since we aren't actually implementing the full WHATWG Streams API defined in https://streams.spec.whatwg.org/.

jdm avatar Jun 04 '23 16:06 jdm

I've pushed my (very rough) in-progress branch to https://github.com/servo/servo/compare/master...jdm:servo:no-streams?expand=1 . I was using tests/wpt/web-platform-tests/xhr/overridemimetype-blob.html as the initial testcase; right now this subtest times out because the promises aren't queued correctly.

jdm avatar Jun 16 '23 02:06 jdm

Note to self: consume_stream in script_runtime.rs will also need to be removed/replaced.

jdm avatar Jun 16 '23 02:06 jdm

The latest commit allows the majority of the tests in XHR to pass. The idlharness tests crash in the JS engine because the memory safety story for readable streams is much less clear than it used to be. My goal once that crash is fixed is to clean up the code and then document the state of the world so it becomes easier to reason about how to make it less complex. It's... difficult to follow at the moment.

jdm avatar Jun 16 '23 04:06 jdm

I also started working on prototyping streams, but I left it due to time constrains at that time and I do not plan to comeback working on it anytime soon. It does not compile, but some initial work has been laid down (mostly webidls, which for some reason need to be splitted over multiple files or else codegen does not work).

sagudev avatar Jun 16 '23 04:06 sagudev

We will need to decide if we turn off readable streams in mozjs as part of this work. That would definitely make the test results more honest.

jdm avatar Jun 17 '23 05:06 jdm

There is --enable-js-streams.

sagudev avatar Jun 17 '23 05:06 sagudev

Yep. I think I'd like to get the tests all passing with js streams enabled, then remove that flag and see what the impact is on the test coverage.

jdm avatar Jun 17 '23 05:06 jdm

We decided to not block SM upgrade on this and port JS streams to SM ESR 115 in https://github.com/servo/mozjs/pull/408. We should still do this sometimes in the future, preferably before next ESR.

sagudev avatar Sep 21 '23 16:09 sagudev

The title of this issue is more straight forward so I'll leave the progress here. So we decided to create another branch, readablestream-re-implementation, to work on this.

Here's a rough overview of the progress. I'll add more details as it proceeds:

  • [x] ReadableStream WebIDL codegen: #32605
    • [ ] Constructor: https://github.com/servo/servo/pull/32634
    • [ ] ReadableStreamMethods implementation
      • [ ] Locked
      • [ ] Cancel
      • [ ] GetReader
  • [x] ReadableStreamDefaultController WebIDL codegen
    • [ ] SetUpReadableStreamDefaultControllerFromUnderlyingSource
    • [ ] ReadableStreamDefaultControllerMethods implementation: todo
      • [ ] GetDesiredSize
      • [ ] Close
      • [ ] Enqueue
      • [ ] Error
  • [x] ReadableBytesStreamController WebIDL codegen
    • [ ] SetUpReadableByteStreamControllerFromUnderlyingSource
    • [ ] ReadableBytesStreamControllerMethods implementation: todo
      • [ ] GetByobRequest
      • [ ] GetDesiredSize
      • [ ] Close
      • [ ] Enqueue
      • [ ] Error
  • [x] ReadableStreamDefaultReader WebIDL codegen: #32605
    • [ ] Constructor: todo
    • [ ] ReadableStreamDefaultReaderMethods implementation: todo
      • [ ] Read
      • [ ] ReleaseLock
      • [ ] Closed
      • [ ] Cancel
  • [x] ReadableStreamBYOBReader WebIDL codegen: #32605
    • [ ] Constructor: todo
    • [ ] ReadableStreamBYOBReaderMethods implementation: todo
      • [ ] Read
      • [ ] ReleaseLock
      • [ ] Closed
      • [ ] Cancel
  • [x] QueuingStrategy: #32572
  • [x] UnderlyingSource: #32572

For anyone who is interested in contributing, here's what you could do:

  • For "todo" item, please open PR against my fork, or wait for that interface is available in servo's branch.
  • For "draft complete" or items that has a opened PR, please help me review and see if the implementation is correct.

wusyong avatar Jun 25 '24 06:06 wusyong

I don't think all the "todo" items can be done separately, I would say let's complete the WebIDL skeleton first, then we will have to look at everything again and come-up with a plan for the implementation.

gterzian avatar Jun 25 '24 11:06 gterzian

@gterzian I think you are right. I just opened https://github.com/servo/servo/pull/32602 but I think it will be super difficult to review.

The only missing WebIDL is ReadableStream left. I guess it's better to close that draft to fill the skeleton first.

wusyong avatar Jun 25 '24 11:06 wusyong