THausherr commented on PR #440:
URL: https://github.com/apache/pdfbox/pull/440#issuecomment-4228130373
This disposes the wrong object if the exception is too early. I asked
copilot and here's the important parts (AI is too verbose):
====
At the end of the try-block, graphics2D will usually still refer to the
image graphics (good)… unless an exception happens before graphics2D =
image.createGraphics() is executed, in which case graphics2D still refers to
the printer graphics and the finally would dispose a graphics object it
probably does not own.
Change the code to dispose only the image graphics created by
image.createGraphics():
introduce Graphics2D imageGraphics = null;
set it right after creation
finally { if (imageGraphics != null) imageGraphics.dispose(); }
=====
Here's the full review:
=====
PR #440 is conceptually aiming at the right thing (avoid leaking/holding
onto graphics resources when print() exits via an exception), but the
implementation as shown in the patch has a bug and likely doesn’t do what the
author intended.
What the PR changes
It hoists Graphics2D graphics2D and Graphics2D printerGraphics out of the
try block and adds a finally intended to dispose graphics if something goes
wrong.
Patch excerpt (from the PR “files” view):
Diff
+ Graphics2D printerGraphics = null;
+ Graphics2D graphics2D = null;
...
- Graphics2D graphics2D = (Graphics2D)graphics;
+ graphics2D = (Graphics2D)graphics;
...
- Graphics2D printerGraphics = null;
...
- graphics2D.dispose();
...
+ finally
+ {
+ if (printerGraphics != null)
+ {
+ graphics2D.dispose();
+ }
+ }
Main issue: finally disposes the wrong object
In the rasterization branch (existing code), the flow is:
printerGraphics = graphics2D; (this is the original printer-provided
graphics)
graphics2D = image.createGraphics(); (this is the BufferedImage graphics
that must be disposed)
So if rasterization is enabled, the thing you must dispose is the image
graphics, i.e. the graphics2D after reassignment.
But in the PR’s finally, the condition is if (printerGraphics != null)
(meaning rasterization happened), and then it calls graphics2D.dispose().
At the end of the try-block, graphics2D will usually still refer to the
image graphics (good)… unless an exception happens before graphics2D =
image.createGraphics() is executed, in which case graphics2D still refers to
the printer graphics and the finally would dispose a graphics object it
probably does not own.
Also: the PR removed the existing graphics2D.dispose() in the normal
(non-exception) rasterization flow, but the finally only disposes when
printerGraphics != null; so disposal would still happen on success. That part
is fine if it’s always the correct graphics instance—but as explained, it’s not
guaranteed.
Secondary issue: likely typo / intent mismatch
If the PR title is “release graphics if an exception will be thrown…”, the
safe pattern is usually:
Never dispose the incoming Graphics/Graphics2D graphics argument (caller
owns it)
Dispose only graphics objects you created (the image.createGraphics()
result)
That suggests the PR should track something like Graphics2D imageGraphics =
null; and if (imageGraphics != null) imageGraphics.dispose(); in finally.
Recommendation (what I would request changes on)
Change the code to dispose only the image graphics created by
image.createGraphics():
introduce Graphics2D imageGraphics = null;
set it right after creation
finally { if (imageGraphics != null) imageGraphics.dispose(); }
Keep printerGraphics for drawing the final image, but do not dispose it.
Add a small regression test if the project has printing tests (or at
least a unit-level test around resource disposal if feasible). If not feasible,
at least add a comment clarifying ownership: “Do not dispose the printer
graphics; only dispose graphics created for rasterization.”
Bottom line
Good idea, but as written, I would not approve: the finally block can
dispose the wrong graphics object and violates typical ownership rules around
Printable#print(Graphics, ...).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
