core: Fix random crash on opening a page having images This patch fixes random crash caused by race condition (use after free and unexplainable memory errors such as malloc in glib,) by properly guarding accesses to image download queue. Inside `while` loop reading the downloads queue, it read without lock and that caused reading of freed memory (`d == undefined`), thus operations like `d.data.arc.ref()` and `allocator.destroy(d)` to be invalid. Because the race condition occurs on downloads queue, opening browse page with massive items (200~) has been crashed the app frequently compared to small number of items (~50). Also, longer the image takes to load, more likely the crash to happens. So streaming service's browse page is more prone to crash than local library. With this change, I don't see random crash on Qobuz pages anymore. In addition, for some mysterious reason, images load faster than before. Definetely previous implementation did something wrong, as adding a lock does not make things faster but slower... <https://codeberg.org/pocka/plac/issues/15>

Changes

1 changed files (+23/-10)

• core/src/image.zig
  • @@ -492,7 +492,7 @@
    pub fn Downloader(opts: DownloaderOptions) type { return struct { const Job = struct { - ready: std.Thread.ResetEvent = .{}, + ready: std.Thread.Condition = .{}, result: *GetResult = undefined, req_hash: u64, arc: Arc = .{},
  • @@ -509,6 +509,7 @@ downloads: u32 = ~(@as(u32, std.math.maxInt(u32)) << opts.concurrent_download_limit),
    mutex: std.Thread.Mutex = .{}, cond: std.Thread.Condition = .{}, downloads_queue: std.DoublyLinkedList(Job) = .{}, + queue_mutex: std.Thread.Mutex = .{}, http_client: std.http.Client, is_closed: bool = false, cache: Cache,
  • @@ -567,16 +568,19 @@ return try GetResult.makeCachedRetained(cached);
    } { + self.queue_mutex.lock(); + defer self.queue_mutex.unlock(); + var node = self.downloads_queue.first; while (node) |d| : (node = d.next) { - d.data.arc.ref(); - defer if (d.data.arc.unref()) { - self.downloads_queue.remove(d); - allocator.destroy(d); - }; + if (d.data.req_hash == req_hash) { + d.data.arc.ref(); + defer if (d.data.arc.unref()) { + self.downloads_queue.remove(d); + allocator.destroy(d); + }; - if (d.data.req_hash == req_hash) { - d.data.ready.wait(); + d.data.ready.wait(&self.queue_mutex); return d.data.result.retain(); } }
  • @@ -591,8 +595,17 @@ };
    node.data.arc.ref(); - self.downloads_queue.prepend(node); + { + self.queue_mutex.lock(); + defer self.queue_mutex.unlock(); + + self.downloads_queue.prepend(node); + } + defer if (node.data.arc.unref()) { + self.queue_mutex.lock(); + defer self.queue_mutex.unlock(); + self.downloads_queue.remove(node); allocator.destroy(node); };
  • @@ -673,7 +686,7 @@ };
    } node.data.result = get_result; - node.data.ready.set(); + node.data.ready.broadcast(); return get_result.retain(); }