Skip to content
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

8244418: MenuBar: IOOB exception on requestFocus on empty bar #216

Closed
wants to merge 4 commits into from

Conversation

@aghaisas
Copy link
Collaborator

aghaisas commented May 8, 2020

Issue :
https://bugs.openjdk.java.net/browse/JDK-8244418

Root Cause :
Incorrect assumption about menu list size.

Fix :
Added check for empty menu list before trying to access it.

Test :
Added a unit test that fails before fix and passes after it.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8244418: MenuBar: IOOB exception on requestFocus on empty bar

Reviewers

  • Jeanette Winzenburg (fastegal - Author)
  • Kevin Rushforth (kcr - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/216/head:pull/216
$ git checkout pull/216

@bridgekeeper
Copy link

bridgekeeper bot commented May 8, 2020

👋 Welcome back aghaisas! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label May 8, 2020
@mlbridge
Copy link

mlbridge bot commented May 8, 2020

Copy link
Collaborator

kleopatra left a comment

looks good - just two inline questions :)

openMenuButton = ((MenuBarButton)container.getChildren().get(0));
setFocusedMenuIndex(0);
openMenuButton.setHover();
if (!container.getChildren().isEmpty()) {

This comment has been minimized.

@kleopatra

kleopatra May 8, 2020 Collaborator

wondering whether this would be an appropriate time to simplify the logic a bit: as unSelectMenus is called in both if and else block, it could be condensed to

unSelectMenus();
if (t1 && !container.getChildren().isEmpty()) {
   ...
}  

might overlook something, though

This comment has been minimized.

@aghaisas

aghaisas May 12, 2020 Author Collaborator

Good suggestion. Fixed.

focusedMenu = index == -1 ? null : getSkinnable().getMenus().get(index);
focusedMenu = null;

if (index != -1 && !(getSkinnable().getMenus().isEmpty())) {
focusedMenu = getSkinnable().getMenus().get(index);
}

Comment on lines 478 to 485

This comment has been minimized.

@kleopatra

kleopatra May 8, 2020 Collaborator

don't quite understand why this change is necessary? The guard in the listener seems to be enough, at least the test passes without this. If it's needed to fix another potentially breaking path to this, would it be possible to add a test method that fails/passes before/after?

This comment has been minimized.

@aghaisas

aghaisas May 12, 2020 Author Collaborator

This was the place where exception was occurring. Hence, I added this check.
When I ran against the test, I still got the exception at caller lambda. I added that check later.
Wanted to remove this check - but simple code scan revealed this method gets called from engine.addTraverseListener() listener with index = 0. Hence, I have kept this check. Covering this scenario in test seems tough.

This comment has been minimized.

@kleopatra

kleopatra May 13, 2020 Collaborator

Hmm .. re-reading my comment: was assuming and then formulating round a corner, sry. Will try again

My assumption (from the method implementation before the fix) was that the method expects only valid indices (either -1 or an index in range) such that

 assertEquals(focusedMenuIndex, getMenus().indexOf(focusedMenu);

Which would leave the validity check to the caller.

With that assumption, the old implementation were just fine. The one location that calls it with an invalid index is the one you fixed (not called if empty), the other is the traversalListener (which I silently and incorrectly dropped from my mind, because it's never notified anyway - which is nothing but a bold guess ;)

With the change, the constraint doesn't hold (as it didn't before if the caller passed in an invalid index) it just doesn't blow. Not sure what - if any - bad might happen if we have a focusedMenuIndex >= getMenu().size() (in particular empty menus).

The other way round: the method might take the responsibility to protect itself (no precondition and the postcondition to guarantee the constraint above).

My preference would be to keep the method as is, and change the callers to check for a valid index. Hard to test, one way or other ;)

This comment has been minimized.

@aghaisas

aghaisas May 15, 2020 Author Collaborator

I differ on this suggestion.
My thinking is - list access in setFocusedMenuIndex() method should have this check. It is not up to the caller to know the internal details of the method. That's the root cause of Exception.
I added another check in menuBarFocusedPropertyListener because, it accesses the different list - container.getChildren().
In general, I feel, the validity check near the list usage is logical and readable as well.

This comment has been minimized.

@kleopatra

kleopatra May 15, 2020 Collaborator

hmm .. yeah I'm aware of getContainer vs. getMenus - but they should be the same size, shouldn't they?

Anyway, if focusedIndex != getMenus().indexOf all users of focusedIndex have to include a check for validity. That might be prevented by not allowing it here in the method, in setting its value not unconditionally to the given index but guard it against being valid:

focusedMenuIndex = index >= getMenus().size() ? -1 : index

Then focused is either valid or -1.

This comment has been minimized.

@aghaisas

aghaisas May 18, 2020 Author Collaborator

Good suggestion on not setting the foucusedMenuIndex unconditionally.
Also, we need to check for index < -1 : just to tighten up this method.
I have added this check.

This comment has been minimized.

@kleopatra

kleopatra May 18, 2020 Collaborator

glad we came to an agreement at last :)

@kevinrushforth
Copy link
Member

kevinrushforth commented May 15, 2020

/reviewers 2

@openjdk
Copy link

openjdk bot commented May 15, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

menuBar.getScene().getWindow().requestFocus();

int focusedIndex = MenuBarSkinShim.getFocusedIndex(skin);
assertEquals(focusedIndex, -1);
Comment on lines 131 to 134

This comment has been minimized.

@kleopatra

kleopatra May 17, 2020 Collaborator

the assert should be the other way round, expected value should be the first parameter :)

didn't notice on first read and now only when writing a test case against our argument, c&p'ed the test as-is, replaced the requestFocus with simulating the notification from traversalListener and was confused about its value being -1

Here's that modified test method (requires test api in MenuBarSkin and shim):

@Test public void testSimulateTraverseIntoOnEmptyMenubar() {
    menuBar.setFocusTraversable(true);

    AnchorPane root = new AnchorPane();
    root.getChildren().add(menuBar);
    startApp(root);

    MenuBarSkin skin = (MenuBarSkin)menuBar.getSkin();
    assertTrue(skin != null);

    // simulate notification from traversalListener 
    MenuBarSkinShim.setFocusedIndex(skin, 0);
    int focusedIndex = MenuBarSkinShim.getFocusedIndex(skin);
    assertEquals(-1, focusedIndex);
}

This comment has been minimized.

@aghaisas

aghaisas May 18, 2020 Author Collaborator

Corrected the similar asserts in this file.

Thanks for suggestion on test. Yes. At best, we can simulate this with Shim.
I have added it now.

Copy link
Collaborator

kleopatra left a comment

looks fine :)

@kevinrushforth kevinrushforth self-requested a review May 26, 2020
Copy link
Member

kevinrushforth left a comment

The fix looks good aside from the one added method used for testing.

The test looks good as well. I confirm that it fails without your fix and passes with your fix.

@@ -762,6 +760,10 @@ int getFocusedMenuIndex() {
return focusedMenuIndex;
}

void setFocusedIndex(int index) {
this.setFocusedMenuIndex(0);

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 12, 2020 Member

Shouldn't this be this.setFocusedMenuIndex(index)? The only reason this isn't causing problems is that the test you added never calls it with anything other than 0.

Also, the naming of this method is a bit confusing. I might recommend calling it test_setFocusedMenuIndex, or else just change the private setFocusedMenuIndex method to be package-scope. Either way adding a comment indicating that it is used for testing purposes seems a good idea (if you keep this method then the comment could say that it is only for testing purposes; if you make setFocusedMenuIndex package-scope then you could say that it is package scope because it is used for testing).

This comment has been minimized.

@kleopatra

kleopatra Jun 13, 2020 Collaborator

darn .. overlooked that fixed index ..

As to the naming: personally, I hate violation of naming conventions even for test-only methods, so would tend to make setFocusedMenuIndex package and doc as being used for testing as well as internally.

We might consider aligning the corresponding methods in the shim (they are also slightly confusing in get/set/FocusedIndex).

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 13, 2020 Member

Good suggestion.

This comment has been minimized.

@aghaisas

aghaisas Jun 15, 2020 Author Collaborator

Thanks @kevinrushforth for catching this. It was a big overlook on my part.

this.focusedMenuIndex = index;
focusedMenu = index == -1 ? null : getSkinnable().getMenus().get(index);
focusedMenuIndex = (index >= -1 && index < getSkinnable().getMenus().size())? index : -1;
focusedMenu = (focusedMenuIndex != -1)? getSkinnable().getMenus().get(index) : null;

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 12, 2020 Member

Minor: we usually put a space before the ? (I probably wouldn't have mentioned it, but there is a substantive change you need to make anyway).

Copy link
Member

kevinrushforth left a comment

Looks good.

Copy link
Collaborator

kleopatra left a comment

bot says this needs a re-review - here we go: looks fine.

BTW: what we might have learned is to test our test methods as well - if possible :)

@openjdk
Copy link

openjdk bot commented Jun 16, 2020

@aghaisas This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8244418: MenuBar: IOOB exception on requestFocus on empty bar

Reviewed-by: fastegal, kcr
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 35 commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate bf2e972d5821bde1eb40851f9c752e47d8658ba1.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jun 16, 2020
@aghaisas
Copy link
Collaborator Author

aghaisas commented Jun 16, 2020

/integrate

@openjdk openjdk bot closed this Jun 16, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 16, 2020
@openjdk
Copy link

openjdk bot commented Jun 16, 2020

@aghaisas The following commits have been pushed to master since your change was applied:

  • bf2e972: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars
  • b200891: 8244824: TableView : Incorrect German translation
  • b2b46eb: 8242892: SpinnerValueFactory has an implicit no-arg constructor
  • afa805f: 8245575: Show the ContextMenu of input controls with long press gesture on iOS
  • 5304266: 8245635: GlassPasteboard::getUTFs fails on iOS
  • ba501ef: 8246357: Allow static build of webkit library on linux
  • a02e09d: 8246195: ListViewSkin/Behavior: misbehavior on switching skin
  • 9749982: 8246204: No 3D support for newer Intel graphics drivers on Linux
  • 6bd0e22: 8239095: Upgrade libFFI to the latest 3.3 version
  • 853ac78: 8245282: Button/Combo Behavior: memory leak on dispose
  • a78b3fb: 8242523: Update the animation and clip envelope classes
  • 1ab653c: 8244657: ChoiceBox/ToolBarSkin: misbehavior on switching skin
  • 804ccce: 8244195: [TEST_BUG] Convert the system tests TabPanePermuteGetTabsTest to unit test
  • 9edba9c: 8243110: SVGTest.testSVGRenderingWithPattern fails intermittently
  • 168b7f7: 8246099: Intermittent test failures in SandboxAppTest
  • c41777e: 8245634: [TestBug] Enable and fix tests ignored with message "impl_cssSet API removed"
  • 3ceee69: 8245499: Text input controls should show handles on iOS
  • 8914bd2: 8234540: javafx.web LeakTest.testGarbageCollectability fails intermittently
  • 16f446a: 8234876: Unit test classes should not extend Application
  • 2d98fe6: 8245601: TESTBUG] Disable TabPaneDragPolicyTest on Mac until JDK-8213136 is fixed and fix ISE
  • f3190db: 8244531: Tests: add support to identify recurring issues with controls et al
  • 1971c70: 8245457: TestBug] Enable and fix ignored tests in ButtonBaseTest & ButtonTest
  • 6e0b45a: 8245183: Two fxml unit tests log warnings about deprecated escape sequences
  • a13a642: 8244579: Windows "User Objects" leakage with WebView
  • 37b5edc: 8245456: MacPasteboard throws ClassCastException on static builds
  • 6e03930: 8237602: TabPane doesn't respect order of TabPane.getTabs() list
  • bb24322: 8244112: Skin implementations: must not violate contract of dispose
  • dbb6437: 8244647: Wrong first layout pass of Scrollbar controls on touch supported devices
  • 7b06190: 8242548: Wrapped labeled controls using -fx-line-spacing cut text off
  • 435671e: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.
  • c14cc44: 8244417: support static build for Windows
  • b14e085: 8244735: Error on iOS passing keys with unicode values greater than 255
  • b0d66d0: 8242508: Upgrade to Visual Studio 2019 version 16.5.3
  • 0f87d20: 8244487: One Windows 10 SDK file missing from FX build
  • 4ec163d: 8242001: ChoiceBox: must update value on setting SelectionModel, part2

Your commit was automatically rebased without conflicts.

Pushed as commit fb962ac.

@aghaisas aghaisas deleted the aghaisas:focusOnEmptyMenubar branch Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.