Skip to content

Fix GC borrow hazards triggered by LoadBlocker::terminate#34122

Merged
jdm merged 5 commits intoservo:mainfrom
taniishkaaa:issue-34108
Nov 4, 2024
Merged

Fix GC borrow hazards triggered by LoadBlocker::terminate#34122
jdm merged 5 commits intoservo:mainfrom
taniishkaaa:issue-34108

Conversation

@taniishkaaa
Copy link
Copy Markdown
Contributor

@taniishkaaa taniishkaaa commented Nov 3, 2024

Fixes GC borrow hazards triggered by LoadBlocker::terminate

@taniishkaaa taniishkaaa requested a review from gterzian as a code owner November 3, 2024 16:09
jdm
jdm approved these changes Nov 4, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Thank you!

components/script/document_loader.rs
Comment on lines +54 to +58
if let Some(this) = blocker.borrow().as_ref() {
let load_data = this.load.clone().unwrap();
this.doc.finish_load(load_data, can_gc);
}
*blocker = None;
*blocker.borrow_mut() = None;
Copy link
Copy Markdown
Member

@jdm jdm Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's write this as:

let Some(mut this) = blocker.borrow_mut().take() else { return };
this.doc.finish_load(this.load.take().unwrap(), can_gc);

This resets the value to None before calling finish_load, unlike the current implementation, but that should not make a difference as far as I can tell.

components/script/dom/htmlimageelement.rs Outdated
Comment on lines +825 to +828
request.blocker = DomRefCell::new(Some(LoadBlocker::new(
&document,
LoadType::Image(url.clone()),
)));
Copy link
Copy Markdown
Member

@jdm jdm Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
request.blocker = DomRefCell::new(Some(LoadBlocker::new(
&document,
LoadType::Image(url.clone()),
)));
*request.blocker.borrow_mut() = Some(LoadBlocker::new(
&document,
LoadType::Image(url.clone()),
));

@taniishkaaa taniishkaaa requested a review from jdm November 4, 2024 09:27
@taniishkaaa
Copy link
Copy Markdown
Contributor Author

taniishkaaa commented Nov 4, 2024

Do I allow this?

@jdm
Copy link
Copy Markdown
Member

jdm commented Nov 4, 2024

Oh, interesting. That's subtle! Let's stick with the code that you wrote instead of my suggestion.

@taniishkaaa
Copy link
Copy Markdown
Contributor Author

taniishkaaa commented Nov 4, 2024

Oh, interesting. That's subtle! Let's stick with the code that you wrote instead of my suggestion.

Alright, no problem!

@jdm jdm added this pull request to the merge queue Nov 4, 2024
github-merge-queue bot pushed a commit that referenced this pull request Nov 4, 2024
@taniishkaaa
Fix GC borrow hazards triggered by LoadBlocker::terminate (#34122)
7bc38b5 
* Fix GC borrow hazards triggered by LoadBlocker::terminate

Signed-off-by: taniishkaaa <[email protected]>

* Fix clippy warnings

Signed-off-by: taniishkaaa <[email protected]>

* Use borrow_mut()

Signed-off-by: taniishkaaa <[email protected]>

* Revert to previous code due to crown unrooted error

Signed-off-by: taniishkaaa <[email protected]>

---------

Signed-off-by: taniishkaaa <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 4, 2024
@Loirooriol
Copy link
Copy Markdown
Contributor

Loirooriol commented Nov 4, 2024 •
edited
Loading

Seems to affect Range tests: https://github.com/servo/servo/actions/runs/11669312577

    OK [expected ERROR] /dom/ranges/Range-cloneContents.html
    OK [expected ERROR] /dom/ranges/Range-deleteContents.html
    OK [expected ERROR] /dom/ranges/Range-extractContents.html
    OK [expected ERROR] /dom/ranges/Range-insertNode.html
    OK [expected ERROR] /dom/ranges/Range-surroundContents.html

@taniishkaaa
Copy link
Copy Markdown
Contributor Author

taniishkaaa commented Nov 4, 2024

Seems to affect Range tests: https://github.com/servo/servo/actions/runs/11669312577

    OK [expected ERROR] /dom/ranges/Range-cloneContents.html
    OK [expected ERROR] /dom/ranges/Range-deleteContents.html
    OK [expected ERROR] /dom/ranges/Range-extractContents.html
    OK [expected ERROR] /dom/ranges/Range-insertNode.html
    OK [expected ERROR] /dom/ranges/Range-surroundContents.html

Should I remove the respective files to update test expectations?

@Loirooriol
Copy link
Copy Markdown
Contributor

Loirooriol commented Nov 4, 2024

I don't know if it's expected or not to affect these tests. But if so, you can update the expectations like

./mach build -r
./mach test-wpt --release --headless --log-raw /tmp/servo.log /dom/ranges/Range-cloneContents.html /dom/ranges/Range-deleteContents.html /dom/ranges/Range-extractContents.html /dom/ranges/Range-insertNode.html /dom/ranges/Range-surroundContents.html
./mach update-wpt /tmp/servo.log

@jdm jdm added this pull request to the merge queue Nov 4, 2024
Merged via the queue into servo:main with commit cc6163d Nov 4, 2024
@mukilan mukilan mentioned this pull request Apr 14, 2025
Merged
mukilan added a commit to mukilan/servo that referenced this pull request Apr 14, 2025
@mukilan
script: LoadBlocker's drop impl shouldn't run after termination.
d5caade 
The logic in LoadBlocker::terminate was modified in servo#34122 to `clone`
the LoadBlocker's inner `load` member instead of `take`ing it. However,
this member serves as a flag so that `LoadBlocker`'s Drop impl can avoid
calling `doc.finish_load` on already terminated loads. The change in
unnecessary 'unknown completed load' warnings when Servo is run with
logging enabled.

Signed-off-by: Mukilan Thiyagarajan <[email protected]>
mukilan added a commit to mukilan/servo that referenced this pull request Apr 16, 2025
@mukilan
script: LoadBlocker's drop impl shouldn't run after termination.
eac7211 
The logic in LoadBlocker::terminate was modified in servo#34122 to `clone`
the LoadBlocker's inner `load` member instead of `take`ing it. However,
this member serves as a flag so that `LoadBlocker`'s Drop impl can avoid
calling `doc.finish_load` on already terminated loads. The change in
unnecessary 'unknown completed load' warnings when Servo is run with
logging enabled.

This patch also updates the expectations for tests which started passing
after the change in servo#34122, but now fail again with this change. These
test failures will be investigated in servo#36561.

Signed-off-by: Mukilan Thiyagarajan <[email protected]>
mukilan added a commit to mukilan/servo that referenced this pull request Apr 17, 2025
@mukilan
script: LoadBlocker's drop impl shouldn't run after termination.
94619a9 
The logic in LoadBlocker::terminate was modified in servo#34122 to `clone`
the LoadBlocker's inner `load` member instead of `take`ing it. However,
this member serves as a flag so that `LoadBlocker`'s Drop impl can avoid
calling `doc.finish_load` on already terminated loads. The change in
unnecessary 'unknown completed load' warnings when Servo is run with
logging enabled.

This patch also updates the expectations for tests which started passing
after the change in servo#34122, but now fail again with this change. These
test failures will be investigated in servo#36561.

Signed-off-by: Mukilan Thiyagarajan <[email protected]>
mukilan added a commit to mukilan/servo that referenced this pull request Apr 21, 2025
@mukilan
script: LoadBlocker's drop impl shouldn't run after termination.
8180292 
The logic in LoadBlocker::terminate was modified in servo#34122 to `clone`
the LoadBlocker's inner `load` member instead of `take`ing it. However,
this member serves as a flag so that `LoadBlocker`'s Drop impl can avoid
calling `doc.finish_load` on already terminated loads. The change in
unnecessary 'unknown completed load' warnings when Servo is run with
logging enabled.

This patch also updates the expectations for tests which started passing
after the change in servo#34122, but now fail again with this change. These
test failures will be investigated in servo#36561.

Signed-off-by: Mukilan Thiyagarajan <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Apr 21, 2025
@mukilan
script: LoadBlocker's drop impl shouldn't run after termination. (#…
2a723a5 
…36508)

The logic in LoadBlocker::terminate was modified in #34122 to `clone`
the LoadBlocker's inner `load` member instead of `take`ing it. However,
this member serves as a flag so that `LoadBlocker`'s Drop impl can avoid
calling `doc.finish_load` on already terminated loads. The change
results in
unnecessary 'unknown completed load' warnings when Servo is run with
logging enabled.

Signed-off-by: Mukilan Thiyagarajan <[email protected]>

Signed-off-by: Mukilan Thiyagarajan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gterzian gterzian Awaiting requested review from gterzian gterzian is a code owner

@jdm jdm Awaiting requested review from jdm

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

LoadBlocker::terminate is a common trigger for GC borrow hazards

3 participants

@taniishkaaa @jdm @Loirooriol