Opened 9 years ago
Closed 8 years ago
#12799 closed bug (fixed)
Unit test BClipboard::Clear1 crashes
Reported by: | markh | Owned by: | axeld |
---|---|---|---|
Priority: | normal | Milestone: | Unscheduled |
Component: | Kits/Application Kit | Version: | R1/Development |
Keywords: | Cc: | ||
Blocked By: | Blocking: | ||
Platform: | All |
Description
When running the BClipboard::Clear1 test, the UnitTester application crashes. The debug log (attached to the ticket) shows the error to be "state: Call (The clipboard must be locked before proceeding.)".
I checked the original BeBook and it says of the Clear call that it should return B_ERROR when the clipboard is not locked. The code of BClipboard however makes a jump into the debugger at this point. This seems incorrect.
Attachments (3)
Change History (18)
by , 9 years ago
Attachment: | UnitTester-1567-debug-25-05-2016-22-27-58.report added |
---|
comment:1 by , 9 years ago
comment:2 by , 9 years ago
The docs at api.haiku-os.org are "reverse engineered" from the code. They reflect the current behavior, not always the expected one.
An analysis of the commit log for BClipboard sheds some light (maybe):
- In http://cgit.haiku-os.org/haiku/commit/src/kits/app/Clipboard.cpp?id=b010ca67201c6b96d2136d4feca376bb9afe9a57 , the code for Clear() calls AssertLocked which at the time was the same as IsLocked(),
- In http://cgit.haiku-os.org/haiku/commit/src/kits/app/Clipboard.cpp?id=76441bab6ebb9132d0bb0ab623087bd2b9e861e0 , AssertLocked() was changed to call debugger(), but Clear() was not adjusted to call IsLocked() instead.
The fix is simply adjusting Clear() to use IsLocked(), and adjusting the docs (they are also in the source tree in docs/user/) to match the correct (BeOS-compatible) behavior.
comment:3 by , 9 years ago
Component: | Build System → Kits/Application Kit |
---|---|
Owner: | changed from | to
comment:4 by , 9 years ago
The _AssertLocked function is a private function that is only called by the functions that want to know if it is locked or not. There does not appear to be a good reason to call the debugger in _AssertLocked.
I would propose either returning the functionality of that procedure to the first revision you mentioned (simply return IsLocked) or eliminating the function altogether as it seems a bit pointless.
Regarding the return value of Clear (and Commit and Revert), the BeBook status that it should return B_ERROR if the object is not locked and B_OK otherwise. Even the old code did not seem to do that. It depended on what the MakeEmpty call would do. I'm not sure what BeOS actually does here. I would think we should follow the BeBook here and always return B_OK if the object is locked.
by , 9 years ago
Attachment: | 0001-Fix-12799-Make-BClipboard-compatible-with-BeOS-again.patch added |
---|
comment:5 by , 9 years ago
patch: | 0 → 1 |
---|
comment:6 by , 9 years ago
Added a patch that implements the functions as described in the BeBook. However, even on BeOS R5 this does not work as described and crashes with the details showing that the "clipboard must be locked before proceeding".
comment:7 by , 9 years ago
Other option is to keep it as what actually happens on BeOS/Haiku and get the tests and documentation fixed. The commit http://cgit.haiku-os.org/haiku/commit/src/tests/kits/app/bclipboard?id=9ed278d6645a43d2a2d54ec57cff815299349e32 mentions that cppunit supports catching debugger invocations, but that does not seem to work as far as I can tell. This seems to be the reason the current unit tests are failing.
comment:8 by , 9 years ago
Ok, if it did the same on BeOS (hit the debugger call), we probably wan tto keep that and the _AssertLocked. It would be nice to note in a comment that this was checked against BeOS and that the Be Book is not entirely correct.
It's possible to enable and disable the debugger with enable_debugger()
/ disable_debugger()
. If the debugger is disabled, the return values will probably match the Be Book, but we'd have to check to make sure.
I don't know how cppunit is configured to catch debugger calls, if that doesn't work it should be fixed. disable_debugger could work but then I don't know how it could check that the debugger function was actually called.
comment:9 by , 9 years ago
I did some digging into the source and found what Ingo was referring to. He added elf symbol patching support for catching debugger calls, but it was only enabled if building from BeOS x86. I changed the Jamfile to also enable it from Haiku, but had to move one include from a cpp file to a header file. If this can be avoided that would be better, but I did not see how I could get that to work.
With these changes, the BClipboard tests pass again as is. There is still the problem that the documentation is incorrect and the code implies that B_NOT_ALLOWED will be returned if the clipboard is not locked, but it may be better to patch that separately.
comment:10 by , 9 years ago
Is there something that needs to be done before this patch can be committed?
comment:12 by , 9 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Had to revert the change, it was failing on the buildbots (e.g. http://buildbot.haiku-os.org/builders/haiku-master-x86/builds/2767).
by , 9 years ago
Attachment: | 0001-Fix-12799-Enable-elf-symbol-patching-for-Haiku.patch added |
---|
comment:13 by , 9 years ago
I updated the patch. I accidentally broke all non Haiku builds. This should fix it, but I don't have any non Haiku build machines to test it on.
Looked up the documentation on the Clear function in the new HaikuBook (https://api.haiku-os.org/classBClipboard.html#ab761d1164ce410618671312d2f9d6e70), which says something different again: B_NOT_ALLOWED: The clipboard is not locked.
Are we deviating on this level from BeOS?