fix: Base64QRCode chama base SetQRCodeData e dispoem o renderizador interno#90
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Qodana Community for .NETIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2026.1.3
with:
upload-result: trueContact Qodana teamContact us at qodana-support@jetbrains.com
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| protected override void Dispose(bool disposing) | ||
| { | ||
| if (disposing) | ||
| { | ||
| this.qr?.Dispose(); | ||
| } | ||
|
|
||
| base.Dispose(disposing); | ||
| } |
There was a problem hiding this comment.
🟡 Shared QR code data is disposed twice during cleanup, once by the inner renderer and again by the base class
The shared data object is disposed by the inner renderer (this.qr?.Dispose() at QRCoder.Core/Renderers/Base64QRCode.cs:104) and then disposed again by the base class call, so the same object undergoes disposal twice per cleanup cycle.
Impact: Currently benign because the data's dispose only nulls fields, but will cause errors if that disposal ever performs non-idempotent cleanup.
Double-dispose mechanism through shared QRCodeData reference
When Base64QRCode is constructed via Base64QRCode(QRCodeData data) at line 81-84, both this.QrCodeData (set by base(data)) and this.qr.QrCodeData (set by new QRCode(data)) point to the same QRCodeData instance.
Similarly, when SetQRCodeData(data) is called (lines 90-94), both base.SetQRCodeData(data) and this.qr.SetQRCodeData(data) set the same reference.
During Dispose(true) (lines 100-108):
this.qr?.Dispose()callsAbstractQRCode.Dispose(true)on the innerQRCode, which callsqr.QrCodeData?.Dispose()and setsqr.QrCodeData = null(QRCoder.Core/Abstractions/AbstractQRCode.cs:63-65)base.Dispose(disposing)then callsthis.QrCodeData?.Dispose()on the SAME object (sincethis.QrCodeDatastill holds the reference) and setsthis.QrCodeData = null
QRCodeData.Dispose() (QRCoder.Core/Models/QRCodeData.cs:214-221) has no _disposed guard, so its body executes twice.
Fix: Either don't call base.Dispose(disposing) (but then this.QrCodeData won't be nulled), or null out this.QrCodeData without disposing it (since qr already disposed it), or add a disposed guard.
Prompt for agents
In Base64QRCode.Dispose(bool disposing), the inner `this.qr` and the base class both hold a reference to the same QRCodeData instance. When qr.Dispose() is called, it disposes the shared QRCodeData. Then base.Dispose(disposing) disposes it again.
To fix this, you should avoid the double-dispose. Options:
1. Set `this.QrCodeData = null` before calling `base.Dispose(disposing)` so the base class skips the dispose (since it checks `?.Dispose()`). But this accesses a protected field directly.
2. Override Dispose to only dispose qr and then null out QrCodeData without calling its Dispose again. For example:
- Dispose qr (which disposes the shared QRCodeData)
- Set this.QrCodeData = null (to prevent base from disposing it again)
- Call base.Dispose(disposing) (which will see QrCodeData is null and skip it)
3. Add a _disposed flag to prevent re-entrance.
Option 2 is simplest: insert `this.QrCodeData = null;` between `this.qr?.Dispose();` and `base.Dispose(disposing);` in the Dispose override at QRCoder.Core/Renderers/Base64QRCode.cs:100-108.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Corrigido. O Dispose agora nulifica this.QrCodeData depois de dispor o qr interno, evitando o duplo Dispose no mesmo objeto. O base.Dispose segue chamado para manter a cadeia de descarte e garantir o idempotence do padrão.
| base.SetQRCodeData(data); | ||
| this.qr.SetQRCodeData(data); |
There was a problem hiding this comment.
📝 Info: The SetQRCodeData fix corrects a pre-existing bug where base class data was never set
Before this PR, SetQRCodeData in Base64QRCode only called this.qr.SetQRCodeData(data) without calling base.SetQRCodeData(data). This meant that this.QrCodeData (the inherited property from AbstractQRCode) remained null when using the parameterless constructor + SetQRCodeData flow. While this didn't cause runtime issues because Base64QRCode delegates all rendering to the inner qr field (which did have the data), it violated the contract of AbstractQRCode and could confuse any code that inspects QrCodeData on the base class. The addition of base.SetQRCodeData(data) at QRCoder.Core/Renderers/Base64QRCode.cs:92 is a correct fix.
Was this helpful? React with 👍 or 👎 to provide feedback.
| protected override void Dispose(bool disposing) | ||
| { | ||
| if (disposing) | ||
| { | ||
| this.qr?.Dispose(); | ||
| } | ||
|
|
||
| base.Dispose(disposing); | ||
| } |
There was a problem hiding this comment.
📝 Info: The Dispose override fixes a resource leak where the inner QRCode was never cleaned up
Before this PR, Base64QRCode had no Dispose(bool) override, so when disposed, only base.Dispose(true) ran — which disposed this.QrCodeData but never disposed the inner this.qr field. The QRCode instance (which is itself an AbstractQRCode with its own QrCodeData reference) was leaked. The new override at QRCoder.Core/Renderers/Base64QRCode.cs:100-108 correctly addresses this leak, though it introduces a double-dispose of the shared QRCodeData (reported separately as a bug).
Was this helpful? React with 👍 or 👎 to provide feedback.
…nterno Resolve comentários do Devin Review: o Base64QRCode delegava a definição de dados apenas para o QRCode interno, sem atualizar o QrCodeData herdado, e não dispoem o renderizador interno no Dispose. Co-Authored-By: Afonso Dutra Nogueira Filho <afonsoft@gmail.com>
356c3e1 to
92b4766
Compare
|
1 participant
All Submissions:
New Feature Submissions:
Changes to Core Features:
What is being fixed?
After merging the SonarCloud cleanup consolidation PR (#83), the Devin Review comments pointed out a remaining pre-existing issue in
Base64QRCode:Base64QRCode.SetQRCodeDataforwarded the call only to the innerQRCoderenderer, without updating the inheritedQrCodeDataproperty. This leftBase64QRCodeitself with a stalenulldata reference, making the inheritedDisposeunable to clean up the actual data.Base64QRCodedid not dispose its innerQRCodeinstance, which holds aQRCodeDatareference and implementsIDisposable.Changes
Base64QRCode.SetQRCodeData(QRCodeData)now callsbase.SetQRCodeData(data)before forwarding to the inner renderer.Base64QRCode.Dispose(bool)to dispose the innerQRCodeand then call the base implementation.Base64QRCodedisposal and theSetQRCodeData+GetGraphic+ dispose path.Verification
dotnet build QRCoder.Core.sln --configuration Release— 0 warnings, 0 errors.dotnet test QRCoder.Core.Tests/ -f net10.0 --configuration Release— 504 passed, 0 failed.Link to Devin session: https://app.devin.ai/sessions/20c92a1bad724f63b8be89a9e195e664
Requested by: @afonsoft