mirror of
https://github.com/lightpanda-io/browser.git
synced 2026-06-11 01:25:53 -04:00
Fix two arena leaks
The first is with WGS not flushing the deferring layer after its synchronous importScripts call, see: https://github.com/lightpanda-io/browser/pull/2329#discussion_r3271068955 The second is from the ability of an XHR request to be re-used. This was gated on a boolean, but the ordering means that the 1st requests' released gets blocked by the gate, and thus we're always 1 release short. The solution is to use a counter instead of a boolean.
This commit is contained in:
@@ -431,6 +431,38 @@
|
||||
}
|
||||
</script>
|
||||
|
||||
<script id=xhr_reuse type=module>
|
||||
{
|
||||
// Reusing the same XHR object from its own onload handler (open()+send()
|
||||
// again) must not leak: send() acquires a self-ref and each request's
|
||||
// terminal callback releases one. The second send() starts while the
|
||||
// first request's release is still pending (it runs after this load
|
||||
// dispatch), so both refs are briefly live. A regression here is caught
|
||||
// by the test runner's ArenaPool leak check at teardown.
|
||||
const state = await testing.async();
|
||||
|
||||
const req = new XMLHttpRequest();
|
||||
let loads = 0;
|
||||
req.onload = () => {
|
||||
loads += 1;
|
||||
if (loads === 1) {
|
||||
// reuse the same object for a second request
|
||||
req.open('GET', 'http://127.0.0.1:9582/xhr');
|
||||
req.send();
|
||||
} else {
|
||||
state.resolve();
|
||||
}
|
||||
};
|
||||
req.open('GET', 'http://127.0.0.1:9582/xhr');
|
||||
req.send();
|
||||
|
||||
await state.done(() => {
|
||||
testing.expectEqual(2, loads);
|
||||
testing.expectEqual(200, req.status);
|
||||
});
|
||||
}
|
||||
</script>
|
||||
|
||||
<script id=xhr_upload type=module>
|
||||
{
|
||||
// upload is an XMLHttpRequestUpload (an XMLHttpRequestEventTarget) and
|
||||
|
||||
3
src/browser/tests/worker/defer-noop.js
Normal file
3
src/browser/tests/worker/defer-noop.js
Normal file
@@ -0,0 +1,3 @@
|
||||
// Helper for fetch-defer-worker.js: a trivial 200-OK script body that gives
|
||||
// importScripts() a blocking syncRequest to run (its contents don't matter).
|
||||
var deferNoop = 1;
|
||||
20
src/browser/tests/worker/fetch-defer-worker.js
Normal file
20
src/browser/tests/worker/fetch-defer-worker.js
Normal file
@@ -0,0 +1,20 @@
|
||||
// Regression for the worker deferred-fetch leak: a fetch() whose response
|
||||
// arrives while a blocking importScripts() syncRequest is in flight gets
|
||||
// deferred by the DeferringLayer. A worker has no ScriptManager to flush it,
|
||||
// so without WorkerGlobalScope.importScript's flushFrame the deferred fetch
|
||||
// never resolves (this await hangs) and its Response arena leaks on teardown.
|
||||
//
|
||||
// Ordering matters: fetch() and importScripts() run synchronously with no
|
||||
// event-loop pump between them, so the fetch is in-flight (not yet completed)
|
||||
// when importScripts begins pumping ticks — its done lands while the worker's
|
||||
// frame still has a blocking request, which is exactly what triggers deferral.
|
||||
self.onmessage = async function() {
|
||||
const pending = fetch('http://127.0.0.1:9582/xhr');
|
||||
importScripts('defer-noop.js');
|
||||
try {
|
||||
const response = await pending;
|
||||
postMessage({ ok: true, status: response.status });
|
||||
} catch (e) {
|
||||
postMessage({ ok: false, err: String(e) });
|
||||
}
|
||||
};
|
||||
@@ -357,6 +357,24 @@
|
||||
}
|
||||
</script>
|
||||
|
||||
<script id="fetch_deferred_behind_importScripts" type=module>
|
||||
// A worker fetch() whose response arrives during a blocking importScripts()
|
||||
// is deferred by the DeferringLayer; the worker must flush it (it has no
|
||||
// ScriptManager). Without the flush the fetch never resolves and its
|
||||
// Response arena leaks (caught by the runner's ArenaPool leak check).
|
||||
{
|
||||
const state = await testing.async();
|
||||
const worker = new Worker('fetch-defer-worker.js');
|
||||
worker.onmessage = (event) => state.resolve(event.data);
|
||||
worker.postMessage(null);
|
||||
|
||||
await state.done((data) => {
|
||||
testing.expectTrue(data.ok, 'worker fetch failed: ' + data.err);
|
||||
testing.expectEqual(200, data.status);
|
||||
});
|
||||
}
|
||||
</script>
|
||||
|
||||
<script id="worker_post_before_load" type=module>
|
||||
// Per spec, messages posted before the worker has finished loading must
|
||||
// be buffered and delivered once onmessage is registered. Without the
|
||||
|
||||
@@ -476,6 +476,8 @@ fn importScript(self: *WorkerGlobalScope, arena: Allocator, url: [:0]const u8) !
|
||||
return error.NetworkError;
|
||||
}
|
||||
|
||||
defer http_client.deferring_layer.flushFrame(self._frame_id);
|
||||
|
||||
var ls: JS.Local.Scope = undefined;
|
||||
self.js.localScope(&ls);
|
||||
defer ls.deinit();
|
||||
|
||||
@@ -48,7 +48,10 @@ _proto: *XMLHttpRequestEventTarget,
|
||||
_upload: ?*XMLHttpRequestUpload = null,
|
||||
_arena: Allocator,
|
||||
_http_response: ?HttpClient.Response = null,
|
||||
_active_request: bool = false,
|
||||
|
||||
// number of inflight requests, we can have multiple, e.g. xhr calling its own
|
||||
// send from the onload callback
|
||||
_active_requests: u8 = 0,
|
||||
|
||||
_url: [:0]const u8 = "",
|
||||
_method: http.Method = .GET,
|
||||
@@ -124,10 +127,10 @@ pub fn deinit(self: *XMLHttpRequest, page: *Page) void {
|
||||
}
|
||||
|
||||
fn releaseSelfRef(self: *XMLHttpRequest) void {
|
||||
if (self._active_request == false) {
|
||||
if (self._active_requests == 0) {
|
||||
return;
|
||||
}
|
||||
self._active_request = false;
|
||||
self._active_requests -= 1;
|
||||
self.releaseRef(self._exec.page);
|
||||
}
|
||||
|
||||
@@ -255,7 +258,7 @@ pub fn send(self: *XMLHttpRequest, body_: ?BodyInit, exec_: *const Execution) !v
|
||||
}
|
||||
|
||||
self.acquireRef();
|
||||
self._active_request = true;
|
||||
self._active_requests += 1;
|
||||
|
||||
exec.makeRequest(.{
|
||||
.ctx = self,
|
||||
|
||||
Reference in New Issue
Block a user