6521141: DebugGraphics NPE @ setFont(); #9673

TejeshR13 · 2022-07-28T11:47:03Z

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

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
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

bridgekeeper · 2022-07-28T11:48:24Z

👋 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.

openjdk · 2022-07-28T11:49:58Z

⚠️ @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).

openjdk · 2022-07-28T11:52:43Z

@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.

mlbridge · 2022-07-28T11:56:32Z

Webrevs

prsadhuk

I guess exposing no-args public constructor was wrong and it should have been protected from beginning.
Also, please add a testcase.

prsadhuk · 2022-07-28T12:49:59Z

src/java.desktop/share/classes/javax/swing/DebugGraphics.java

-                " Setting clipRect: " + (new Rectangle(x, y, width, height)) +
-                " New clipRect: " + graphics.getClip());
+                    " Setting clipRect: " + (new Rectangle(x, y, width, height)) +
+                    " New clipRect: " + graphics.getClip());


Why this line change is needed? Probably you have added some tabs..Remove this change and similarly from other logs down below.

I think auto indentation has caused this, will undo these tabs changes.

TejeshR13 · 2022-07-28T14:45:36Z

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.

kevinrushforth · 2022-07-28T15:10:43Z

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.

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).

TejeshR13 · 2022-07-28T15:21:48Z

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.

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.

prrace · 2022-07-28T18:11:02Z

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();
}

TejeshR13 · 2022-07-29T05:14:31Z

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.

prrace · 2022-07-29T16:50:08Z

src/java.desktop/share/classes/javax/swing/DebugGraphics.java

+        //  Creates a Graphics context when Application calls the constructor
+        //  directly.
+        StackWalker walker = StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE);
+        if ((graphics == null) && (walker.getCallerClass() != this.getClass())) {


I don't know if creating a StackWalker is expensive but I think it should be done only if graphics == null

Also the version of getInstance() being called here might throw SecurityException
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html#getInstance(java.lang.StackWalker.Option)

So you'll need to wrap it in a doPrivileged.

Checking only if graphics == null will also handle the case when user sets the graphics to null by passing null in the constructor, so removing StackWalker and checking only for graphics == null will handle both the cases whether its internal class or called from other class.

@prrace Then I will proceed by removing Stackwalker and create a graphics instance only if its null.

No, because that won't work, or more precisely will be a waste of work.
See
DebugGraphics(Graphics graphics) {
this(); // << no-args constructor and graphics will still be null
this.graphics = null;
}

graphics will ALWAYS be null when you come via here which is exactly why we needed StackWalker

Ok, then we should set graphics only If the no-args constructor is called by another program and not via by the same class. I had understood the previous suggestion wrongly, now I got it. We are supposed to create instance of stackwalker only if graphics is null and wrap it around doPrivileged.

…tions based on review

turbanoff · 2022-08-03T06:15:05Z

src/java.desktop/share/classes/javax/swing/DebugGraphics.java

+                }
+            });
+
+            if(walker.getCallerClass() != this.getClass()) {


Let's add space after if

prrace · 2022-08-04T20:24:38Z

src/java.desktop/share/classes/javax/swing/DebugGraphics.java

@@ -72,11 +74,32 @@ public class DebugGraphics extends Graphics {
    /**
     * Constructs a new debug graphics context that supports slowed
     * down drawing.
+     * <p>
+     * NOTE: This constructor should not be called by
+     * application, it is for Internal use only. When called directly


"application" -> "applications"
"Internal" -> "internal"

prrace · 2022-08-04T20:25:13Z

test/jdk/javax/swing/DebugGraphics/DebugGraphicsNPETest.java

+
+/* @test
+ * @bug 6521141
+ * @key headful


Why does this test need to be headful ?

Actually not required, since no UI is used..... Will remove and test......

TejeshR13 · 2022-08-05T09:27:11Z

@prrace, I have updated the PR, let me know if its ok now......

prsadhuk · 2022-08-05T10:52:43Z

test/jdk/javax/swing/DebugGraphics/DebugGraphicsNPETest.java

+ */
+public class DebugGraphicsNPETest {
+    public static void main(String[] args) throws Exception {
+        SwingUtilities.invokeAndWait(new Runnable() {


No need of using EDT here as no Swing components in used. You can just move runTest() lines in main..

Yeah right. I have updated the test @prsadhuk

prsadhuk · 2022-08-08T12:05:51Z

src/java.desktop/share/classes/javax/swing/DebugGraphics.java

    public DebugGraphics() {
        super();
        buffer = null;
        xOffset = yOffset = 0;
+
+        //  Creates a Graphics context when the constructor is called.
+        if (graphics == null) {


Probably it will be better to use this.graphics here and below while setting.

Updated @prsadhuk .

openjdk · 2022-08-11T22:28:04Z

@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:

083e014: 8292233: Increase symtab hash table size
45e5b31: 8292244: Remove unnecessary include directories
9bfffa0: 8291945: Add OSInfo API for static OS information
bd58553: 8290833: Remove ConstantPoolCache::walk_entries_for_initialization()
755ecf6: 8292153: x86: Represent Registers as values
dedc05c: 8291640: java/beans/XMLDecoder/8028054/Task.java should use the 3-arg Class.forName
3d20a8b: 8291959: FileFontStrike#initNative does not properly initialize IG Table on Windows
a28ab7b: 8288568: Reduce runtime of java.security microbenchmarks
7ea9ba1: 8292064: Convert java/lang/management/MemoryMXBean shell tests to java version
fc1d94e: 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 · 2022-08-12T06:27:32Z

/integrate

openjdk · 2022-08-12T06:29:01Z

@TejeshR13
Your change (at version cedaa2f) is now ready to be sponsored by a Committer.

prsadhuk · 2022-08-16T05:51:14Z

/sponsor

openjdk · 2022-08-16T05:53:01Z

Going to push as commit 21f4eb2.
Since your change was applied there have been 353 commits pushed to the master branch:

6e6ae59: 8292286: Convert PlaceholderTable to ResourceHashtable
ea2c82e: 8291949: Unexpected extending of SupportedGroups
b5707b0: 8292196: Reduce runtime of java.util.regex microbenchmarks
b00eede: 8291511: Redefinition of EXIT_FAILURE in libw2k_lsa_auth
3a09077: 8291337: Reduce runtime of vm.lamdba microbenchmarks
dd2034b: 8291972: Fix double copy of arguments when thawing two interpreted frames
aa5b718: 8292182: [TESTLIB] Enhance JAXPPolicyManager to setup required permissions for jtreg version 7 jar
695bb39: 8292347: Remove unused Type::is_ptr_to_boxing_obj
ec96b1f: 8290291: G1: Merge multiple calls of block_size in HeapRegion::block_start
fd4b2f2: 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.

openjdk · 2022-08-16T05:53:31Z

@prsadhuk @TejeshR13 Pushed as commit 21f4eb2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

TejeshR13 added 11 commits January 2, 2022 20:01

Initial Commit.

288c597

Merge branch 'master' of github.com:TejeshR13/jdk

2403494

Merge remote-tracking branch 'upstream/master'

92823e0

Merge branch 'openjdk:master' into master

347af6e

Merge remote-tracking branch 'upstream/master'

98cf2b7

Merge branch 'master' of github.com:TejeshR13/jdk

8788395

Merge remote-tracking branch 'upstream/master'

a6d536c

Merge remote-tracking branch 'upstream/master'

c36d137

Merge remote-tracking branch 'upstream/master'

5a80d75

Merge remote-tracking branch 'upstream/master'

a7788e4

Fix : graphics null check

12511ec

openjdk bot added the rfr Pull request is ready for review label Jul 28, 2022

openjdk bot added the client client-libs-dev@openjdk.org label Jul 28, 2022

prsadhuk reviewed Jul 28, 2022

View reviewed changes

TejeshR13 added 2 commits July 28, 2022 21:59

Undo of Auto Indentation

04d444e

Test added

a9dbc1b

openjdk bot removed the rfr Pull request is ready for review label Jul 28, 2022

Removed whitespace error

bc36344

openjdk bot added the rfr Pull request is ready for review label Jul 28, 2022

TejeshR13 added 2 commits July 29, 2022 19:48

Fix: Java doc modified and Graphics contexts created in null case

22bc13a

Minor <p> update

391b20d

prrace reviewed Jul 29, 2022

View reviewed changes

Updations on removing StackWalker for graphics instance and test upda…

d9d62c8

…tions based on review

openjdk bot added the csr Pull request needs approved CSR before integration label Aug 1, 2022

Updated based on review comments

8f5cfb5

turbanoff reviewed Aug 3, 2022

View reviewed changes

Updated based on review comments

32d7315

prrace suggested changes Aug 4, 2022

View reviewed changes

Updated based on review comments

063248a

prsadhuk reviewed Aug 5, 2022

View reviewed changes

Updated based on review comments

e9b164d

AJ1062910 approved these changes Aug 5, 2022

View reviewed changes

prrace approved these changes Aug 5, 2022

View reviewed changes

prsadhuk reviewed Aug 8, 2022

View reviewed changes

Updated based on review comments

cedaa2f

openjdk bot removed the csr Pull request needs approved CSR before integration label Aug 11, 2022

openjdk bot added the ready Pull request is ready to be integrated label Aug 11, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 12, 2022

openjdk bot added the integrated Pull request has been integrated label Aug 16, 2022

openjdk bot closed this Aug 16, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6521141: DebugGraphics NPE @ setFont(); #9673

6521141: DebugGraphics NPE @ setFont(); #9673

TejeshR13 commented Jul 28, 2022 •

edited by openjdk bot

bridgekeeper bot commented Jul 28, 2022

openjdk bot commented Jul 28, 2022

openjdk bot commented Jul 28, 2022

mlbridge bot commented Jul 28, 2022 •

edited

prsadhuk left a comment

prsadhuk Jul 28, 2022

TejeshR13 Jul 28, 2022

TejeshR13 commented Jul 28, 2022

kevinrushforth commented Jul 28, 2022

TejeshR13 commented Jul 28, 2022

prrace commented Jul 28, 2022

TejeshR13 commented Jul 29, 2022

prrace Jul 29, 2022

TejeshR13 Aug 1, 2022

TejeshR13 Aug 1, 2022 •

edited

prrace Aug 1, 2022 •

edited

TejeshR13 Aug 2, 2022 •

edited

turbanoff Aug 3, 2022

TejeshR13 Aug 3, 2022

prrace Aug 4, 2022

TejeshR13 Aug 5, 2022

prrace Aug 4, 2022

TejeshR13 Aug 5, 2022

TejeshR13 commented Aug 5, 2022

prsadhuk Aug 5, 2022

TejeshR13 Aug 5, 2022

prsadhuk Aug 8, 2022

TejeshR13 Aug 8, 2022

openjdk bot commented Aug 11, 2022 •

edited

TejeshR13 commented Aug 12, 2022

openjdk bot commented Aug 12, 2022

prsadhuk commented Aug 16, 2022

openjdk bot commented Aug 16, 2022

openjdk bot commented Aug 16, 2022

6521141: DebugGraphics NPE @ setFont(); #9673

6521141: DebugGraphics NPE @ setFont(); #9673

Conversation

TejeshR13 commented Jul 28, 2022 • edited by openjdk bot

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Jul 28, 2022

openjdk bot commented Jul 28, 2022

openjdk bot commented Jul 28, 2022

mlbridge bot commented Jul 28, 2022 • edited

Webrevs

prsadhuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejeshR13 commented Jul 28, 2022

kevinrushforth commented Jul 28, 2022

TejeshR13 commented Jul 28, 2022

prrace commented Jul 28, 2022

TejeshR13 commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejeshR13 Aug 1, 2022 • edited

Choose a reason for hiding this comment

prrace Aug 1, 2022 • edited

Choose a reason for hiding this comment

TejeshR13 Aug 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejeshR13 commented Aug 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Aug 11, 2022 • edited

TejeshR13 commented Aug 12, 2022

openjdk bot commented Aug 12, 2022

prsadhuk commented Aug 16, 2022

openjdk bot commented Aug 16, 2022

openjdk bot commented Aug 16, 2022

TejeshR13 commented Jul 28, 2022 •

edited by openjdk bot

mlbridge bot commented Jul 28, 2022 •

edited

TejeshR13 Aug 1, 2022 •

edited

prrace Aug 1, 2022 •

edited

TejeshR13 Aug 2, 2022 •

edited

openjdk bot commented Aug 11, 2022 •

edited