Add pluggable URL scheme resolver (e.g. blob:) to UrlRequest#37
Add pluggable URL scheme resolver (e.g. blob:) to UrlRequest#37bkaradzic-microsoft wants to merge 6 commits into
Conversation
Introduce UrlRequest::RegisterSchemeResolver so a consumer can register a process-global resolver for a non-transport URL scheme such as "blob:". When a UrlRequest is opened with a URL whose scheme has a registered resolver, the platform transport is bypassed and the resolver supplies the response at SendAsync() time. Resolution is deferred to SendAsync (rather than Open) so a blob: URL revoked between open() and send() is honored, and an unhandled URL (e.g. revoked) surfaces as a status-0 transport-style error. This lets every consumer (fetch, XMLHttpRequest, image/video src, texture loaders, ...) resolve such URLs uniformly through UrlRequest instead of each carrying its own branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033
There was a problem hiding this comment.
Pull request overview
This PR adds a process-global, pluggable URL scheme resolver mechanism to UrlRequest, allowing non-transport schemes (e.g. blob:) to be resolved via a registered callback and served through the existing UrlRequest consumer surface (fetch/XHR/resource loaders) without per-consumer special-casing.
Changes:
- Introduces
UrlSchemeResolverResultandUrlSchemeResolver, plus the publicUrlRequest::RegisterSchemeResolver()API for process-global registration. - Diverts
UrlRequest::Open()when a registered scheme is detected, and performs resolver execution duringSendAsync()to honor revoke-after-open scenarios. - Adds shared-layer storage for resolver state and exposes resolved buffers via
ResponseBuffer()for scheme-resolved requests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Source/UrlRequest_Shared.h | Hooks scheme diversion into Open(), resolves via resolver path in SendAsync(), and routes ResponseBuffer() for resolved responses. |
| Source/UrlRequest_Base.h | Implements the resolver registry, scheme detection, and shared-layer scheme resolution response population. |
| Include/UrlLib/UrlLib.h | Adds the public resolver API surface (UrlSchemeResolverResult, UrlSchemeResolver, RegisterSchemeResolver). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Consume the pending resolver on the first ResolveScheme() call (move it out and clear it before invoking) so a second SendAsync() on the same request does not re-run the resolver -- avoiding repeated resolver side effects or clobbering the already-populated response. IsSchemeResolution() stays true so ResponseBuffer() keeps serving the resolved bytes. Add a UrlLibTests suite (SchemeResolver.cpp) covering: a handled resolver populating a String and a Buffer response (status, statusText, content-type, body, response URL); the handled == false transport-style error contract (status None, ErrorSymbol "SchemeResolverNotFound", empty buffer); and resolve-exactly-once across repeated sends. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033
A resolver that throws no longer escapes SendAsync() synchronously: the call is guarded and the exception is mapped to the same transport-style error surface as handled == false, reported as "SchemeResolverThrew" with the exception message as detail. Removal is now the explicit UnregisterSchemeResolver(scheme) rather than registering an empty std::function. RegisterSchemeResolver throws std::invalid_argument for a null resolver or empty scheme, so a moved-from or default-constructed resolver can no longer silently unregister a scheme whose captured state (e.g. a blob store) is still in use. Also adds a regression test showing the resolver path already inherits the canonical reason-phrase fallback in StatusText() when a resolver supplies a status code but no status text. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033
The test sent a request through the real platform transport after unregistering the resolver, but used the resolver-path helper that asserts the send settles inline. On Apple backends the transport completes asynchronously, so the assert failed in CI (macOS_Xcode264). Assert the property actually under test instead: after unregistering, the resolver is never consulted again. The transport's outcome for an unknown scheme is platform-specific (throw at Open, fail inline, or fail async), so each of those is tolerated and an in-flight request is aborted. The completion flag is now held by shared_ptr rather than captured by reference, so a continuation that runs later cannot touch a dangling stack slot. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033
Explicitly aborting the (non-diverted) request perturbed the shared NSURLSession on the Apple backend, so the following test in the suite failed with a spurious NSURLErrorCancelled (-999). Let the request wind down through its own destructor instead, which the earlier CI run showed leaves subsequent tests unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033
| // Not diverted, so this now goes to the real transport. Whether it settles inline or on a | ||
| // worker thread, it must not produce the resolver's response. An in-flight request is left | ||
| // to wind down through the request's own destructor rather than an explicit Abort(): on the | ||
| // NSURLSession backend an explicit abort perturbs the shared session and surfaces as a | ||
| // spurious NSURLErrorCancelled (-999) in subsequent tests. | ||
| if (SendCompletesSynchronously(afterUnregister)) |
There was a problem hiding this comment.
[Reviewed by Copilot on behalf of @bghgary]
This leaves a transport request in flight across the destruction of afterUnregister, and the stated rationale doesn't hold: ~ImplBase() calls the same non-virtual Abort() as the explicit call did, so removing the explicit one doesn't change what happens to the request.
On the Apple backend it changes nothing at all. UrlRequest::Impl there has no destructor, never reads m_cancellationSource, and never calls [task cancel], so neither path stops a resumed NSURLSessionDataTask. Since nothing cancels the task, an explicit Abort() also can't be the source of the NSURLErrorCancelled (-999) attributed to it here.
The hazard is lifetime, not cancellation. SendAsync()'s completion block touches m_statusCode, m_headers, m_responseBuffer and SetError(...), so it implicitly captures a raw this, and nothing extends the Impl's lifetime (no enable_shared_from_this, no captured shared_ptr). "urllibtest-unregister:anything" yields a valid NSURL with a scheme, so the task really is resumed and NSURLSession fails it asynchronously after this scope exits, at which point the handler writes into a freed Impl. That is a likelier source of the instability the last two commits chased than an explicit abort, and it also explains why SendCompletesSynchronously() returns false here.
The lifetime gap is pre-existing rather than something this PR introduced, but this test is the first code to exercise it. Waiting for the request to settle before leaving scope, rather than abandoning it, would avoid depending on that timing. The Apple backend gap looks worth its own issue.
There was a problem hiding this comment.
You are right on every point, and my rationale in the previous commit was wrong. I verified each claim against the source:
~ImplBase()calls the same non-virtualAbort(), so removing the explicit call changed nothing about cancellation.Abort()only doesm_cancellationSource.cancel(), and the AppleImplhas no destructor, never readsm_cancellationSource, and never calls[task cancel]— so neither path stops a resumedNSURLSessionDataTask, and an explicit abort could not have been the source of the -999.- The completion handler in
UrlRequest_Apple.mmwritesm_statusCode,m_headers,m_responseString/m_responseBufferand callsSetError(...)through a rawthis, with nothing extending the impl's lifetime.
Your lifetime diagnosis also explains the failure far better than mine did. The -999 surfaced in a different, previously-passing test (SuccessfulLocalFileReportsNoError) as a non-empty ErrorString/ErrorSymbol with code -999 — exactly what you would expect when the abandoned task's late failure handler writes into the freed allocation that the next test's Impl was reused for. That is a use-after-free, not session cancellation, and it also explains why SendCompletesSynchronously() returned false there.
Fixed in 6d45aeb: the test now blocks until the transport request settles (SendAndWait, 60s cap) before leaving scope, and calls ADD_FAILURE() rather than silently continuing if it does not settle — so it never abandons an in-flight request or depends on that timing. The misleading comment about aborting perturbing a shared session is gone.
Agreed the backend gap deserves its own issue, and that it is pre-existing rather than introduced here. Filed as BabylonJS/JsRuntimeHost#214 (UrlLib has Issues disabled), covering the missing enable_shared_from_this/strong capture and the ignored m_cancellationSource, with a note to audit the other backends for the same pattern.
The previous rationale was wrong: ~ImplBase() calls the same non-virtual Abort() as the explicit call, so dropping the explicit Abort() changed nothing about cancellation, and on the Apple backend neither path stops a resumed NSURLSessionDataTask -- so an explicit abort could not have produced the NSURLErrorCancelled (-999) attributed to it. The real hazard is lifetime, not cancellation. UrlRequest_Apple.mm's completion handler writes m_statusCode / m_headers / m_responseBuffer and calls SetError(...) through a raw `this`, and nothing extends the Impl's lifetime. Abandoning the in-flight request let that handler write into freed memory, which is what corrupted a later test's request state. Wait for the request to settle before leaving scope, and fail explicitly if it does not, so the test never depends on that timing. The underlying backend gap is pre-existing and tracked separately in BabylonJS/JsRuntimeHost#214. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033
What
Adds a pluggable URL scheme resolver hook to
UrlRequest, so a consumer can register a resolver for a non-transport URL scheme (e.g.blob:) and have everyUrlRequest-based consumer (fetch, XMLHttpRequest, image/videosrc, texture loaders, ...) resolve such URLs uniformly through the transport layer instead of each carrying its own branch.How
UrlRequest::RegisterSchemeResolver(std::string scheme, UrlSchemeResolver resolver), plus theUrlSchemeResolverResultstruct andUrlSchemeResolveralias.UrlRequestis opened with a URL whose (lower-cased) scheme has a registered resolver, the platform transport is bypassed. Resolution is deferred toSendAsync()(rather thanOpen()) so a URL revoked betweenopen()andsend()is honored.handled == false(e.g. a revokedblob:URL) leaves the status at0/Noneand records a transport-style error, mirroring how a genuine network failure surfaces.ImplBase+ the shared wrapper, so every platform backend (Win32/Unix/Apple/Android) inherits it with no per-platform changes.Testing
Builds clean with the Win32 backend (
UrlLibandUrlLibTeststargets). Downstream, this powersURL.createObjectURLblob:support in JsRuntimeHost — see BabylonJS/JsRuntimeHost#207 (tracked by BabylonJS/JsRuntimeHost#213), where all 216 unit tests pass with fetch/XMLHttpRequest resolvingblob:through this hook.