871872 - Clean up ContactDB temporary cache if the child dies without asking for more contacts.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Reuben Morais [:reuben]]

Right now if the child receives a chunk and doesn't use it or call continue() again, we keep the dispatcher around in the parent. We should clean that up when the child dies.

Comment 1

[Reuben Morais [:reuben]]

Attachment: Clean up existing cursors on shutdown

So because we moved to using DOMRequestIpcHelper for cursors in bug 862351, we don't have a list of cursors in the child anymore, and that's the reason for the DOMRequestHelper.jsm changes here. I could also keep that list in ContactManager, if that's preferred.

Comment 2

[Reuben Morais [:reuben]]

Looking green on Try, FWIW: https://tbpl.mozilla.org/?tree=Try&rev=582dedf62c9e

Comment 3

[Gregor Wagner [:gwagner]]

Hm no we should just fix this in the case "child-process-shutdown" call.
Calling cleanup from the child when the child just crashed doesn't work :)

Comment 4

[Reuben Morais [:reuben]]

https://hg.mozilla.org/try/rev/c587cfc2e1b3

Comment 5

[Reuben Morais [:reuben]]

Attachment: Clean up existing cursors on shutdown

Tested manually on the device, since we have leakcheck disabled in B2G tests.

Comment 6

[Gregor Wagner [:gwagner]]

I would prefer if we don't have to queue up all cursorIDs until we shut down the app.
How about moving the successCallback of sendNow into ContactsService and do a proper cursorID deletion when we are done with a cursor?

Comment 7

[Reuben Morais [:reuben]]

Attachment: Clean up existing cursors on child process shutdown

As discussed on IRC.

Comment 8

[Gregor Wagner [:gwagner]]

Comment on attachment 751925 [details] [diff] [review]
Clean up existing cursors on child process shutdown

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

::: dom/contacts/fallback/ContactService.jsm
@@ +111,5 @@
>              try {
>                mm.sendAsyncMessage("Contacts:GetAll:Next", {cursorId: msg.cursorId, contacts: aContacts});
> +              if (aContacts === null) {
> +                let index = this._cursors[mm].indexOf(msg.cursorId);
> +                this._cursors[mm].splice(msg.cursorId, 1);

that doesn't work.

Comment 9

[Reuben Morais [:reuben]]

Sigh, pretend that's splice(index, 1), I attached the patch from the wrong tree.

Comment 10

[Gregor Wagner [:gwagner]]

Comment on attachment 751925 [details] [diff] [review]
Clean up existing cursors on child process shutdown

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +693,5 @@
>      }
>    },
>  
> +  clearDispatcher: function CDB_clearDispatcher(aCursorId) {
> +    debug("clearDispatcher: " + aCursorId);

if (DEBUG) ....

::: dom/contacts/tests/test_contacts_getall.html
@@ +444,5 @@
> +  } else {
> +    ok(true, "All done!\n");
> +    SimpleTest.finish();
> +  }
> +}

Please don't change this.

Comment 11

[Reuben Morais [:reuben]]

https://hg.mozilla.org/projects/birch/rev/a6f2feef48a5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6f2feef48a5