There was concern in bug 755375 that the reconciliation algorithm for the storage layer (documented line 251 onwards in storage.js) was a little too aggressive and/or dangerous. A thorough re-review is required to ensure correctness. The module has tests, but the method in general may have edge cases that are either untestable or we don't test correctly yet.

One problem in the current implementation that's known is that if I have installed apps on a build with mozapps API support without AITC support, but update to a build with AITC support and logged into myapps, my local apps will all disappear.

https://mxr.mozilla.org/mozilla-central/source/services/aitc/modules/storage.js#251 Please review and raise issues!

I started with the review and found a number of issues. It was going to take a while to document everything and the new code would be pretty much rewritten anyway, so I just rewrote it. mconnor gets code review. anant: please ensure I didn't change behavior at all with the patch. It might help to look at just the new code instead of trying to parse the diff. With this patch, I think I fixed a few invalid test cases. I also added a new test case. I also addressed issues where the callback could be invoked multiple times.

Forgot to qref

Comment on attachment 638536 [details] [diff] [review] Rewrite reconcile logic, v2 The general algorithm looks good to me.

Comment on attachment 638536 [details] [diff] [review] Rewrite reconcile logic, v2 Algorithm looks good. There are some concerns that are more long term: * app.origin may at some point not be guaranteed to be unique. * this is largely written with Set in mind, but had to be modified, so that's a weird artifact * the split of the function is sort of arbitrary, but isn't terrible since this isn't called multiple times

Is there anything I can do to verify this from an end-user perspective?

(In reply to Jason Smith [:jsmith] from comment #8) > Is there anything I can do to verify this from an end-user perspective? Nope.