New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix FileAPI's refcount implementation #12579
Conversation
Heads up! This PR modifies the following files:
|
These changes are based on https://github.com/izgzhen/gsoc-file-support/blob/master/notes/analysis.md, which is unfortunately a mess now (in continued revision...). Basically, the fixes aim at a correct slicing: After slicing, whatever happens to parent blob should be irrelevant for child blob. I will try to comment on each vital change as well. Also, wondering how to test that logic :/ Finally, our current design is a monster, maybe the most complicated thing I ever wrote. Hope this time it is done right. |
@@ -178,7 +178,7 @@ impl<UI: 'static + UIProvider> FileManager<UI> { | |||
if let Ok(id) = Uuid::parse_str(&id.0) { | |||
spawn_named("revoke blob url".to_owned(), move || { | |||
// Since it is revocation, unset_url_validity is true | |||
let _ = sender.send(store.dec_ref(&id, &origin, true)); | |||
let _ = sender.send(store.set_blob_url_validity(false, &id, &origin)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that as long as Blob
is not dropped or close
d, it holds a readable reference to resource by FileID
. Thus, revocation should not dec_ref
but just simply flip the validity bit.
I personally think that reading through the new implementation ( |
self.remove(id); | ||
|
||
if let Some(parent_id) = opt_parent_id { | ||
// unset_url_validity for parent is false since we only need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment isn't necessary now?
Overall looks good. While we're at it, should we add a decref in the Blob destructor (specifically, the script-side BlobImpl destructor)? |
Ah, right, we did. Sgtm. |
ping? |
@bors-servo r+ |
📌 Commit 49ed453 has been approved by |
Fix FileAPI's refcount implementation Revise several intricate parts of FileAPI's internal refcounting-related implementation. Goal: Get it done right once and for all. r? @Manishearth <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because it is internal logic change <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12579) <!-- Reviewable:end -->
☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev |
Revise several intricate parts of FileAPI's internal refcounting-related implementation.
Goal: Get it done right once and for all.
r? @Manishearth
./mach build -d
does not report any errors./mach test-tidy
does not report any errorsThis change is