Per mak: > > OT: I see the node.containerOpen = true, but where is the > > corresponding call to node.containerOpen = false? > > As far as I can see, there are no such calls in the Sync codebase. File a bug in Sync please, any container opened that is not needed for caching purposes (so unless you use it to track changes) should be explicitly closed. Not doing so causes the result's observers to stay alive, till it is cycle collected, that could be much later, these observers can regress global performances.

there are other entries I'd like to fix around too. So taking.

I'll self review this part, it's plain mechanical changes to tests, will assume r+ once I get a green try.

minor change to browser code

Comment on attachment 527705 [details] [diff] [review] sync fixes Looks good, tests pass... but please wrap code in try .. finally. E.g., // Remember all the children GUIDs and recursively get more try { for (let i = 0; i < node.childCount; i++) { let child = node.getChild(i); items[this.GUIDForId(child.itemId)] = true; this._getChildren(child, items); } } finally { node.containerOpen = false; } We've already seen that apparently simple operations like GUID fetching can throw, which means the cleanup code isn't run.

now with more try/finally!

Comment on attachment 527706 [details] [diff] [review] browser change r=sdwilsh

Comment on attachment 527704 [details] [diff] [review] tests fixes try was fine, the only failure was due to other stuff, so r+ on mechanical changes, will land first on Places btw.

http://hg.mozilla.org/projects/places/rev/c87e47e6ca8c http://hg.mozilla.org/projects/places/rev/7bc382705bc7 http://hg.mozilla.org/projects/places/rev/80793bce352a

http://hg.mozilla.org/mozilla-central/rev/c87e47e6ca8c http://hg.mozilla.org/mozilla-central/rev/7bc382705bc7 http://hg.mozilla.org/mozilla-central/rev/80793bce352a