-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add file backend support for Blob and related #11221
Conversation
Heads up! This PR modifies the following files:
|
@Manishearth I refined the design a bit and I think we can push the progress of this thread in parallel with #11189 |
typeString: String, | ||
isClosed_: Cell<bool>, | ||
} | ||
|
||
#[derive(Clone, JSTraceable)] | ||
pub struct BlobImpl { | ||
slice: DOMRefCell<Option<DataSlice>>, |
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.
Perhaps this should be an enum?
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.
I am thinking about the possibility of caching here. If it is file-backed, we can put the cached content inside the other DataSlice
field.
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.
I'd rather have an enum here and within the FileId variant have an option for caching. As it stands slice = Some can mean cached or an in-memory slice, a clear boundary would be nicer and more Rusty.
New code was committed to pull request. |
@Manishearth In order to get port to filemanger thread inside blob (by |
Exposing it won't hurt anything; the storage APIs aren't accessible from workers, so there hasn't been any need to make the storage thread available. |
Any particular reason for workers not being able to use storage? If there's a security reason we should not even give workers an unused storage chan (sandboxing). |
Seems like it used to be for security reasons but now it's just for "threading issues". |
I didn't see any |
Oh, sure, the spec doesn't let you, the question is: If a sandboxed worker was compromised with an RCE vulnerability and got access to storage data, is that an escalation of access? Assuming that we have checks in place in the resource/storage thread that ensure that no process gets data from a different origin (we don't, but we should eventually), I don't think this is an issue. Also, I don't see us putting workers in separate processes, so if a worker thread is compromised the script thread would be compromised too anyway. |
let file_manager: IpcSender<FileManagerThreadMsg> = unimplemented!(); | ||
let (chan, recv) = ipc::channel().unwrap(); | ||
let _ = file_manager.send(FileManagerThreadMsg::ReadFile(chan, id)); | ||
let result = recv.recv().unwrap(); |
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.
Should we be handling/propagating errors here?
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.
I think read_file
can be made Option<DataSlice>
and we can throw a script-level exception or something at the dom API where it is used (like Slice
)
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.
Result<DataSlice,()>
and use try!
or the ?
operator, but yeah
Almost done! |
New code was committed to pull request. |
☔ The latest upstream changes (presumably #10961) made this pull request unmergeable. Please resolve the merge conflicts. |
@@ -308,12 +308,17 @@ impl HTMLFormElement { | |||
DispositionParam::Filename(Charset::Ext(String::from(charset.clone())), | |||
None, | |||
f.name().clone().into())); | |||
/// XXX: unwrap |
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.
replace it with an expect
though in this case unwrap_or(text/plain)
might be better?
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.
Not sure, can't find related spec, maybe an empty ""
?
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.
Section 4.4, it defaults to text/plain
☔ The latest upstream changes (presumably #11225) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #11326) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #11368) made this pull request unmergeable. Please resolve the merge conflicts. |
⌛ Testing commit 43ad4ba with merge e83fb45... |
Add file backend support for Blob and related - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy --faster` does not report any errors - [x] These changes fix #10851, related to #11131 - [x] These changes do not require tests because the implementation is partial and can't work alone 1. Add new backend to `Blob` and a `BlobImpl` struct to abstract multiple backends 2. Rewrite most interfaces of `Blob` to accommodate the change 3. Change the `read` behaviour of `FileReader`, considering the case when blob is file-backed and not cached The design is still immature, welcome comments! - [x] I used `DOMRefCell` to cache the bytes in `BlobImpl`, is it sound? - [x] The interfaces (like `BlobImpl::get_bytes`) handle requests in a default-to-empty way when the inner `DataSlice` is not cached. It might be possible to handle this condition better. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11221) <!-- Reviewable:end -->
💔 Test failed - mac-rel-css |
|
@bors-servo try |
⌛ Trying commit 43ad4ba with merge decbabd... |
Add file backend support for Blob and related - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy --faster` does not report any errors - [x] These changes fix #10851, related to #11131 - [x] These changes do not require tests because the implementation is partial and can't work alone 1. Add new backend to `Blob` and a `BlobImpl` struct to abstract multiple backends 2. Rewrite most interfaces of `Blob` to accommodate the change 3. Change the `read` behaviour of `FileReader`, considering the case when blob is file-backed and not cached The design is still immature, welcome comments! - [x] I used `DOMRefCell` to cache the bytes in `BlobImpl`, is it sound? - [x] The interfaces (like `BlobImpl::get_bytes`) handle requests in a default-to-empty way when the inner `DataSlice` is not cached. It might be possible to handle this condition better. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11221) <!-- Reviewable:end -->
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows |
⌛ Testing commit 43ad4ba with merge 7293702... |
Add file backend support for Blob and related - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy --faster` does not report any errors - [x] These changes fix #10851, related to #11131 - [x] These changes do not require tests because the implementation is partial and can't work alone 1. Add new backend to `Blob` and a `BlobImpl` struct to abstract multiple backends 2. Rewrite most interfaces of `Blob` to accommodate the change 3. Change the `read` behaviour of `FileReader`, considering the case when blob is file-backed and not cached The design is still immature, welcome comments! - [x] I used `DOMRefCell` to cache the bytes in `BlobImpl`, is it sound? - [x] The interfaces (like `BlobImpl::get_bytes`) handle requests in a default-to-empty way when the inner `DataSlice` is not cached. It might be possible to handle this condition better. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11221) <!-- Reviewable:end -->
💔 Test failed - mac-rel-css |
@bors retry try-
|
@bors-servo retry try- |
Add file backend support for Blob and related - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy --faster` does not report any errors - [x] These changes fix #10851, related to #11131 - [x] These changes do not require tests because the implementation is partial and can't work alone 1. Add new backend to `Blob` and a `BlobImpl` struct to abstract multiple backends 2. Rewrite most interfaces of `Blob` to accommodate the change 3. Change the `read` behaviour of `FileReader`, considering the case when blob is file-backed and not cached The design is still immature, welcome comments! - [x] I used `DOMRefCell` to cache the bytes in `BlobImpl`, is it sound? - [x] The interfaces (like `BlobImpl::get_bytes`) handle requests in a default-to-empty way when the inner `DataSlice` is not cached. It might be possible to handle this condition better. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11221) <!-- Reviewable:end -->
⌛ Testing commit 43ad4ba with merge 3d7b176... |
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows |
./mach build -d
does not report any errors./mach test-tidy --faster
does not report any errorsMajor changes
Blob
and aBlobImpl
struct to abstract multiple backendsBlob
to accommodate the changeread
behaviour ofFileReader
, considering the case when blob is file-backed and not cachedThe design is still immature, welcome comments!
Problems to resolve
DOMRefCell
to cache the bytes inBlobImpl
, is it sound?BlobImpl::get_bytes
) handle requests in a default-to-empty way when the innerDataSlice
is not cached. It might be possible to handle this condition better.This change is