Sys.StringBuilder里的Bug

他们肯定没有做好Code Review。

Sys.StringBuilder类源码

话不多说，首先我把Sys.StringBuilder类的源代码完整的贴出，然后根据代码来寻找问题：

Sys.StringBuilder = function Sys$StringBuilder(initialText) {
    /// <param name="initialText" optional="true" mayBeNull="true"></param>
    var e = Function._validateParams(arguments, [
        {name: "initialText", mayBeNull: true, optional: true}
    ]);
    if (e) throw e;

    this._parts = 
        (typeof(initialText) !== 'undefined' && initialText !== null && initialText !== '') ?
        [initialText.toString()] : [];
    this._value = {};
    this._len = 0;
}

    function Sys$StringBuilder$append(text) {
        /// <param name="text" mayBeNull="true"></param>
        var e = Function._validateParams(arguments, [
            {name: "text", mayBeNull: true}
        ]);
        if (e) throw e;

        this._parts[this._parts.length] = text;
    }

    function Sys$StringBuilder$appendLine(text) {
        /// <param name="text" optional="true" mayBeNull="true"></param>
        var e = Function._validateParams(arguments, [
            {name: "text", mayBeNull: true, optional: true}
        ]);
        if (e) throw e;

        this._parts[this._parts.length] =
            ((typeof(text) === 'undefined') || (text === null) || (text === '')) ?
            '\r\n' : text + '\r\n';
    }

    function Sys$StringBuilder$clear() {
        if (arguments.length !== 0) throw Error.parameterCount();
        this._parts = [];
        this._value = {};
        this._len = 0;
    }

    function Sys$StringBuilder$isEmpty() {
        /// <returns type="Boolean"></returns>
        if (arguments.length !== 0) throw Error.parameterCount();
        if (this._parts.length === 0) return true;
        return this.toString() === '';
    }

    function Sys$StringBuilder$toString(separator) {
        /// <param name="separator" type="String" optional="true" mayBeNull="true"></param>
        /// <returns type="String"></returns>
        var e = Function._validateParams(arguments, [
            {name: "separator", type: String, mayBeNull: true, optional: true}
        ]);
        if (e) throw e;

        separator = separator || '';

        var parts = this._parts;
        if (this._len !== parts.length) {
            this._value = {};
            this._len = parts.length;
        }

        var val = this._value;
        if (typeof(val[separator]) === 'undefined') {
            // Need to remove empty elements before joining in the separator case
            if (separator !== '') {
                for (var i = 0; i < parts.length;) {
                    if ((typeof(parts[i]) === 'undefined') || 
                        (parts[i] === '') || (parts[i] === null)) {
                        parts.splice(i, 1);
                    }
                    else {
                        i++;
                    }
                }
            }
            val[separator] = this._parts.join(separator);
        }
        return val[separator];
    }

Sys.StringBuilder.prototype = {
    append: Sys$StringBuilder$append,

    appendLine: Sys$StringBuilder$appendLine,

    clear: Sys$StringBuilder$clear,

    isEmpty: Sys$StringBuilder$isEmpty,

    // separator may be null, to match behavior of ECMA Array.join(separator) and
    // .NET String.Join(separator, value)
    toString: Sys$StringBuilder$toString
}
Sys.StringBuilder.registerClass('Sys.StringBuilder');

 

Sys.StringBuilder类的Bug分析与重现

Sys.StringBuilder方法的实现很简单，只是把每一次append的内容添加到_parts数组里，在toString时调用数组的join方法来生成一个新的字符串，以次避免过多字符串拼接造成的性能损失。其中Bug的关键就在于它的toString方法，我们自己来分析一下它的实现。

toString方法的作用是把separater作为分隔符，连接StringBuilder内已经添加的部分。它的作用和.NET Framework里String.Join(string separator, string[] value)的功能非常相似。首先，如果用户没有提供separater，则默认separator为空字符串。然后判断_len与_parts数组的长度是否相同，如果不同，则表示_parts被修改过了，于是将作为缓存的_value字典清空并保留新的_len值。如果separator不为空字符串，则遍历_parts数组，清除其中所有为空的元素。这个操作不会影响结果，但是能够提高join方法的性能。最后将调用_parts数组的join方法生成一个新的字符串，并以separator作为key保存在_value数组中。这样，如果Sys.StringBuilder没有改变的话，使用相同的separator调用toString方法则会直接从_value字典的缓存中获取，而避免了再次调用join方法造成的性能损失。

这只是我对于这段实现的“意图”的一个理解，因为这段实现有着非常严重的Bug。大家有没有觉得这段代码有些奇怪？如果_len与_parts数组的长度不同时，则会认为_parts数组被改变了（这“似乎”没错）。但是在重新设定了_len的数值之后，接下来的代码又可能修改了_parts数组（用于清空其中的空元素），这样即使Sys.StringBuilder的内容不被外界改变时，_parts数组的长度也变化了，这造成了_len的数值与_parts.length不同，缓存失效，又必须调用join方法了——没错，甚至会重新遍历_parts数组。

下面的代码就直接说明了问题：

var sb = new Sys.StringBuilder();
for (var i = 0; i < 5; i++)
{
	sb.append(i);
	sb.append(null);
}

alert(sb.toString('|'));
alert(sb.toString('|'));

 

首先，我们向StringBuilder中添加部分空元素，然后连续调用两次toString方法，传入同样的separator。如果跟踪一下代码就会发现，第二次调用toString方法时并没有利用到缓存，而且重新遍历了_parts数组。虽然生成了相同的结果，但是与它设计的本意违背了，它的缓存“无缘无故”地失效了。

如果说，上面的问题只能说明这个Bug使Sys.StringBuilder没有符合Sys.StringBuilder的设计初衷，那么我下面构造的一个场景就表明这个Bug的严重性了，它能够使Sys.StringBuilder的使用得到错误的结果。如下：

var sb = new Sys.StringBuilder();
for (var i = 0; i < 5; i++)
{
	sb.append(i);
}

sb.append(null);
alert(sb.toString('|')); // "0|1|2|3|4"

sb.append('abc');
alert(sb.toString('|')); // 依旧是"0|1|2|3|4"？

 

很明显，两次alert得到的结果都是"0|1|2|3|4"完全是错误的。我只是简单的利用了toString方法中判断_parts数组有没有被更新的错误方法——怎么可以使用_len记录_parts数组长度的同时又减少了_parts数组的元素呢？在上面的代码中，我向StringBuilder里添加了一个将会被去掉的null元素，然后在调用toString方法之后重新append一个新的元素以“补充”_parts数组。“愚蠢”的StringBuilder这下认为Sys.StringBuilder没有任何改变，从_values字典中读取了被缓存的结果，这自然是错误的。

 

RC版本中的StringBuilder居然是正确的

RC版本中的Sys.StringBuilder类的实现居然是正确的，这不是天方夜谭，我们来看一下StringBuilder在RC中的做法：

function Sys$StringBuilder$append(text) {
    /// <param name="text" mayBeNull="true"></param>
    var e = Function._validateParams(arguments, [
        {name: "text", mayBeNull: true}
    ]);
    if (e) throw e;

    if (typeof(text) !== 'undefined' && text !== null && text !== '') {
        this._parts[this._parts.length] = text.toString();
        this._value = {};
    }
}

function Sys$StringBuilder$toString(separator) {
    /// <param name="separator" type="String" optional="true" mayBeNull="true"></param>
    /// <returns type="String"></returns>
    var e = Function._validateParams(arguments, [
        {name: "separator", type: String, mayBeNull: true, optional: true}             
    ]);
    if (e) throw e;

    separator = separator || '';

    if (typeof(this._value[separator]) === 'undefined')
        this._value[separator] = this._parts.join(separator);

    return this._value[separator];
}

 

多简单优雅的实现，每当调用append方法时，将会直接判断text的有效性（而不是在调用toString方法时去除无效元素），在确认有效之后则添加至_parts数组中，然后将_value字典，也就是把缓存清空。在调用toString方法时，则不需要去除parts数组中的无效元素，也可以非常合理的利用缓存。可以看出，在RC中，并没有_len的作用。

那么为什么在RTM时Sys.StringBuilder反而被修改了呢？在这里，我指的是为什么去“修改”，而不是为什么“改错了”。我认为，他们只是为了避免在调用append方法时将_value字典设为新的对象吧。按照RC中的实现，如果调用了1000次append方法，则会生成1000个多余的对象，这的确是种浪费。但是，他们只要这样改不就可以了吗？如下：

function Sys$StringBuilder$append(text) {
    if (typeof(text) !== 'undefined' && text !== null && text !== '') {
        this._parts[this._parts.length] = text.toString();
        this._value = null;
    }
}

function Sys$StringBuilder$toString(separator) {
    separator = separator || '';

    if (!this._value) this._value = {};

    if (typeof(this._value[separator]) === 'undefined')
        this._value[separator] = this._parts.join(separator);

    return this._value[separator];
}

 

还好，现在的Sys.StringBuilder的Bug并不是那么容易遇到的，在整个Microsoft AJAX Library的代码中还没有遇到过提供separator的toString方法调用。不过这一点并不能掩盖这个错误，如果您需要使用Sys.StringBuilder的话，也请注意一下这个Bug。

总之，在Sys.StringBuilder问题上，他们一定没有做好Code Review。