jdk
jdk copied to clipboard
openjdk
6521141: DebugGraphics NPE @ setFont();
DebugGraphics class has a Graphics instance which is been used in slowed down drawing. The graphics object is not initialized anywhere inside the class, where it is expected to set explicitly by the user. When the user doesn't set it and try to use the any mehtods like drawing/setFont, NPE is raised which is expected. The scenario is taken care by checking if the graphics object is null before using it inside the class, thus eliminating the NPE case.
Progress
- [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [x] Change requires a CSR request to be approved
Issues
- JDK-6521141: DebugGraphics NPE @ setFont();
- JDK-8291634: DebugGraphics NPE @ setFont(); (CSR)
Reviewers
- Phil Race (@prrace - Reviewer) ⚠️ Review applies to e9b164d1
- @AJ1032480 (no known github.com user name / role) ⚠️ Review applies to e9b164d1
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9673/head:pull/9673
$ git checkout pull/9673
Update a local copy of the PR:
$ git checkout pull/9673
$ git pull https://git.openjdk.org/jdk pull/9673/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9673
View PR using the GUI difftool:
$ git pr show -t 9673
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9673.diff
:wave: Welcome back tr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
⚠️ @TejeshR13 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).
@TejeshR13 The following label will be automatically applied to this pull request:
client
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.
Webrevs
- 09: Full - Incremental (cedaa2f8)
- 08: Full - Incremental (e9b164d1)
- 07: Full - Incremental (063248ab)
- 06: Full - Incremental (8f5cfb50)
- 05: Full - Incremental (d9d62c86)
- 04: Full - Incremental (22bc13a4)
- 03: Full - Incremental (bc363443)
- 02: Full - Incremental (a9dbc1b6)
- 01: Full - Incremental (04d444e3)
- 00: Full (12511ec1)
I guess exposing no-args public constructor was wrong and it should have been protected from beginning. Also, please add a testcase.
Yeah, its been used internally by other constructors after setting the graphics instances. Exposing it as public causes user to create it without graphics been set. I tried modifying it to protected and ran the test, but some html test fails.
I guess exposing no-args public constructor was wrong and it should have been protected from beginning. Also, please add a testcase.
Yeah, its been used internally by other constructors after setting the
graphicsinstances. Exposing it as public causes user to create it withoutgraphicsbeen set. I tried modifying it to protected and ran the test, but some html test fails.
Even if it hadn't failed the test, you cannot simply change a method, constructor, or field in a public (or protected) class in an exported package from public to protected, since that would be an incompatible API change (at least not without a very compelling reason and a lot of discussion, and even then, probably not).
I guess exposing no-args public constructor was wrong and it should have been protected from beginning. Also, please add a testcase.
Yeah, its been used internally by other constructors after setting the
graphicsinstances. Exposing it as public causes user to create it withoutgraphicsbeen set. I tried modifying it to protected and ran the test, but some html test fails.Even if it hadn't failed the test, you cannot simply change a method, constructor, or field in a public (or protected) class in an exported package from public to protected, since that would be an incompatible API change (at least not without a very compelling reason and a lot of discussion, and even then, probably not).
Yeah, sure @kevinrushforth.
You wrote " The graphics object is not initialized anywhere inside the class, where it is expected to set explicitly by the user. "
which reads a bit like if the user does DebugGraphics dg = new DebugGraphics(); they must then follow it up with dg.setGraphics(g); however there's no such method.
So any DebugGraphics created this way is useless. You can't use it for debugging Making it protected won't prevent mis-use. Someone could subclass and provide a public no-args constructor and the problem will recur.
The javadoc for the class says : DebugGraphics objects are rarely created by hand
So likely no one should be doing this.
I'd just add javadoc saying "This constructor should not be called by applications, it is for internal use only. When called directly it will create an un-usable instance".
The only other thing I can think of is to try to figure out by using https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html#getCallerClass())
if it was called from outside DebugGraphics, and if so install some other graphics instead.
something like .. if (graphics == null && stackWalker.getCallerClass() != this.class ) { BufferedImage bi = new BufferedImage(1,1,BufferedImage.TYPE_INT_RGB); graphics = bi.createGraphics(); }
Yeah, this looks like a better idea @prrace. I will update the java doc and also will create the BufferedImage graphics from the no-arg constructor. With this we will be warning about the usage of no-arg constructor to user and also handle NPE cases.
@TejeshR13 This change now passes all automated pre-integration checks.
ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.
After integration, the commit message for the final commit will be:
6521141: DebugGraphics NPE @ setFont();
Reviewed-by: prr
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.
At the time when this comment was updated there had been 329 new commits pushed to the master branch:
- 083e014d0caf673f5da04229ba263f45724cb418: 8292233: Increase symtab hash table size
- 45e5b31a183e2ddca8f8d10a922b20af97efdaff: 8292244: Remove unnecessary include directories
- 9bfffa082e85372ec39a0fdab6d5f2c175162246: 8291945: Add OSInfo API for static OS information
- bd5855337c9eebc0044fd467fa39a671e260f891: 8290833: Remove ConstantPoolCache::walk_entries_for_initialization()
- 755ecf6b7384e67ccb51c4498f94336631db690d: 8292153: x86: Represent Registers as values
- dedc05cb40617f7b7e2cc235528b1892dcba4cd3: 8291640: java/beans/XMLDecoder/8028054/Task.java should use the 3-arg Class.forName
- 3d20a8b20a636e4c11ad1568b011191726b45b90: 8291959: FileFontStrike#initNative does not properly initialize IG Table on Windows
- a28ab7b62abcfce56425d62d5a8162d8f1623393: 8288568: Reduce runtime of java.security microbenchmarks
- 7ea9ba1f6c18ace5aa0896ab8676926fdc0d64ea: 8292064: Convert java/lang/management/MemoryMXBean shell tests to java version
- fc1d94ef1a4b088044465a5df5d8f40ab2c11253: 8292232: AIX build failure by JDK-8290840
- ... and 319 more: https://git.openjdk.org/jdk/compare/c7c20661eee727ed8354b19723c359ae7c2d4bd8...master
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prsadhuk, @prrace) but any other Committer may sponsor as well.
➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@TejeshR13 Your change (at version cedaa2f8f46c9444e0c84fd2938f628a6977f7ec) is now ready to be sponsored by a Committer.
Going to push as commit 21f4eb2233a95be44a5db59b7791cd952ddbd56e.
Since your change was applied there have been 353 commits pushed to the master branch:
- 6e6ae596d6bd73909b90911e01fbd0c16f6335e1: 8292286: Convert PlaceholderTable to ResourceHashtable
- ea2c82e74f5580f396920f9e561cbec80c03f373: 8291949: Unexpected extending of SupportedGroups
- b5707b0376660cd8763e46d525ba614b08a59d7b: 8292196: Reduce runtime of java.util.regex microbenchmarks
- b00eedeb029445417f99e8aa4e8fca12e5c69155: 8291511: Redefinition of EXIT_FAILURE in libw2k_lsa_auth
- 3a090777bada2e00f573fe8ab113bfa3884982eb: 8291337: Reduce runtime of vm.lamdba microbenchmarks
- dd2034b00725f0fc777c1706b1db898475e89c5c: 8291972: Fix double copy of arguments when thawing two interpreted frames
- aa5b71893307b9fe6137bc3541edccaab73735ac: 8292182: [TESTLIB] Enhance JAXPPolicyManager to setup required permissions for jtreg version 7 jar
- 695bb3939135394a4627d1c41cfc30d11b19bf48: 8292347: Remove unused Type::is_ptr_to_boxing_obj
- ec96b1f1879ee8ee5164be22d0a178f9d5048ab9: 8290291: G1: Merge multiple calls of block_size in HeapRegion::block_start
- fd4b2f2868ac6eaf192b50db5c5adc76e0d54308: 8291718: Remove mark_for_deoptimization from klass unloading
- ... and 343 more: https://git.openjdk.org/jdk/compare/c7c20661eee727ed8354b19723c359ae7c2d4bd8...master
Your commit was automatically rebased without conflicts.
@prsadhuk @TejeshR13 Pushed as commit 21f4eb2233a95be44a5db59b7791cd952ddbd56e.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.