代码改变世界

Sys.StringBuilder里的Bug

2007-02-13 02:35 Jeffrey Zhao 阅读(...) 评论(...) 编辑 收藏

他们肯定没有做好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。