Closed Bug 641531 Opened 13 years ago Closed 13 years ago

Close Places containers after use

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: rnewman, Assigned: mak)

References

Details

(Whiteboard: [fixed-in-places])

Attachments

(3 files, 1 obsolete file)

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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch tests fixesSplinter Review
I'll self review this part, it's plain mechanical changes to tests, will assume r+ once I get a green try.
Attachment #527704 - Flags: review?(mak77)
Attached patch sync fixes (obsolete) — Splinter Review
Attachment #527705 - Flags: review?(philipp)
Attached patch browser changeSplinter Review
minor change to browser code
Attachment #527706 - Flags: review?(sdwilsh)
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.
Attached patch sync fixes v1.1Splinter Review
now with more try/finally!
Attachment #527705 - Attachment is obsolete: true
Attachment #527705 - Flags: review?(philipp)
Attachment #527766 - Flags: review?(philipp)
Comment on attachment 527706 [details] [diff] [review]
browser change

r=sdwilsh
Attachment #527706 - Flags: review?(sdwilsh) → review+
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.
Attachment #527704 - Flags: review?(mak77) → review+
Attachment #527766 - Flags: review?(philipp) → review+
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.