Opened 3 years ago

Closed 3 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:
Has a Patch: yes 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.

Change History (18)

comment:1 by markh, 3 years ago

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?

comment:2 by pulkomandy, 3 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):

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 bonefish, 3 years ago

Component: Build SystemKits/Application Kit
Owner: changed from bonefish to axeld

comment:4 by markh, 3 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.

comment:5 by markh, 3 years ago

Has a Patch: set

comment:6 by markh, 3 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 markh, 3 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 pulkomandy, 3 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 markh, 3 years ago

I did some digging into the source and found what Info 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.

Version 0, edited 3 years ago by markh (next)

comment:10 by markh, 3 years ago

Is there something that needs to be done before this patch can be committed?

comment:11 by waddlesplash, 3 years ago

Resolution: fixed
Status: newclosed

Applied in hrev50361. Thanks!

comment:12 by waddlesplash, 3 years ago

Resolution: fixed
Status: closedreopened

Had to revert the change, it was failing on the buildbots (e.g. http://buildbot.haiku-os.org/builders/haiku-master-x86/builds/2767).

comment:13 by markh, 3 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.

comment:14 by waddlesplash, 3 years ago

I don't either -- can someone else test?

comment:15 by korli, 3 years ago

Resolution: fixed
Status: reopenedclosed

Applied in hrev50632.

Note: See TracTickets for help on using tickets.