Promise for async IPDL might be permanently pending

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hchang, Assigned: hchang)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf-])

Attachments

(4 attachments)

Assignee

Description

2 years ago
We have forcibly rejected pending Promise for async IPDL 
when the channel is closed. However, we do not handle the 
case where the actor is destroyed and removed from the 
protocol tree. 

Besides, in the "handler" side, a Promise will always be 
associated with a GetIPCChannel()->Send() no matter it's
Resolve()'ed or Reject()'ed. This makes the handler layer
unable to properly drop a promise for an already died actor.

So, the solution should consist of two parts:

1) For the sender side, when an actor is destroyed, its
   owning pending Promises have to be reject with, say,
   "ActorDestroyed", reason.

2) For the receiver side, a Promise should be able to 
   be reject with, say "ActorDestroyed", reason to indicate
   not to reply the message.
Assignee

Updated

2 years ago
Assignee: nobody → hchang
Assignee

Updated

2 years ago
Blocks: 1349255
Assignee

Updated

2 years ago
No longer blocks: 1349255
Assignee

Updated

2 years ago
Blocks: 1349255
Nominate this bug to be a qf:p1 bug.
Whiteboard: [qf]
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8868455 - Flags: review?(kchen)
Assignee

Comment 3

2 years ago
Hi Kanru,

I have submitted a patch to do the solution we have come out.
It's been tested accompanied with my patch in bug 1349255 and
the result looks good. Could you help review my changes?

Thanks!
Assignee

Comment 7

2 years ago
A little explanation about the changes to lower.py:

1) On the sender side, while sending async-return IPDL messages, the actor's
   address will be kept track in the PromiseHolder [1]. lower.py is modified to
   extend the callarg to pass 'this' as the third arg. (attachment 8868807 [details])


2) Again, on the sender side, right before ActorDestory() is called in DestroySubTree(),
   we call GetIPCChannel()->RejectPendingPromisesForActor() to reject pending promises.
   lower.py is also modified to add that function invocation. (attachment 8868809 [details])

3) On the receiver/handler side, when the promise is rejected with reason 'ActorDestroyed',
   the reply will be silently cancelled. lower.py is modified to add this "early return".
   (attachment 8868808 [details])

[1] http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/ipc/glue/MessageChannel.h#96

Comment 8

2 years ago
mozreview-review
Comment on attachment 8868455 [details]
Bug 1364857 - Reject pending promises for actor when it's going to be destroyed.

https://reviewboard.mozilla.org/r/140062/#review143750

::: ipc/glue/MessageChannel.cpp:958
(Diff revision 1)
> +  while (itr != mPendingPromises.end()) {
> +    if (itr->second.mActorId != aActorId) {
> +      ++itr;
> +      continue;
> +    }
> +    auto promise = itr->second.mPromise;

|auto& promise| here otherwise this is a unneccessery addRef.

::: ipc/glue/MessageChannel.cpp:964
(Diff revision 1)
> +    mPendingPromises.erase(itr++);
> +    gUnresolvedPromises--;

you can write |itr = mPendingPromises.erase(itr);|
Attachment #8868455 - Flags: review?(kchen) → review+
Thanks for fixing this!

I'm a bit worried about the promise may be resolved or rejected after it had been rejected by the IPC code. Currently the debug build will assert but to make release build safer I think MozPromise::{Resolve,Reject} should be no-op if !IsPending(). And we should also consider to make IsPending() public.
Assignee

Comment 11

2 years ago
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #9)
> Thanks for fixing this!
> 
> I'm a bit worried about the promise may be resolved or rejected after it had
> been rejected by the IPC code. Currently the debug build will assert but to
> make release build safer I think MozPromise::{Resolve,Reject} should be
> no-op if !IsPending(). And we should also consider to make IsPending()
> public.

Thanks for the review :)

Regarding your concern, do you mean my change might lead to 
re-reject or re-resolve? If so, do you refer to the sender side,
receiver side or both?
Flags: needinfo?(kchen)
(In reply to Henry Chang [:hchang] from comment #11)
> (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #9)
> > Thanks for fixing this!
> > 
> > I'm a bit worried about the promise may be resolved or rejected after it had
> > been rejected by the IPC code. Currently the debug build will assert but to
> > make release build safer I think MozPromise::{Resolve,Reject} should be
> > no-op if !IsPending(). And we should also consider to make IsPending()
> > public.
> 
> Thanks for the review :)
> 
> Regarding your concern, do you mean my change might lead to 
> re-reject or re-resolve? If so, do you refer to the sender side,
> receiver side or both?

The receiver side because the promise now can be resolved in two places. One from the DestroySubtree and one from the handler function.
Flags: needinfo?(kchen)
Comment hidden (mozreview-request)

Comment 14

2 years ago
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a0b74ac73ad
Reject pending promises for actor when it's going to be destroyed. r=kanru
It wasn't clear to those doing qf triage today how this was related to performance in the 57 timeframe so we marked it qf-. However, we weren't sure that mattered since you already landed it here :)
Whiteboard: [qf] → [qf-]

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1a0b74ac73ad
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.