Skip to content
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

Implement Blob URLs #10539

Closed
Ms2ger opened this issue Apr 12, 2016 · 18 comments
Closed

Implement Blob URLs #10539

Ms2ger opened this issue Apr 12, 2016 · 18 comments

Comments

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Apr 12, 2016

This is not an easy bug.

This will need integration with the URL parser.

@jdm
Copy link
Member

@jdm jdm commented Apr 12, 2016

Please note, this is part of a GSoC project and anyone who wants to work on this should talk to me before doing anything.

@notriddle notriddle added the A-network label Apr 13, 2016
@bpowers
Copy link

@bpowers bpowers commented Apr 23, 2016

hi @jdm, did this end up as part of an accepted GSoC project?

@jdm
Copy link
Member

@jdm jdm commented Apr 23, 2016

It did!

@izgzhen
Copy link
Contributor

@izgzhen izgzhen commented May 24, 2016

Just about to file an issue and found this one :D. Can someone help me to label it as assigned?

And some other related info:

@izgzhen
Copy link
Contributor

@izgzhen izgzhen commented May 26, 2016

I found a problem regarding the implementation: https://w3c.github.io/FileAPI/#requestResponseModel. Here it mentions the model of "request". However, first it sounds a pretty counterintuitive thing to think about how to handle request in a browser; second, as I observed, in current implementation we will somehow intercept the LoadData message in resource_thread.rs's CoreResourceManager::load based on the url scheme and dispatch to some special loaders like about_loader and file_loader.

I am proposing to handle blob: in a similar way, however, we still don't fix the GET method problem (as specified in the spec). That is to say, our model is more like somehow internalize the "request" (which has headers, method as protocol says) into a "load" one.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented May 26, 2016

somehow intercept the

Well, there's already a switch on a scheme in both http_loader and fetch, we just add an entry to that. It's possible to filter on GET, and I think XHR does this for us anyway.

@izgzhen
Copy link
Contributor

@izgzhen izgzhen commented May 27, 2016

Some experimental code:

https://github.com/izgzhen/servo/tree/blob-loader and https://github.com/izgzhen/servo/tree/url-interface-mock (the latest commit)

Some problems:

  1. Where to put Blob URL Store? Currently I prefer to manage it under CoreResourceManager, and accessible through message interface from script thread.
  2. And the above question will determine how loader will communicate with the Blob Store. Since the loader is called directly in core_resource_thread, so we can directly call it with load requests if we choose the example impl above.
  3. URL origin check (currently I unicode_serialization the origin and do a simple comparison, I thought we have already better checking logics for this somewhere but I have not yet try to find it for now).

And finally the GET method problem, I inspected some dispatch on scheme code (in http_loader etc.), but not yet found anything particular interesting. Will track more in coming days.

@jdm
Copy link
Member

@jdm jdm commented May 27, 2016

I think @Manishearth meant switching on the scheme in resource_thread.rs, not http_loader.rs.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented May 27, 2016

Yeah, basically.

Where to put Blob URL Store? Currently I prefer to manage it under CoreResourceManager,

CRM seems fine for now. FileManager is also okay with me

and accessible through message interface from script thread.

Does script ever need to directly fetch a blob URI? I thought we'd only have to think about HTTP and XHR loads on blob: uris, and switching in resource_thread will fix that (for XHR, you can add a similar switch in fetch, this is already specced)

so we can directly call it with load requests if we choose the example impl above.

yep. might need to be spawned in a temporary thread or loaded in the file manager thread event loop to avoid the resource task getting clogged, since there's an IPC copy involved.

URL origin check (currently I unicode_serialization the origin and do a simple comparison,

Fetch currently checks if o.ascii_serialization() is equal. XHR does the same with "as_str". I'm not sure why a simple PartialEq check on the url::Origin won't work, though.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented May 27, 2016

It depends which of these two you need. https://html.spec.whatwg.org/multipage/browsers.html#same-origin . "same origin-domain" is something you'd have to implement manually.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented May 27, 2016

We should really start using typed origins everywhere

@izgzhen
Copy link
Contributor

@izgzhen izgzhen commented May 27, 2016

I think it would be convenient to implement the same-origin check inside Origin. Also, derive serialize/deserialize for it. (Oddly url doesn't derive it now, and does't use #[derive(Ser...)] feature either)

@Manishearth
Copy link
Member

@Manishearth Manishearth commented May 27, 2016

the same-origin check is already inside Origin 😄

@Manishearth
Copy link
Member

@Manishearth Manishearth commented May 27, 2016

Why are we using strings at all though? You can obtain typed origins from typed URLs, and we have typed URLs.

@izgzhen
Copy link
Contributor

@izgzhen izgzhen commented May 27, 2016

Does script ever need to directly fetch a blob URI?

No, the communication between script thread and store the creation and revocation api.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented May 27, 2016

Well, the file thread will handle creation. We only need a revocation message.

@izgzhen
Copy link
Contributor

@izgzhen izgzhen commented May 27, 2016

Check just by urlA.origin() == urlB.origin()?! It looks more like structural equivalence to me (though it looks like that the spec version "same-origin" is a structural one in a typed, or normalized context).

And for the "creation", it is to create a URL from existing Blob, so it looks like not really related to current file manager thread (although put the store under its management would leverage clogging problem you mentioned).

@Manishearth
Copy link
Member

@Manishearth Manishearth commented May 27, 2016

same-origin is structural 😄 .

Well, the existing blob is probably in the file manager thread anyway. But yeah, that depends on your caching strategy, and which in-memory data goes where. The file manager thread will be creating some blob URIs at any rate.

This was referenced Jun 1, 2016
bors-servo added a commit that referenced this issue Jun 3, 2016
Add Blob URL store

Spec: https://w3c.github.io/FileAPI/#BlobURLStore.

I finally decide to put the store under `ScriptThread` and interpret the "global object" as the script thread itself. The new APIs will be used during the page loading (if scheme is `blob`) and `URL.createObjectURL/revokeObjectURL`.

Related to #11131.

<!-- 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 fix part of #10539

<!-- Either: -->
- [x] These changes do not require tests because it is new stub code which needs further integrating PRs.

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11534)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 3, 2016
Add Blob URL store

Spec: https://w3c.github.io/FileAPI/#BlobURLStore.

I finally decide to put the store under `ScriptThread` and interpret the "global object" as the script thread itself. The new APIs will be used during the page loading (if scheme is `blob`) and `URL.createObjectURL/revokeObjectURL`.

Related to #11131.

<!-- 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 fix part of #10539

<!-- Either: -->
- [x] These changes do not require tests because it is new stub code which needs further integrating PRs.

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11534)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 3, 2016
Add Blob URL store

Spec: https://w3c.github.io/FileAPI/#BlobURLStore.

I finally decide to put the store under `ScriptThread` and interpret the "global object" as the script thread itself. The new APIs will be used during the page loading (if scheme is `blob`) and `URL.createObjectURL/revokeObjectURL`.

Related to #11131.

<!-- 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 fix part of #10539

<!-- Either: -->
- [x] These changes do not require tests because it is new stub code which needs further integrating PRs.

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11534)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 3, 2016
Add Blob URL store

Spec: https://w3c.github.io/FileAPI/#BlobURLStore.

I finally decide to put the store under `ScriptThread` and interpret the "global object" as the script thread itself. The new APIs will be used during the page loading (if scheme is `blob`) and `URL.createObjectURL/revokeObjectURL`.

Related to #11131.

<!-- 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 fix part of #10539

<!-- Either: -->
- [x] These changes do not require tests because it is new stub code which needs further integrating PRs.

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11534)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 8, 2016
Add blob loader

Add a blob loader to implement [Blob URL](https://w3c.github.io/FileAPI/#url). The related interfaces to script thread are also declared.

Progressing in parallel with PR #11534. Related to #11131.

<!-- 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 fix part of #10539

<!-- Either: -->
- [x] These changes do not require tests because not integrated yet.

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11536)
<!-- Reviewable:end -->
@izgzhen izgzhen mentioned this issue Jun 10, 2016
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Jun 11, 2016
Implement Blob URL's DOM interfaces

r? @Manishearth

Implement the two functions in `URL` to create/revoke Blob URLs, and related code to approximate our proposed design to make things work together.

<!-- 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 fix part of #10539, related to #11131

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11716)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 12, 2016
Implement Blob URL's DOM interfaces

r? @Manishearth

Implement the two functions in `URL` to create/revoke Blob URLs, and related code to approximate our proposed design to make things work together.

<!-- 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 fix part of #10539, related to #11131

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11716)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 12, 2016
Implement Blob URL's DOM interfaces

r? @Manishearth

Implement the two functions in `URL` to create/revoke Blob URLs, and related code to approximate our proposed design to make things work together.

<!-- 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 fix part of #10539, related to #11131

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11716)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 14, 2016
Implement Blob URL's DOM interfaces

r? @Manishearth

Implement the two functions in `URL` to create/revoke Blob URLs, and related code to approximate our proposed design to make things work together.

<!-- 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 fix part of #10539, related to #11131

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11716)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 14, 2016
Implement Blob URL's DOM interfaces

r? @Manishearth

Implement the two functions in `URL` to create/revoke Blob URLs, and related code to approximate our proposed design to make things work together.

<!-- 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 fix part of #10539, related to #11131

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11716)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 14, 2016
Implement Blob URL's DOM interfaces

r? @Manishearth

Implement the two functions in `URL` to create/revoke Blob URLs, and related code to approximate our proposed design to make things work together.

<!-- 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 fix part of #10539, related to #11131

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11716)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 17, 2016
Implement Blob URL's DOM interfaces

r? @Manishearth

Implement the two functions in `URL` to create/revoke Blob URLs, and related code to approximate our proposed design to make things work together.

<!-- 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 fix part of #10539, related to #11131

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11716)
<!-- Reviewable:end -->
@izgzhen izgzhen mentioned this issue Jul 15, 2016
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Jul 15, 2016
Put Blob URL online

This PR connects the resource requests with file manager thread, including:

+ `script_thread` load request
+ `image_cache_thread` load request
+ XHR load request (the responding part code yet not implemented due to unfamiliarity with fetch standard, but the infra is here)

One notable change is the introduction of "long-live validity", to handle the case specified in WPT test FileAPI/blob/Blob-XHR-revoke.html.

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 fix #10539
- [x] There are tests for these changes

<!-- 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/12440)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 15, 2016
Put Blob URL online

This PR connects the resource requests with file manager thread, including:

+ `script_thread` load request
+ `image_cache_thread` load request
+ XHR load request (the responding part code yet not implemented due to unfamiliarity with fetch standard, but the infra is here)

One notable change is the introduction of "long-live validity", to handle the case specified in WPT test FileAPI/blob/Blob-XHR-revoke.html.

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 fix #10539
- [x] There are tests for these changes

<!-- 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/12440)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 15, 2016
Put Blob URL online

This PR connects the resource requests with file manager thread, including:

+ `script_thread` load request
+ `image_cache_thread` load request
+ XHR load request (the responding part code yet not implemented due to unfamiliarity with fetch standard, but the infra is here)

One notable change is the introduction of "long-live validity", to handle the case specified in WPT test FileAPI/blob/Blob-XHR-revoke.html.

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 fix #10539
- [x] There are tests for these changes

<!-- 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/12440)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 15, 2016
Put Blob URL online

This PR connects the resource requests with file manager thread, including:

+ `script_thread` load request
+ `image_cache_thread` load request
+ XHR load request (the responding part code yet not implemented due to unfamiliarity with fetch standard, but the infra is here)

One notable change is the introduction of "long-live validity", to handle the case specified in WPT test FileAPI/blob/Blob-XHR-revoke.html.

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 fix #10539
- [x] There are tests for these changes

<!-- 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/12440)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 15, 2016
Put Blob URL online

This PR connects the resource requests with file manager thread, including:

+ `script_thread` load request
+ `image_cache_thread` load request
+ XHR load request (the responding part code yet not implemented due to unfamiliarity with fetch standard, but the infra is here)

One notable change is the introduction of "long-live validity", to handle the case specified in WPT test FileAPI/blob/Blob-XHR-revoke.html.

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 fix #10539
- [x] There are tests for these changes

<!-- 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/12440)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants
You can’t perform that action at this time.