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

VERIFIED FIXED in Firefox 11

Status

()

VERIFIED FIXED
7 years ago
29 days ago

People

(Reporter: i, Assigned: gps)

Tracking

unspecified
mozilla12
Points:
---

Firefox Tracking Flags

(firefox11 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Severity: normal → blocker
OS: All → Windows 7
Priority: -- → P1
Hardware: All → x86_64
Greg, take a look?
Assignee: nobody → gps
Severity: blocker → normal
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: P1 → --
Summary: sync error when add-on sync enabled → Addon Sync: addons failed: this._changes[0] is undefined in pruneChangesBeforeDate
(Assignee)

Comment 2

7 years ago
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 gps@mozilla.com.
(Assignee)

Comment 3

7 years ago
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.
(Assignee)

Comment 4

7 years ago
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.
Attachment #585010 - Flags: review?(rnewman)
(Assignee)

Updated

7 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
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
Attachment #585010 - Flags: review?(rnewman)
Attachment #585010 - Flags: review+
Attachment #585010 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 6

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

Patch with MOAR tests per review. All tests pass.
Attachment #585010 - Attachment is obsolete: true
Attachment #585010 - Flags: approval-mozilla-aurora?
Attachment #585027 - Flags: review?(rnewman)
Attachment #585027 - Flags: review?(rnewman)
Attachment #585027 - Flags: review+
Attachment #585027 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 7

7 years ago
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.
(Assignee)

Comment 8

7 years ago
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.
(Assignee)

Comment 9

7 years ago
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.
(Reporter)

Comment 10

7 years ago
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!
https://hg.mozilla.org/mozilla-central/rev/9dda4ea46da1
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
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.
Attachment #585027 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox11: --- → fixed
I don't see this just enabling add-ons sync on nightly or aurora.  Is there any specific setup or conditions to reproduce this?
(Assignee)

Comment 15

7 years ago
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.
Whiteboard: [qa+]
Whiteboard: [qa+]

Updated

7 years ago
Status: RESOLVED → VERIFIED
status-firefox11: fixed → verified
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.