-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
6286501: JTabbedPane throws NPE from its stateChanged listener in particular case #10216
Conversation
👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into |
@@ -4099,7 +4099,7 @@ public void stateChanged(ChangeEvent e) { | |||
|
|||
setFocusIndex(tabPane.getSelectedIndex(), false); | |||
|
|||
if (scrollableTabLayoutEnabled()) { | |||
if (tabPane.getLayout() instanceof TabbedPaneScrollLayout) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see BasicTabbedPaneUI.tabPane object getting used in other listeners in this Handler class. Is there a reason why only when we hit stateChanged() function we are seeing NPE?
Is this NPE for tabPane object itself or Layout not being initialized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, when a tab index is selected via tbp.setSelectedIndex()
in testcase, it fires a state change event (via DefaultSingleSelectionModel#setSelectedIndex
) which is caught by the application and in this case, state change listener in the testcase calls JTabbedPane.updateUI()
which calls JComponent.setUI()
which first uninstalls UI (which made tabPane null
in BasicTabbedPaneUI#uninstallUI
) and then install new UI instance but when it goes to JDK's stateChange listener BasicTabbedPaneUI.stateChanged()
it passes the new instance but scrollableTabLayoutEnabled() still uses old UI instance in which tabPane is already made null so NPE happens, so it is better to use the tabPane instance passed to UI's stateChange listener..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we install new UI instance will we not call BasicTabbedPaneUI.installUI() to update current tabPane object? or is it happening at later stage(like after BasicTabbedPaneUI.stateChanged() is called)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
installUI is called for new UI instance with current tabpane object but scrollableTabLayoutEnabled was using the old UI instance whose tabpane object is already reset to null.
@prsadhuk 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:
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 173 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit ab7f58a.
Your commit was automatically rebased without conflicts. |
JTabbedPane throws NPE from its stateChanged listener as it tries to access uninstantiated TabbedPane.
Fix is to use the tabPane passed to stateChangeListener so that correct tabPane is accessed.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10216/head:pull/10216
$ git checkout pull/10216
Update a local copy of the PR:
$ git checkout pull/10216
$ git pull https://git.openjdk.org/jdk pull/10216/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10216
View PR using the GUI difftool:
$ git pr show -t 10216
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10216.diff