589017 - TBPL not sorting pushlog JSON output

Status: VERIFIED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Robert Helmer [:rhelmer]]

Attachment: pushlog JSON

Since TBPL bug 584836 landed and added native JSON parsing for pushlog, the order of pushes is sometimes wrong (example JSON exhibiting the problem attached).

Looks ok in Chrome/Firefox 3.6 but not latest Minefield, sicking suspects a possible JSON parser bug.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: minimal json for testcase

Comment 2

[Robert Helmer [:rhelmer]]

Attachment: testcase

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to comment #0)
> Looks ok in Chrome/Firefox 3.6 but not latest Minefield, sicking suspects a
> possible JSON parser bug.

This is wrong; it's easier to see in the testcase but Minefield (m-c) and Firefox 3.6 behave the same. Chrome's JSON parser is sorting the keys, and TBPL is apparently depending on this behavior.

Comment 4

[Robert Helmer [:rhelmer]]

Attachment: pushlog JSON is not sorted by key; sort in JS

Not sure if this should be fixed on the pushlog side, but here is a script to do it in JS.

Anyone know a prettier way to make sure the sort happens numerically?

Comment 5

[Robert Helmer [:rhelmer]]

Attachment: testcase

Comment 6

[Robert Helmer [:rhelmer]]

(In reply to comment #3)
> (In reply to comment #0)
> > Looks ok in Chrome/Firefox 3.6 but not latest Minefield, sicking suspects a
> > possible JSON parser bug.
> 
> This is wrong; it's easier to see in the testcase but Minefield (m-c) and
> Firefox 3.6 behave the same. Chrome's JSON parser is sorting the keys, and TBPL
> is apparently depending on this behavior.

Just to clarify, the behavior I'm seeing in Chrome is likely a bug in Chrome; that's what the testcase attachments are about. Firefox's JSON parser looks like it's doing the right thing.

This obscured the problem at first, so it looked like a Firefox and not a TBPL and/or pushlog JSON bug.

Comment 7

[Robert Helmer [:rhelmer]]

(In reply to comment #6)
> Just to clarify, the behavior I'm seeing in Chrome is likely a bug in Chrome;
> that's what the testcase attachments are about. Firefox's JSON parser looks
> like it's doing the right thing.

There are several bugs in the chromium tracker for this, here is one with recent activity:
http://code.google.com/p/v8/issues/detail?id=164

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 467635 [details] [diff] [review]
pushlog JSON is not sorted by key; sort in JS

>diff -r c2a561d9a397 js/PushlogJSONParser.js
>--- a/js/PushlogJSONParser.js	Thu Aug 19 15:51:36 2010 +0200
>+++ b/js/PushlogJSONParser.js	Thu Aug 19 19:17:09 2010 -0700
>@@ -4,8 +4,12 @@
>     var self = this;
>     $.getJSON(this._getLogUrl(repoName, timeOffset), function(data) {
>       var pushes = [];
>-      for (var i in data) {
>-        var push = data[i];
>+      var sorted = [];
>+      for (k in data) sorted.push(k);
>+      sorted.sort(function(a,b){ return a-b });
>+      for (var i = 0; i < sorted.length; i++) {
>+        var key = sorted[i];
>+        var push = data[key];
>         var patches = [];
>         for (var i in push.changesets) {
>           var patch = push.changesets[i];

Huh, we were using "i" as loop variable both in the outer and in the inner loop? Why did that work?
I've pushed your patch with some changes:
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/fc73f6ae02b7

(In reply to comment #4)
> Anyone know a prettier way to make sure the sort happens numerically?

Ensure that the items in the array are numbers. You can convert them by prefixing them with a plus.

Comment 9

[Robert Helmer [:rhelmer]]

(In reply to comment #8)
> I've pushed your patch with some changes:
> http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/fc73f6ae02b7
> 
> (In reply to comment #4)
> > Anyone know a prettier way to make sure the sort happens numerically?
> 
> Ensure that the items in the array are numbers. You can convert them by
> prefixing them with a plus.

Hmm I don't think that's enough; I tried in my patch with parseInt() around the strings, but still got lexical sort. Consider the output of:

"""
  unsorted = [5,14,13,1,2];
  sorted = [];
  for (var i = 0; i < unsorted.length; i++ ) {
    n = unsorted[i];
    sorted.push(+n);
  }
  alert(sorted.sort());
  alert(sorted.sort(function(a,b){ return a-b }));
"""

First alert:
1,13,14,2,5

Second alert:
1,2,5,13,14

Comment 10

[Markus Stange [:mstange]]

Hmm, you're right, thanks! I'll push the fix.

Comment 11

[Markus Stange [:mstange]]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/pushloghtml

Comment 12

[Markus Stange [:mstange]]

Uhm, that was supposed to be
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/17ce643cf5aa

Comment 13

[Robert Helmer [:rhelmer]]

(In reply to comment #12)
> Uhm, that was supposed to be
> http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/17ce643cf5aa

Cool, WFM locally now. I think it would have only have bitten when the number went up to the next power of 10, which would be super confusing and hard to reproduce.