714202 - Addon Sync: addons failed: this._changes[0] is undefined in pruneChangesBeforeDate

Status: VERIFIED FIXED

Description

[Jian Zhou]

Created attachment 584887 [details]
error-1325207033137.txt

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:12.0a1) Gecko/20111229 Firefox/12.0a1
Build ID: 20111229031056

Steps to reproduce:

I enabled add-on sync. 


Actual results:

When I updated nightly at Dec 29th, I encountered an syncing error. When I disable add-on sync, no error occur. I have to reset the recovery key to solve it.

Comment 1

[Richard Newman [:rnewman]]

Greg, take a look?

Comment 2

[Gregory Szorc [:gps]]

It appears to be failing in the 2nd line of the following:

    while (this._changes.length > 0) {
      if (this._changes[0][0] >= date) {
        return;
      }

      delete this._changes[0];
    }

The elements of this._changes should always be arrays. This is most weird. The next step is to figure out how an undefined element is being written to that array.

As part of fixing this, I'll have to determine whether it is possible bad data was being serialized to the reconciler's JSON file. If so, I'll need to sanitize data on load so issues in the wild repair themselves.

Jian: can you upload or give a link to a copy of your weave/addonsreconciler.json file from the profile directory? This file may contain personal information. If you don't want the world to see it, you can email it to me at [e-mail].

Comment 3

[Gregory Szorc [:gps]]

Looks like we are using the delete operator incorrectly.

From https://developer.mozilla.org/en/JavaScript/Reference/Operators/delete:

> When you delete an array element, the array length is not affected. For
> example, if you delete a[3], a[4] is still a[4] and a[3] is undefined.

*sigh*

It also looks like there is no explicit test coverage for this. I'll code up a fix.

I don't think undefined is being written to the reconciler's JSON file because JS will error before the mutated array has an opportunity to be written. But, I would still like verification from OP that the saved JSON is sane.

Comment 4

[Gregory Szorc [:gps]]

Created attachment 585010 [details] [diff] [review]
Use Array.filter to produce new array

I switched the implementation to use Array.filter. Includes a new test ensuring that the function behaves properly. The test failed on previous code and passes with this patch.

This patch will eventually need to land in Firefox 11 (currently Aurora) because it is a critical Sync bug.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 585010 [details] [diff] [review]
Use Array.filter to produce new array

Review of attachment 585010 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/tests/unit/test_addons_reconciler.js
@@ +174,5 @@
> +  do_check_eq(young, reconciler._changes[0][0]);
> +  do_check_eq("bar", reconciler._changes[0][2]);
> +
> +  run_next_test();
> +});

Please also add tests for:

* Pruning an empty change array
* Pruning a change array for which no items will be removed
* Pruning a change array for which all items will be removed

Comment 6

[Gregory Szorc [:gps]]

Created attachment 585027 [details] [diff] [review]
Use Array.filter to produce new array, v2

Patch with MOAR tests per review. All tests pass.

Comment 7

[Jian Zhou]

Created attachment 585035 [details]
weave/addonsreconciler.json

This is the addonsreconciler.json in my profile directory. I think there's no particular personal information in it. Hope it will help you to fix the bug.

Comment 8

[Gregory Szorc [:gps]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/9dda4ea46da1

rnewman gave approval in IRC to skip s-c and get this in m-c as quick as possible.

Comment 9

[Gregory Szorc [:gps]]

Jian: I wanted to thank you for an awesome first bug report! It's pretty rare (at least for Sync) for someone's first bug report to be completely diagnosable from the initial comment. Your thoroughness is very much appreciated. Have a great new year.

Comment 10

[Jian Zhou]

It's my pleasure. I'm glad to hear that.

I love FF though it's not perfect. Actually I'm majored in Computer Architecture and I once planed to read the source code of FF, but I feel that it is really difficult to begin with. Anyway, I've made a little contribution to FF now, which makes me very happy. lol

Happy new year!

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9dda4ea46da1

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 585027 [details] [diff] [review]
Use Array.filter to produce new array, v2

[Triage Comment]
Approved for Aurora to fix a major add-on sync issue.

Comment 13

[Gregory Szorc [:gps]]

Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/e9e19f91a325

Comment 14

[Tracy Walker [:tracy]]

I don't see this just enabling add-ons sync on nightly or aurora.  Is there any specific setup or conditions to reproduce this?

Comment 15

[Gregory Szorc [:gps]]

Tracy: this is fixed in Nightly and will not repro there. To repro on Aurora 11.0a2, if you perform enough add-on actions, you should eventually run into this bug.