Repository navigation
Kill SumatraPDF process on preview window close - #7
Conversation
Co-authored-by: emako <24737061+emako@users.noreply.github.com> Agent-Logs-Url: https://github.com/emako/QuickLook.Plugin.SumatraPDFReader/sessions/ef40938c-d836-4eed-a85d-6c4b09be9d00
There was a problem hiding this comment.
Pull request overview
Prevents leaked/background SumatraPDF.exe instances by ensuring the embedded Sumatra process is terminated when the preview control/panel is disposed (e.g., preview window close or preview refresh).
Changes:
- Dispose the hosted
SumatraPDFControlwhenSumatraPanelunloads and before creating a new preview control. - Extend
SumatraPDFControl.Dispose()to kill the backingSumatraPDF.exeprocess (with guarded checks and exception handling).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| QuickLook.Plugin.SumatraPDFReader/SumatraPanel.cs | Persists the hosted control as a field, disposes it on Unloaded, and disposes prior instances on repeated PreviewFile() calls. |
| QuickLook.Plugin.SumatraPDFReader/SumatraPDFControl.Designer.cs | Adds process termination logic to Dispose() so SumatraPDF.exe doesn’t remain running after the preview is closed. |
Files not reviewed (1)
- QuickLook.Plugin.SumatraPDFReader/SumatraPDFControl.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| private void SumatraPanel_Unloaded(object sender, RoutedEventArgs e) | ||
| { |
There was a problem hiding this comment.
SumatraPanel_Unloaded disposes the hosted WinForms control but leaves it assigned to presenter.Child. That can leave WindowsFormsHost holding a reference to a disposed control. Clear/detach the child (e.g., set presenter.Child = null) before/when disposing so the host doesn't reference a disposed instance.
| { | |
| { | |
| presenter.Child = null; |
| _sumatraPDFControl?.Dispose(); | ||
| _sumatraPDFControl = new SumatraPDFControl(); | ||
|
|
There was a problem hiding this comment.
In PreviewFile, the existing control is disposed while it may still be attached as presenter.Child until the end of the method. Detach the previous child from presenter first (or set presenter.Child = null) before disposing it to avoid the host interacting with a disposed WinForms control during layout/handle recreation.
| try | ||
| { | ||
| if (SumatraProcess != null && !SumatraProcess.HasExited) | ||
| { | ||
| SumatraProcess.Kill(); | ||
| } | ||
| } | ||
| catch (InvalidOperationException) | ||
| { | ||
| // Process may have already exited | ||
| } | ||
| catch (System.ComponentModel.Win32Exception) | ||
| { | ||
| // Access denied or process could not be terminated | ||
| } |
There was a problem hiding this comment.
The new SumatraProcess termination runs regardless of the disposing flag. In the finalizer path (disposing == false), accessing managed objects like Process is unsafe; this should be guarded by if (disposing) (or equivalent) so process cleanup only happens on explicit dispose.
| try | |
| { | |
| if (SumatraProcess != null && !SumatraProcess.HasExited) | |
| { | |
| SumatraProcess.Kill(); | |
| } | |
| } | |
| catch (InvalidOperationException) | |
| { | |
| // Process may have already exited | |
| } | |
| catch (System.ComponentModel.Win32Exception) | |
| { | |
| // Access denied or process could not be terminated | |
| } | |
| if (disposing) | |
| { | |
| try | |
| { | |
| if (SumatraProcess != null && !SumatraProcess.HasExited) | |
| { | |
| SumatraProcess.Kill(); | |
| } | |
| } | |
| catch (InvalidOperationException) | |
| { | |
| // Process may have already exited | |
| } | |
| catch (System.ComponentModel.Win32Exception) | |
| { | |
| // Access denied or process could not be terminated | |
| } | |
| } |
| catch (System.ComponentModel.Win32Exception) | ||
| { | ||
| // Access denied or process could not be terminated | ||
| } |
There was a problem hiding this comment.
After calling Kill(), the Process object itself is still kept alive and its OS handle isn’t released until GC. Consider disposing SumatraProcess (and setting the field to null) in Dispose() (ideally in a finally) to avoid leaking a process handle across repeated previews.
| } | |
| } | |
| finally | |
| { | |
| if (SumatraProcess != null) | |
| { | |
| SumatraProcess.Dispose(); | |
| SumatraProcess = null; | |
| } | |
| } |
Each preview open spawned a new
SumatraPDF.exethat was never terminated —Dispose()only sentWM_DESTROYto the window but never killed the backing process.Changes
SumatraPDFControl.Designer.cs— AddSumatraProcess.Kill()toDispose(), guarded byHasExitedcheck with specific exception handling (InvalidOperationException,Win32Exception):SumatraPanel.cs— StoreSumatraPDFControlas a field and wire upUnloadedto callDispose()when the panel leaves the visual tree (window close / preview change). Also dispose any existing instance at the top ofPreviewFile()to prevent leaks on re-entrant calls.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
www.oasis-open.org/opt/hostedtoolcache/CodeQL/2.24.3/x64/codeql/tools/linux64/java/bin/java /opt/hostedtoolcache/CodeQL/2.24.3/x64/codeql/tools/linux64/java/bin/java -jar /opt/hostedtoolcache/CodeQL/2.24.3/x64/codeql/xml/tools/xml-extractor.jar --fileList=/tmp/codeql-scratch-b692a6f0cd709e3b/dbs/csharp/working/files-to-index16873092065553289612.list --sourceArchiveDir=/tmp/codeql-scratch-b692a6f0cd709e3b/dbs/csharp/src --outputDir=/tmp/codeql-scratch-b692a6f0cd709e3b/dbs/csharp/trap/csharp(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
⌨️ Start Copilot coding agent tasks without leaving your editor — available in VS Code, Visual Studio, JetBrains IDEs and Eclipse.