855556 - Don't try to send contacts if the child process was killed

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Reuben Morais [:reuben]]

Right now if a getAll call was made and the child process dies, we keep trying to send contacts. We should cancel any ongoing calls to getAll if we see that the child process gets killed.

Comment 1

[Reuben Morais [:reuben]]

Attachment: Stop sending contacts if the child process is dead

Comment 2

[Reuben Morais [:reuben]]

Attachment: Stop sending contacts if the child process is dead

Comment 3

[Gregor Wagner [:gwagner]]

Hm the DB code shouldn't care about child processes. Can't we do this in the ContactService where we actually manage the IPC?

Comment 4

[Reuben Morais [:reuben]]

It needs some interaction from the DB because it has to know when to stop calling the callback. I tried to make it as loosely coupled as I could by simply catching the exception.

I can move the loop into ContactService, but that would also move some DB logic to the service, and make it harder to understand the program flow when reading the code.

Comment 5

[Reuben Morais [:reuben]]

Attachment: Clean up this .bind(this) mess.

Comment 6

[Reuben Morais [:reuben]]

Attachment: 2 - Stop sending contacts if the child process is dead

Comment 7

[Gregor Wagner [:gwagner]]

(In reply to Reuben Morais [:reuben] from comment #4)
> It needs some interaction from the DB because it has to know when to stop
> calling the callback. I tried to make it as loosely coupled as I could by
> simply catching the exception.
> 
> I can move the loop into ContactService, but that would also move some DB
> logic to the service, and make it harder to understand the program flow when
> reading the code.

Why do you have to move the loop? You just wrap "mm.sendAsyncMessage" in a try block and call something like db.stopSending(cursorID)

Comment 8

[Reuben Morais [:reuben]]

Attachment: 2 - Stop sending contacts if the child process is dead

(In reply to Gregor Wagner [:gwagner] from comment #7)
> (In reply to Reuben Morais [:reuben] from comment #4)
> > It needs some interaction from the DB because it has to know when to stop
> > calling the callback. I tried to make it as loosely coupled as I could by
> > simply catching the exception.
> > 
> > I can move the loop into ContactService, but that would also move some DB
> > logic to the service, and make it harder to understand the program flow when
> > reading the code.
> 
> Why do you have to move the loop? You just wrap "mm.sendAsyncMessage" in a
> try block and call something like db.stopSending(cursorID)

stopSending can't stop the already running function, so it gets messy for stuff like this:

aCallback(chunk);
aCallback(null);

So here I wrap the sendAsyncMessage call in a try-catch, but I also throw the exception. That way, if IPC changes and we have to detect the child process being dead with a different method, only ContactService will need to be changed.

Comment 9

[Reuben Morais [:reuben]]

Contacts is using getAll in v1_0_1, so we should land this fix there as well.

Comment 10

[Reuben Morais [:reuben]]

https://hg.mozilla.org/projects/birch/rev/2bd159c2257d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bd159c2257d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8434f8bd54bf
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8def3040033b

Comment 13

[Reuben Morais [:reuben]]

Attachment: 3 - Test the cached branch of getAll more extensively

We just caught a bug in ContactDispatcher that is not very visible when running tests, so this patch tests that part of getAll more extensively.

Comment 14

[Reuben Morais [:reuben]]

https://hg.mozilla.org/projects/birch/rev/6e30c4246140

Comment 15

[Reuben Morais [:reuben]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/8434f8bd54bf

https://hg.mozilla.org/releases/mozilla-b2g18/rev/c3e641054923

Comment 16

[Reuben Morais [:reuben]]

Also, I guess you already backed out v1_0_1:

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/296c4c249525

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Re-landed.
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/caf98b429cf7

Comment 18

[Reuben Morais [:reuben]]

Landed the third patch on the branches:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/83f0da64373e
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/29dbdb5798bf

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e30c4246140