<AWT Dev> [12] RFR : JDK-8198000 : java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert on Windows
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Thu Nov 8 18:06:51 UTC 2018
+1
On 08/11/2018 09:33, Phil Race wrote:
> Ok +1
>
> -phil.
>
> On 11/8/18 9:27 AM, Ambarish Rapte wrote:
>>
>> Hi Phil,
>>
>> Thanks for the guiding over discussion,
>>
>> Please take a look at the updated webrev : http://cr.openjdk.java.net/~arapte/8198000/webrev.05/
>>
>> Regards,
>>
>> Ambarish
>>
>> *From:*Ambarish Rapte
>> *Sent:* Wednesday, November 07, 2018 10:49 PM
>> *To:* Philip Race <philip.race at oracle.com>
>> *Cc:* awt-dev at openjdk.java.net
>> *Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 : java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert on Windows
>>
>> Hi Phil,
>>
>> > I don't think so. I already pointed out that it is apparently returning -1 in your test
>> > when HIWORD is 1. I am pretty sure it isn't expecting you to interpret 0xFFFF stored
>> > in 16 bits as positive.
>>
>> The value 0xFFFF is -1 when saved into a signed short and 65535 when assigned to unsigned short.
>>
>> I was trying to find any supporting documentation or source code from MSDN. But cannot be certain of the behavior based on documentation.
>>
>> So I also think that we can go ahead with the earlier version of fix as you suggested. : http://cr.openjdk.java.net/~arapte/8198000/webrev.04/
>>
>> Please take a look at the fix, The webrev has fix only for JDK-8198000.
>> The fix change for issue #2 i.e. cmath::pow function is removed from webrev.04
>>
>> > These are platform UI components. They can behave differently w.r.t things like
>> > how they respond to mouse clicks, all dependent on what the platform norms are.
>> > I see no reason to go out of our way to "make windows mouse clicks behave like mac".
>>
>> This looks good to me as well. With the changes in webrev.04, an item gets selected only when mouse is clicked on the item.
>>
>> > Not understanding you. How can you tell if 0x0001 is supposed to be "1" or "65536" ?
>>
>> Sorry for not explaining the fix in earlier email with webrev.03.
>>
>> I have removed this change from the webrev.
>>
>> I should not have mixed fix of the two issues. If the issue #2 needs to be addressed, I request if we can work on this issue separately.
>>
>> > So if the user clicks outside we do nothing. What is wrong with that ?
>>
>> I too see nothing wrong in this behavior. Mouse click outside the client area can be neglected under this fix.
>>
>> Please take a look at the updated webrev.04 which includes fix only for JDK-8198000
>>
>> Regards,
>>
>> Ambarish
>>
>> *From:*Philip Race
>> *Sent:* Tuesday, November 06, 2018 10:21 PM
>> *To:* Ambarish Rapte <ambarish.rapte at oracle.com <mailto:ambarish.rapte at oracle.com>>
>> *Cc:* awt-dev at openjdk.java.net <mailto:awt-dev at openjdk.java.net>
>> *Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 : java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert on Windows
>>
>> On 11/5/18 8:51 AM, Ambarish Rapte wrote:
>>
>> Hi Phil,
>>
>> The little abstraction or lack of clarity in documentation is leading us to make some assumptions.
>>
>> Just trying to connect the pieces,
>>
>> I referred these two doc pages to learn LOWORD behavior:
>>
>> 1. LOWORD : https://msdn.microsoft.com/en-us/library/windows/desktop/ms632659(v=vs.85).aspx <https://msdn.microsoft.com/en-us/library/windows/desktop/ms632659%28v=vs.85%29.aspx>
>> 2. WORD : https://docs.microsoft.com/en-us/windows/desktop/winprog/windows-data-types
>>
>> And the documentation of LB_ITEMFROMPOINT does not mention LB_ERR(-1)
>>
>> Based on these documentation, it looks safe to conclude that LOWORD would be positive.
>>
>>
>> I don't think so. I already pointed out that it is apparently returning -1 in your test
>> when HIWORD is 1. I am pretty sure it isn't expecting you to interpret 0xFFFF stored
>> in 16 bits as positive.
>>
>> Issue #1 :
>>
>> Yes, the HIWORD is 1, when the mouse is clicked inside the list’s client area but not on the item.
>>
>> Once we separate the HIWORD and LOWORD, LOWORD gives correct index of the last item, and this issue gets solved.
>>
>> Checking if HIWORD is 1 can be additional check. But as we extract correct index, this check can be ignored for allowing selection.
>>
>> Or the other way, we can use HIWORD to avoid selection/deselection when it is 1.
>>
>> Additionally, The behavior on Ubuntu and mac is different,
>>
>> Ubuntu: Last item does not get selected & it also does not select any other item. User must click on the item to select or deselect.
>>
>> Mac: Selects the last item
>>
>> Windows: Focused item gets selected / deselected on multiple clicks, 3-4
>>
>> * After this fix[Allowing selection], Mac and Windows will have same behavior.
>>
>>
>> These are platform UI components. They can behave differently w.r.t things like
>> how they respond to mouse clicks, all dependent on what the platform norms are.
>> I see no reason to go out of our way to "make windows mouse clicks behave like mac".
>>
>>
>> *
>>
>> Issue #2 :
>>
>> This looks a very corner case of list of size more than 65535 size, but the item selection and deselection works fine with keyboard.
>>
>> Also List behaves correctly on Ubuntu and Mac.
>>
>> Just for a look, I am including a fix for this issue in webrev.03: http://cr.openjdk.java.net/~arapte/8198000/webrev.03/ <http://cr.openjdk.java.net/%7Earapte/8198000/webrev.03/>
>>
>> As you have already mentioned we should not really change a long running behavior, but I could not hold on from suggestion.
>>
>> If you think this fix is not needed, I shall just change/remove it.
>>
>>
>> Not understanding you. How can you tell if 0x0001 is supposed to be "1" or "65536" ?
>>
>> I also don't like std::pow .. it is unnecessary here.
>>
>> Not withstanding the lack of clarity on how you are claiming to know that,
>> I was expecting a fix to look more like
>>
>> LONG count = GetCount();
>> if (count > 0) {
>> LONG item = static_cast<LONG>(SendListMessage(LB_ITEMFROMPOINT, 0, msg->lParam));
>> if (HIWORD(item) == 0) {
>> item = LOWORD(item);
>> if (item > 0 && item < GetCount()) {
>> ...
>>
>> So if the user clicks outside we do nothing. What is wrong with that ?
>> -phil.
>>
>>
>>
>> Regards,
>>
>> Ambarish
>>
>> *From:*Philip Race
>> *Sent:* Saturday, November 03, 2018 10:59 PM
>> *To:* Ambarish Rapte <ambarish.rapte at oracle.com> <mailto:ambarish.rapte at oracle.com>
>> *Cc:* awt-dev at openjdk.java.net <mailto:awt-dev at openjdk.java.net>
>> *Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 : java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert on Windows
>>
>> - I am not sure you can assume the LOWORD value is non-negative.
>> It seems to me that the 0XFFFF we got back is meant to be interpreted as "-1"
>> which is what I wrote below.
>> I do note that it appears that in issue #2 you are seeing that up to 65534 items
>> may be allowed and it wraps if you add more than 65535 ..
>> So *perhaps* we can interpret 0xFFFF as meaning positive 65535 but I think
>> *only* if HIWORD is "0".
>>
>> - Issue #1 : I am not sure that a "1" in HIWORD automatically means it is off the
>> end of the list ... just "outside the client area". I think if HIWORD is 1 we just
>> bail don't we ?
>> Then you don't have to worry about whether 0xFFFF meant -1 or 65536
>>
>> - Issue #2 : There is always *some* limit in cases like this. 32767 (2^15-1) or 65535 (2^16-1)
>> are very typical in these platform APIs. Often the platform doesn't explicitly document it
>> and you have to infer it from the data type. I think it was all very moot when these APIs
>> were designed because you'd run out of memory before you could get that many items :-)
>> I'd be surprised if there were not already some open bug pointing out that we accept "int"
>> for index and don't have any feedback when exceeding platform limits.
>> From a compatibility point of view, I don't think it is worth doing anything that would
>> break ancient applications to provide that feedback.
>>
>> -phil.
>>
>>
>>
>> On 11/2/18, 10:23 AM, Ambarish Rapte wrote:
>>
>> Hi Phil,
>>
>> Thanks for guiding with the documentation.
>>
>> The fix is modified after a relook at the documentation, and observed two issues [mentioned below].
>>
>> webrev: http://cr.openjdk.java.net/~arapte/8198000/webrev.02 <http://cr.openjdk.java.net/%7Earapte/8198000/webrev.02>
>>
>> Fix Justification:
>>
>> LB_ITEMFROMPOINT documentation(https://docs.microsoft.com/en-us/windows/desktop/controls/lb-itemfrompoint):
>>
>> 1. Message LB_ITEMFROMPOINT returns a LONG value.
>> 2. There is no mention of LB_ERR as return value, like it is mentioned clearly for some of the other messages as LB_GETCOUNT, LB_SETITEMHEIGHT, LB_GETTEXT, LB_GETCURSEL.
>>
>> * So the existing comparison against LB_ERR is incorrect.
>>
>> 3. The two parts LOWORD and HIWORD of return value are of type WORD(unsigned short), so the return value can never be negative.
>>
>> * It is another reason to not to compare the returned index value with LB_ERR which is defined as (-1).
>>
>> Fix:
>>
>> Extract the index from LOWORD into a WORD variable and verify only if the index is smaller than the list size. (webrev.02)
>>
>> This fixes both JDK-8198000 & below mentioned Issue 1.
>>
>> 2 other Issues:
>>
>> Issue 1: NON selection of an item.
>>
>> 1. Currently the returned LONG value is used without extracting the LOWORD.
>> 2. As far as the HIWORD is 0, the LONG return value would be same as index of item(LOWORD).
>> 3. But when HIWORD is 1, the LONG return value would be a large unexpected value. If it is used as an index, then it would result in NON selection of the item.
>> 4. Test program: http://cr.openjdk.java.net/~arapte/8198000/ListSize3.java <http://cr.openjdk.java.net/%7Earapte/8198000/ListSize3.java>
>> 5. Steps:
>>
>> 1. Compile and execute the test program with release build JDK.
>> 2. Click in the list's client area below the last item, i.e. do not click on any item.
>>
>> 6. Expected behavior: Last item should get selected.
>> 7. Actual behavior: Last item does not get selected on first click. But the focused item gets selected after few clicks.
>>
>> Issue 2: Incorrect selection when list size exceeds sizeof(WORD) [0xFFFF].
>>
>> Steps:
>>
>> 1. Compile and execute the program with release build JDK (with or without the fix) : http://cr.openjdk.java.net/~arapte/8198000/List65544.java <http://cr.openjdk.java.net/%7Earapte/8198000/List65544.java>
>> 2. Click the first item in list, press End key.
>> 3. Click the last item 65544.
>> 4. Expected behavior: The item 65544 should get selected.
>> 5. Actual behavior: The item 65544 does not get selected and instead item 8 gets selected.
>> 6. Verified only windows behavior.
>> 7. I suggest to file a new JBS for this issue.
>>
>> Regards,
>>
>> Ambarish
>>
>> *From:*Phil Race
>> *Sent:* Thursday, November 01, 2018 1:51 AM
>> *To:* Ambarish Rapte <ambarish.rapte at oracle.com> <mailto:ambarish.rapte at oracle.com>; awt-dev at openjdk.java.net <mailto:awt-dev at openjdk.java.net>
>> *Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 : java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert on Windows
>>
>> That adds what I suggested, but I had also suggested you leave in what you
>> had added as it also adds some protection.
>>
>> Additionally I read the MS docs and they do explain the 131071 return value.
>> The message this code is sending is LB_ITEMFROMPOINT and the docs say here
>> https://docs.microsoft.com/en-us/windows/desktop/controls/lb-itemfrompoint that
>>
>> >The return value contains the index of the nearest item in the *LOWORD* <https://msdn.microsoft.com/library/windows/desktop/ms632659>. The *HIWORD* <https://msdn.microsoft.com/library/windows/desktop/ms632657> is zero if the > specified point is in the client area of the list box, or one if it is outside the client area"
>>
>> You got 131071 which is, in hex 0X1FFFF.
>>
>> So you got "1" for hi-word, meaning "outside client area" and "-1" for loword,
>> meaning the index. And a return index of "-1" doubtless means an error ..
>>
>> -phil.
>>
>>
>> On 10/31/18 12:49 PM, Ambarish Rapte wrote:
>>
>> Hi Phil & Sergey,
>>
>> This issue was introduced with the fix for JDK-6806217 <https://bugs.openjdk.java.net/browse/JDK-6806217>, in 7b55, which modified AwtList::HandleEvent(), so It was not observed with JDK6.
>>
>> Please review the updated change as discussed offline: http://cr.openjdk.java.net/~arapte/8198000/webrev.01/ <http://cr.openjdk.java.net/%7Earapte/8198000/webrev.01/>
>>
>> Regards,
>>
>> Ambarish
>>
>> *From:*Phil Race
>> *Sent:* Tuesday, October 30, 2018 2:09 AM
>> *To:* Ambarish Rapte <ambarish.rapte at oracle.com> <mailto:ambarish.rapte at oracle.com>; awt-dev at openjdk.java.net <mailto:awt-dev at openjdk.java.net>
>> *Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 : java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert on Windows
>>
>> On 10/29/18 7:31 AM, Ambarish Rapte wrote:
>>
>> Hi,
>>
>> Please review this Windows only fix,
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8198000
>>
>> Webrev: http://cr.openjdk.java.net/~arapte/8198000/webrev.00/ <http://cr.openjdk.java.net/%7Earapte/8198000/webrev.00/>
>>
>> Issue:
>>
>> 1. Test asserts with debug build jdk, only on windows.
>>
>>
>> Only the debug build turns on asserts.
>> But I think JDK 6 always turned on asserts, and this test was introduced in 6,
>> so something must have changed else we'd have seen this test fail a long time ago.
>> Can you identify what it was ?
>>
>> Also same comment as the other bug - you need to add the bug id to the test.
>>
>>
>>
>>
>> 2.
>> 3. Assert at Line no 77, awt_List.h :: IsItemSelected()
>> 4. awt_List.cpp :: AwtList::HandleEvent() calls IsItemSelected() with an incorrect index value.
>>
>>
>> Why ?
>>
>>
>>
>>
>> 5.
>> 6. In AwtList::HandleEvent() , the call SendListMessage(LB_ITEMFROMPOINT, 0, msg->lParam) returns an arbitrary value 131071, which gets passed to IsItemSelected().
>> 7. Could not find any relevance to the value 131071, from LB_ITEMFROMPOINT doc.
>>
>>
>> That is (128*1024)-1, so it is probably not "arbitrary".
>>
>> Please add the eval above to the bug report .. once we have a complete understanding.
>>
>> -phil.
>>
>>
>>
>>
>> 8.
>>
>> Fix: Index should be verified before making call to IsItemSelected() : http://cr.openjdk.java.net/~arapte/8198000/webrev.00/ <http://cr.openjdk.java.net/%7Earapte/8198000/webrev.00/>
>>
>> Verification: All list tests pass.
>>
>> Regards,
>>
>> Ambarish
>>
>
--
Best regards, Sergey.
More information about the awt-dev
mailing list