script: Break the ScriptThread dependency on the initial Pipeline
#40918
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdm
approved these changes
Nov 27, 2025
| ) -> JoinHandle<()> { | ||
| let webview_id = WebViewId::installed(); | ||
| let event_loop_id = ScriptEventLoopId::installed() | ||
| .expect("A DedicatedWorker should always be run from a ScriptThread context"); |
Member
There was a problem hiding this comment.
What about the case where a worker is launched from another worker?
Member
Author
There was a problem hiding this comment.
This expect message is a bit confusing, so I'll update it. Essentially, the thread should either be a ScriptThread or a dedicated worker launched from another ScriptThread in which case the ScriptEventLoopId should also be installed. That said, maybe the ScriptEventLoopId should also be installed.
| .expect("Thread spawning failed") | ||
| } | ||
|
|
||
| pub(crate) fn webview_id(&self) -> Option<WebViewId> { |
Member
Author
There was a problem hiding this comment.
Oh, this doesn't need to be an Option at all. I'll address it.
1e16e54 to
12653a0
Compare
12653a0 to
5687592
Compare
`ScriptThread`s start with an initial `Pipeline`, but that `Pipeline` should not be used for any `ScriptThread` global data as a `ScriptThread` can have any number of `Pipeline`s that come and go. There's no architectural reason why a `ScriptThread` should be associated with only a single `WebView`. This change makes it so that the `ScriptThread` no longer uses the `PipelineId` and the `WebViewId` of the initial `Pipeline` to initialize itself. Instead a `ScriptEventLoopId` is used to uniquely identify every `ScriptThread` in both the `ScriptThread` and the `Constellation`. The remaining use was for crash reporting. Now when a crash happens, it launches the sad tab in all `WebView`s that depeneded on that `ScriptThread`. This is a change which should allow simplifying the way that `EventLoop`s/`ScriptThread`s are started in the `Constellation`, which will be handled in a followup. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
5687592 to
13bbcd3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ScriptThreads start with an initialPipeline, but thatPipelineshould not be used for any
ScriptThreadglobal data as aScriptThreadcan have any number ofPipelines that come and go.There's no architectural reason why a
ScriptThreadshould beassociated with only a single
WebView.This change makes it so that the
ScriptThreadno longer uses thePipelineIdand theWebViewIdof the initialPipelineto initializeitself. Instead a
ScriptEventLoopIdis used to uniquely identify everyScriptThreadin both theScriptThreadand theConstellation.The remaining use was for crash reporting. Now when a crash happens, it
launches the sad tab in all
WebViews that depeneded on thatScriptThread.This is a change which should allow simplifying the way that
EventLoops/ScriptThreads are started in theConstellation, whichwill be handled in a followup.
Testing: This shouldn't change behavior except in the case where a
ScriptThreadis used in more than a singleWebView, which should workproperly now. I tested this change manually by running servoshell in
multiprocess mode while causing a panic to happen due to a null pointer
dereference. The sad tab appeared after this change, so things seem to work
properly still. Testing crashes is tricky with the way we test servo now.