client, pager: fix cached item refcounting bugs

4 files changed, 66 insertions, 30 deletions

diff --git a/src/loader/loader.nim b/src/loader/loader.nim
index f0ecb384..31de14c3 100644
--- a/src/loader/loader.nim
+++ b/src/loader/loader.nim
@@ -80,6 +80,7 @@ type
   LoaderCommand = enum
     lcAddCacheFile
     lcAddClient
+    lcGetCacheFile
     lcLoad
     lcLoadConfig
     lcPassFd
@@ -93,13 +94,11 @@ type
 
   ClientKey* = array[32, uint8]
 
-  CachedItemObj = object
+  CachedItem = ref object
     id: int
     path: string
     refc: int
 
-  CachedItem = ref CachedItemObj
-
   ClientData = ref object
     pid: int
     key: ClientKey
@@ -249,21 +248,19 @@ proc redirectToFile(ctx: LoaderContext; output: OutputHandle;
     return false
   return ctx.redirectToStream(output, ps)
 
-type AddCacheFileResult = tuple[outputId: int; cacheFile: string]
-
 proc addCacheFile(ctx: LoaderContext; client: ClientData; output: OutputHandle):
-    AddCacheFileResult =
+    int =
   if output.parent != nil and output.parent.cacheId != -1:
     # may happen e.g. if client tries to cache a `cache:' URL
-    return (output.parent.cacheId, "") #TODO can we get the file name somehow?
+    return output.parent.cacheId
   let tmpf = getTempFile(ctx.config.tmpdir)
   if ctx.redirectToFile(output, tmpf):
     let cacheId = output.outputId
     if output.parent != nil:
       output.parent.cacheId = cacheId
     client.cacheMap.add(CachedItem(id: cacheId, path: tmpf, refc: 1))
-    return (cacheId, tmpf)
-  return (-1, "")
+    return cacheId
+  return -1
 
 proc addFd(ctx: LoaderContext; handle: LoaderHandle) =
   let output = handle.output
@@ -498,6 +495,17 @@ proc loadConfig(ctx: LoaderContext; stream: SocketStream; client: ClientData;
   r.sread(config)
   ctx.load(stream, request, client, config)
 
+proc getCacheFile(ctx: LoaderContext; stream: SocketStream; client: ClientData;
+    r: var BufferedReader) =
+  var cacheId: int
+  r.sread(cacheId)
+  stream.withPacketWriter w:
+    let n = client.cacheMap.find(cacheId)
+    if n != -1:
+      w.swrite(client.cacheMap[n].path)
+    else:
+      w.swrite("")
+
 proc addClient(ctx: LoaderContext; stream: SocketStream;
     r: var BufferedReader) =
   var key: ClientKey
@@ -542,10 +550,9 @@ proc addCacheFile(ctx: LoaderContext; stream: SocketStream;
   let output = ctx.findOutput(outputId, sourceClient)
   assert output != nil
   let targetClient = ctx.clientData[targetPid]
-  let (id, file) = ctx.addCacheFile(targetClient, output)
+  let id = ctx.addCacheFile(targetClient, output)
   stream.withPacketWriter w:
     w.swrite(id)
-    w.swrite(file)
   stream.sclose()
 
 proc redirectToFile(ctx: LoaderContext; stream: SocketStream;
@@ -698,6 +705,9 @@ proc acceptConnection(ctx: LoaderContext) =
       of lcLoadConfig:
         privileged_command
         ctx.loadConfig(stream, client, r)
+      of lcGetCacheFile:
+        privileged_command
+        ctx.getCacheFile(stream, client, r)
       of lcRemoveCachedItem:
         ctx.removeCachedItem(stream, client, r)
       of lcLoad:
@@ -967,10 +977,10 @@ proc tee*(loader: FileLoader; sourceId, targetPid: int): (SocketStream, int) =
 # so that we can be sure that a container only loads images on the page that
 # it owns.
 proc addCacheFile*(loader: FileLoader; outputId, targetPid: int;
-    sourcePid = -1): AddCacheFileResult =
+    sourcePid = -1): int =
   let stream = loader.connect()
   if stream == nil:
-    return (-1, "")
+    return -1
   let sourcePid = if sourcePid == -1: loader.clientPid else: sourcePid
   stream.withLoaderPacketWriter loader, w:
     w.swrite(lcAddCacheFile)
@@ -979,10 +989,20 @@ proc addCacheFile*(loader: FileLoader; outputId, targetPid: int;
     w.swrite(sourcePid)
   var r = stream.initPacketReader()
   var outputId: int
-  var cacheFile: string
   r.sread(outputId)
-  r.sread(cacheFile)
-  return (outputId, cacheFile)
+  return outputId
+
+proc getCacheFile*(loader: FileLoader; cacheId: int): string =
+  let stream = loader.connect()
+  if stream == nil:
+    return ""
+  stream.withLoaderPacketWriter loader, w:
+    w.swrite(lcGetCacheFile)
+    w.swrite(cacheId)
+  var r = stream.initPacketReader()
+  var s: string
+  r.sread(s)
+  return s
 
 proc redirectToFile*(loader: FileLoader; outputId: int; targetPath: string):
     bool =
diff --git a/src/local/client.nim b/src/local/client.nim
index ee469652..c52ea71f 100644
--- a/src/local/client.nim
+++ b/src/local/client.nim
@@ -426,21 +426,25 @@ proc acceptBuffers(client: Client) =
       pager.alert("Error: failed to set up buffer")
       continue
     let key = pager.addLoaderClient(container.process, container.loaderConfig)
+    let loader = pager.loader
     stream.withPacketWriter w:
       w.swrite(key)
-      let loader = pager.loader
       if item.fdin != -1:
         let outputId = item.istreamOutputId
         if container.cacheId == -1:
-          (container.cacheId, container.cacheFile) =
-            loader.addCacheFile(outputId, loader.clientPid)
+          container.cacheId = loader.addCacheFile(outputId, loader.clientPid)
+        if container.request.url.scheme == "cache":
+          # loading from cache; now both the buffer and us hold a new reference
+          # to the cached item, but it's only shared with the buffer. add a
+          # pager ref too.
+          loader.shareCachedItem(container.cacheId, loader.clientPid)
         var outCacheId = container.cacheId
         let pid = container.process
         if item.fdout == item.fdin:
           loader.shareCachedItem(container.cacheId, pid)
           loader.resume(item.istreamOutputId)
         else:
-          outCacheId = loader.addCacheFile(item.ostreamOutputId, pid).outputId
+          outCacheId = loader.addCacheFile(item.ostreamOutputId, pid)
           loader.resume([item.istreamOutputId, item.ostreamOutputId])
         w.swrite(outCacheId)
     if item.fdin != -1:
@@ -450,7 +454,11 @@ proc acceptBuffers(client: Client) =
       discard close(item.fdout)
       container.setStream(stream, registerFun)
     else:
-      # buffer is cloned, no need to cache anything
+      # buffer is cloned, just share the parent's cached source
+      loader.shareCachedItem(container.cacheId, container.process)
+      # also add a reference here; it will be removed when the container is
+      # deleted
+      loader.shareCachedItem(container.cacheId, loader.clientPid)
       container.setCloneStream(stream, registerFun)
     let fd = int(stream.fd)
     client.fdmap[fd] = container
diff --git a/src/local/container.nim b/src/local/container.nim
index 8717610f..7aa5d324 100644
--- a/src/local/container.nim
+++ b/src/local/container.nim
@@ -154,7 +154,6 @@ type
     filter*: BufferFilter
     bgcolor*: CellColor
     tailOnLoad*: bool
-    cacheFile* {.jsget.}: string
     mainConfig*: Config
     flags*: set[ContainerFlag]
     images*: seq[PosBitmap]
@@ -168,7 +167,7 @@ proc newContainer*(config: BufferConfig; loaderConfig: LoaderClientConfig;
     url: URL; request: Request; luctx: LUContext; attrs: WindowAttributes;
     title: string; redirectDepth: int; flags: set[ContainerFlag];
     contentType: Option[string]; charsetStack: seq[Charset]; cacheId: int;
-    cacheFile: string; mainConfig: Config): Container =
+    mainConfig: Config): Container =
   return Container(
     url: url,
     request: request,
@@ -184,7 +183,6 @@ proc newContainer*(config: BufferConfig; loaderConfig: LoaderClientConfig;
     ),
     loadinfo: "Connecting to " & request.url.host & "...",
     cacheId: cacheId,
-    cacheFile: cacheFile,
     process: -1,
     mainConfig: mainConfig,
     flags: flags,
diff --git a/src/local/pager.nim b/src/local/pager.nim
index 8760da86..a70b2b3d 100644
--- a/src/local/pager.nim
+++ b/src/local/pager.nim
@@ -483,7 +483,7 @@ proc draw*(pager: Pager) =
         let bmp = NetworkBitmap(image.bmp)
         let cached = container.findCachedImage(bmp.imageId)
         if cached == nil:
-          let (cacheId, _) = pager.loader.addCacheFile(bmp.outputId,
+          let cacheId = pager.loader.addCacheFile(bmp.outputId,
             pager.loader.clientPid, container.process)
           let request = newRequest(newURL("cache:" & $cacheId).get)
           # capture bmp for the closure
@@ -566,9 +566,13 @@ proc newContainer(pager: Pager; bufferConfig: BufferConfig;
     loaderConfig: LoaderClientConfig; request: Request; title = "";
     redirectDepth = 0; flags = {cfCanReinterpret, cfUserRequested};
     contentType = none(string); charsetStack: seq[Charset] = @[];
-    url = request.url; cacheId = -1; cacheFile = ""): Container =
+    url = request.url): Container =
   let stream = pager.loader.startRequest(request, loaderConfig)
   pager.loader.registerFun(stream.fd)
+  let cacheId = if request.url.scheme == "cache":
+    parseInt32(request.url.pathname).get(-1)
+  else:
+    -1
   let container = newContainer(
     bufferConfig,
     loaderConfig,
@@ -582,7 +586,6 @@ proc newContainer(pager: Pager; bufferConfig: BufferConfig;
     contentType,
     charsetStack,
     cacheId,
-    cacheFile,
     pager.config
   )
   pager.connectingContainers.add(ConnectingContainerItem(
@@ -602,9 +605,7 @@ proc newContainerFrom(pager: Pager; container: Container; contentType: string):
     newRequest(url),
     contentType = some(contentType),
     charsetStack = container.charsetStack,
-    url = container.url,
-    cacheId = container.cacheId,
-    cacheFile = container.cacheFile
+    url = container.url
   )
 
 func findConnectingContainer*(pager: Pager; fd: int): int =
@@ -835,6 +836,7 @@ proc deleteContainer(pager: Pager; container, setTarget: Container) =
     pager.setContainer(setTarget)
   pager.unreg.add(container)
   if container.process != -1:
+    pager.loader.removeCachedItem(container.cacheId)
     pager.forkserver.removeChild(container.process)
     pager.loader.removeClient(container.process)
 
@@ -927,6 +929,14 @@ proc toggleSource(pager: Pager) {.jsfunc.} =
       pager.container.sourcepair = container
       pager.addContainer(container)
 
+proc getCacheFile(pager: Pager; cacheId: int): string {.jsfunc.} =
+  return pager.loader.getCacheFile(cacheId)
+
+proc cacheFile(pager: Pager): string {.jsfget.} =
+  if pager.container != nil:
+    return pager.getCacheFile(pager.container.cacheId)
+  return ""
+
 proc getEditorCommand(pager: Pager; file: string; line = 1): string {.jsfunc.} =
   var editor = pager.config.external.editor
   if (let uqEditor = ChaPath(editor).unquote(); uqEditor.isSome):