Transaction boundary problems produce inapporpriate commits

CRM_Core_Transaction defines an API for easily managing transactions, and the API is consumed in many places. A quick grep finds several dozen references. I believe the following code represents a typical usage:

<?php
01: function foo() {
02: require_once 'CRM/Core/Transaction.php';
03: $transaction = new CRM_Core_Transaction( );
04: ...
05: if (...error...)

{ 06: $transaction->rollback(); 07: return; 08: }

09: ...
10: $transaction->commit();
11: }
12:
13: function bar() {
14: require_once 'CRM/Core/Transaction.php';
15: $transaction = new CRM_Core_Transaction( );
16: foo();
17: if (...error...)

{ 18: $transaction->rollback(); 19: return; 20: }

21: ...bar...
22: $transaction->commit();
23: }
24:
25: bar();
?>

The great thing about this is that neither foo() nor bar() needs a complete picture of how many business functions are involved with the transaction. The CRM_Core_Transaction implementation will ensure that COMMIT or ROLLBACK is only called once, when the outermost $transaction terminates.

Unfortunately, there seem to be two major bugs in the implementation of this API:

1. Double-pseudo-commit: Suppose one invokes bar() which invokes foo(). Execution proceeds to line 10. The transaction count is 2, then commit() runs, so the count decrements to 1.

Now, foo() terminates, and local variables are deallocated, so $transaction is destroyed. CRM_Core_Transaction::__destruct invokes commit() a second time. The transaction count decrements to 0, and a SQL COMMIT is issued.

Flow-control returns to bar(), but now $transaction is useless. It's impossible for bar() to either rollback or commit. Instead, bar() is effectively using AUTO-COMMIT mode.

2. Exception rollback: Suppose an exception occurs while processing line 4. Control passes to CRM_Core_Error. Depending on which function processes the exception condition (''handle'' or ''fatal''), we might make an invalid call to the non-static function CRM_Core_Transaction::rollback(). This call does not trigger a rollback. Regardless, exit() is eventually called without issuing either a SQL COMMIT or SQL ROLLBACK, and the final transaction status is indeterminate. In my testing, this effectively commits.

I believe the attached patch fixes both issues, but I haven't tested it much. It hasn't broken anything for me... yet... on 2.2.8. I haven't tried it with any other releases, but a quick look at some code on 3.1 suggests that the same behavior will occur there.

A quick note on the patch... I tried to write it in a way that would work with API consumers which explicitly invoke commit(), and also work with API consumers which neglect to invoke commit(). In either case, $_count will increment exactly once for each instance of CRM_Core_Transaction, and $_count will decrement exactly once for each instance.