From 1563088f060b1f1d4826bb3a6d4120b276a4e15f Mon Sep 17 00:00:00 2001 From: Andreu Botella Date: Wed, 8 Sep 2021 11:29:21 +0200 Subject: [PATCH] fix: a `Request` whose URL is a revoked blob URL should still fetch (#11947) In the spec, a URL record has an associated "blob URL entry", which for `blob:` URLs is populated during parsing to contain a reference to the `Blob` object that backs that object URL. It is this blob URL entry that the `fetch` API uses to resolve an object URL. Therefore, since the `Request` constructor parses URL inputs, it will have an associated blob URL entry which will be used when fetching, even if the object URL has been revoked since the construction of the `Request` object. (The `Request` constructor takes the URL as a string and parses it, so the object URL must be live at the time it is called.) This PR adds a new `blobFromObjectUrl` JS function (backed by a new `op_blob_from_object_url` op) that, if the URL is a valid object URL, returns a new `Blob` object whose parts are references to the same Rust `BlobPart`s used by the original `Blob` object. It uses this function to add a new `blobUrlEntry` field to inner requests, which will be `null` or such a `Blob`, and then uses `Blob.prototype.stream()` as the response's body. As a result of this, the `blob:` URL resolution from `op_fetch` is now useless, and has been removed. --- ext/fetch/23_request.js | 8 +++++++ ext/fetch/26_fetch.js | 28 +++++++++++++++++++++++ ext/fetch/lib.rs | 47 +++----------------------------------- ext/web/09_file.js | 44 ++++++++++++++++++++++++++++++++--- ext/web/blob.rs | 45 +++++++++++++++++++++++++++++++++++- ext/web/internal.d.ts | 1 + ext/web/lib.rs | 2 ++ tools/wpt/expectation.json | 4 +--- 8 files changed, 128 insertions(+), 51 deletions(-) diff --git a/ext/fetch/23_request.js b/ext/fetch/23_request.js index 1372125c11..36c63db2ab 100644 --- a/ext/fetch/23_request.js +++ b/ext/fetch/23_request.js @@ -19,6 +19,7 @@ const { mixinBody, extractBody } = window.__bootstrap.fetchBody; const { getLocationHref } = window.__bootstrap.location; const mimesniff = window.__bootstrap.mimesniff; + const { blobFromObjectUrl } = window.__bootstrap.file; const { headersFromHeaderList, headerListFromHeaders, @@ -59,6 +60,7 @@ * @property {number} redirectCount * @property {string[]} urlList * @property {number | null} clientRid NOTE: non standard extension for `Deno.HttpClient`. + * @property {Blob | null} blobUrlEntry */ const defaultInnerRequest = { @@ -81,11 +83,16 @@ * @returns */ function newInnerRequest(method, url, headerList = [], body = null) { + let blobUrlEntry = null; + if (url.startsWith("blob:")) { + blobUrlEntry = blobFromObjectUrl(url); + } return { method: method, headerList, body, urlList: [url], + blobUrlEntry, ...defaultInnerRequest, }; } @@ -118,6 +125,7 @@ redirectCount: request.redirectCount, urlList: request.urlList, clientRid: request.clientRid, + blobUrlEntry: request.blobUrlEntry, }; } diff --git a/ext/fetch/26_fetch.js b/ext/fetch/26_fetch.js index edd14abf18..ada524fcbc 100644 --- a/ext/fetch/26_fetch.js +++ b/ext/fetch/26_fetch.js @@ -35,6 +35,7 @@ Promise, PromisePrototypeThen, PromisePrototypeCatch, + String, StringPrototypeToLowerCase, TypedArrayPrototypeSubarray, TypeError, @@ -172,6 +173,33 @@ * @returns {Promise} */ async function mainFetch(req, recursive, terminator) { + if (req.blobUrlEntry !== null) { + if (req.method !== "GET") { + throw new TypeError("Blob URL fetch only supports GET method."); + } + + const body = new InnerBody(req.blobUrlEntry.stream()); + terminator[abortSignal.add](() => + body.error(new DOMException("Ongoing fetch was aborted.", "AbortError")) + ); + + return { + headerList: [ + ["content-length", String(req.blobUrlEntry.size)], + ["content-type", req.blobUrlEntry.type], + ], + status: 200, + statusMessage: "OK", + body, + type: "basic", + url() { + if (this.urlList.length == 0) return null; + return this.urlList[this.urlList.length - 1]; + }, + urlList: recursive ? [] : [...req.urlList], + }; + } + /** @type {ReadableStream | Uint8Array | null} */ let reqBody = null; diff --git a/ext/fetch/lib.rs b/ext/fetch/lib.rs index c419180c5a..70ed403586 100644 --- a/ext/fetch/lib.rs +++ b/ext/fetch/lib.rs @@ -26,7 +26,6 @@ use deno_core::ZeroCopyBuf; use deno_tls::create_http_client; use deno_tls::rustls::RootCertStore; use deno_tls::Proxy; -use deno_web::BlobStore; use http::header::CONTENT_LENGTH; use reqwest::header::HeaderName; use reqwest::header::HeaderValue; @@ -275,49 +274,9 @@ where (request_rid, None, None) } "blob" => { - let blob_store = state.try_borrow::().ok_or_else(|| { - type_error("Blob URLs are not supported in this context.") - })?; - - let blob = blob_store - .get_object_url(url)? - .ok_or_else(|| type_error("Blob for the given URL not found."))?; - - if method != "GET" { - return Err(type_error("Blob URL fetch only supports GET method.")); - } - - let cancel_handle = CancelHandle::new_rc(); - let cancel_handle_ = cancel_handle.clone(); - - let fut = async move { - // TODO(lucacsonato): this should be a stream! - let chunk = match blob.read_all().or_cancel(cancel_handle_).await? { - Ok(chunk) => chunk, - Err(err) => return Ok(Err(err)), - }; - - let res = http::Response::builder() - .status(http::StatusCode::OK) - .header(http::header::CONTENT_LENGTH, chunk.len()) - .header(http::header::CONTENT_TYPE, blob.media_type.clone()) - .body(reqwest::Body::from(chunk)) - .map_err(|err| type_error(err.to_string())); - - match res { - Ok(response) => Ok(Ok(Response::from(response))), - Err(err) => Ok(Err(err)), - } - }; - - let request_rid = state - .resource_table - .add(FetchRequestResource(Box::pin(fut))); - - let cancel_handle_rid = - state.resource_table.add(FetchCancelHandle(cancel_handle)); - - (request_rid, None, Some(cancel_handle_rid)) + // Blob URL resolution happens in the JS side of fetch. If we got here is + // because the URL isn't an object URL. + return Err(type_error("Blob for the given URL not found.")); } _ => return Err(type_error(format!("scheme '{}' not supported", scheme))), }; diff --git a/ext/web/09_file.js b/ext/web/09_file.js index 43d9ffcb30..64cdb7b8ae 100644 --- a/ext/web/09_file.js +++ b/ext/web/09_file.js @@ -167,11 +167,12 @@ return bag; } + const _type = Symbol("Type"); const _size = Symbol("Size"); const _parts = Symbol("Parts"); class Blob { - #type = ""; + [_type] = ""; [_size] = 0; [_parts]; @@ -199,7 +200,7 @@ this[_parts] = parts; this[_size] = size; - this.#type = normalizeType(options.type); + this[_type] = normalizeType(options.type); } /** @returns {number} */ @@ -211,7 +212,7 @@ /** @returns {string} */ get type() { webidl.assertBranded(this, Blob); - return this.#type; + return this[_type]; } /** @@ -561,7 +562,44 @@ } } + /** + * Construct a new Blob object from an object URL. + * + * This new object will not duplicate data in memory with the original Blob + * object from which this URL was created or with other Blob objects created + * from the same URL, but they will be different objects. + * + * The object returned from this function will not be a File object, even if + * the original object from which the object URL was constructed was one. This + * means that the `name` and `lastModified` properties are lost. + * + * @param {string} url + * @returns {Blob | null} + */ + function blobFromObjectUrl(url) { + const blobData = core.opSync("op_blob_from_object_url", url); + if (blobData === null) { + return null; + } + + /** @type {BlobReference[]} */ + const parts = []; + let totalSize = 0; + + for (const { uuid, size } of blobData.parts) { + ArrayPrototypePush(parts, new BlobReference(uuid, size)); + totalSize += size; + } + + const blob = webidl.createBranded(Blob); + blob[_type] = blobData.media_type; + blob[_size] = totalSize; + blob[_parts] = parts; + return blob; + } + window.__bootstrap.file = { + blobFromObjectUrl, getParts, Blob, File, diff --git a/ext/web/blob.rs b/ext/web/blob.rs index 2179de4a69..37e93c8533 100644 --- a/ext/web/blob.rs +++ b/ext/web/blob.rs @@ -3,7 +3,7 @@ use deno_core::error::type_error; use deno_core::parking_lot::Mutex; use deno_core::url::Url; use deno_core::ZeroCopyBuf; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use std::cell::RefCell; use std::collections::HashMap; use std::fmt::Debug; @@ -260,3 +260,46 @@ pub fn op_blob_revoke_object_url( blob_store.remove_object_url(&url); Ok(()) } + +#[derive(Serialize)] +pub struct ReturnBlob { + pub media_type: String, + pub parts: Vec, +} + +#[derive(Serialize)] +pub struct ReturnBlobPart { + pub uuid: Uuid, + pub size: usize, +} + +pub fn op_blob_from_object_url( + state: &mut deno_core::OpState, + url: String, + _: (), +) -> Result, AnyError> { + let url = Url::parse(&url)?; + if url.scheme() != "blob" { + return Ok(None); + } + + let blob_store = state.try_borrow::().ok_or_else(|| { + type_error("Blob URLs are not supported in this context.") + })?; + if let Some(blob) = blob_store.get_object_url(url)? { + let parts = blob + .parts + .iter() + .map(|part| ReturnBlobPart { + uuid: blob_store.insert_part(part.clone()), + size: part.size(), + }) + .collect(); + Ok(Some(ReturnBlob { + media_type: blob.media_type.clone(), + parts, + })) + } else { + Ok(None) + } +} diff --git a/ext/web/internal.d.ts b/ext/web/internal.d.ts index 6e72c1a80c..94fc9c1968 100644 --- a/ext/web/internal.d.ts +++ b/ext/web/internal.d.ts @@ -73,6 +73,7 @@ declare namespace globalThis { }; declare var file: { + blobFromObjectUrl(url: string): Blob | null; getParts(blob: Blob): string[]; Blob: typeof Blob; File: typeof File; diff --git a/ext/web/lib.rs b/ext/web/lib.rs index 87ae46f2f7..5f8ab1625b 100644 --- a/ext/web/lib.rs +++ b/ext/web/lib.rs @@ -29,6 +29,7 @@ use std::usize; use crate::blob::op_blob_create_object_url; use crate::blob::op_blob_create_part; +use crate::blob::op_blob_from_object_url; use crate::blob::op_blob_read_part; use crate::blob::op_blob_remove_part; use crate::blob::op_blob_revoke_object_url; @@ -88,6 +89,7 @@ pub fn init(blob_store: BlobStore, maybe_location: Option) -> Extension { "op_blob_revoke_object_url", op_sync(op_blob_revoke_object_url), ), + ("op_blob_from_object_url", op_sync(op_blob_from_object_url)), ( "op_message_port_create_entangled", op_sync(op_message_port_create_entangled), diff --git a/tools/wpt/expectation.json b/tools/wpt/expectation.json index 1c0ef521a2..a8683e1bba 100644 --- a/tools/wpt/expectation.json +++ b/tools/wpt/expectation.json @@ -13998,9 +13998,7 @@ "fileReader.any.html": true, "url": { "url-format.any.html": true, - "url-with-fetch.any.html": [ - "Revoke blob URL after creating Request, will fetch" - ] + "url-with-fetch.any.html": true }, "reading-data-section": { "Determining-Encoding.any.html": true,