Closed Bug 855556 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
blocking-b2g tef+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: reuben, Assigned: reuben)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(3 files, 3 obsolete files)

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.
Depends on: 855015
Attachment #731050 - Attachment is obsolete: true
Attachment #736724 - Flags: review?(anygregor)
Hm the DB code shouldn't care about child processes. Can't we do this in the ContactService where we actually manage the IPC?
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.
No longer depends on: 855015
Attachment #736724 - Attachment is obsolete: true
Attachment #736724 - Flags: review?(anygregor)
Attachment #740479 - Flags: review?(anygregor)
Attachment #740480 - Flags: review?(anygregor)
Attachment #740479 - Flags: review?(anygregor) → review+
(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)
(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.
Attachment #740480 - Attachment is obsolete: true
Attachment #740480 - Flags: review?(anygregor)
Attachment #740762 - Flags: review?(anygregor)
Attachment #740762 - Flags: review?(anygregor) → review+
Contacts is using getAll in v1_0_1, so we should land this fix there as well.
blocking-b2g: --- → tef?
https://hg.mozilla.org/mozilla-central/rev/2bd159c2257d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
blocking-b2g: tef? → tef+
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.
Attachment #742577 - Flags: review?(anygregor)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #742577 - Flags: review?(anygregor) → review+
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
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.