Pass a Resolve function instead of a MozPromise to async return methods

RESOLVED FIXED in Firefox 55

Status

()

Core
IPC
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: kanru, Assigned: kanru)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 months ago
The rationale behind this are:

1. We already don't want the user to call Reject
2. It is easier to control the flow if the Resolve method immediately send the results back. The IPC message is already async so there is no point to the added delay of MozPromise.
(Assignee)

Comment 1

5 months ago
Created attachment 8871697 [details] [diff] [review]
0001-Bug-1368031-Do-not-send-reply-if-actor-is-dead-r-bil.patch
Attachment #8871697 - Flags: review?(wmccloskey)
(Assignee)

Comment 2

5 months ago
Created attachment 8871698 [details] [diff] [review]
0002-Bug-1368031-Pass-resolver-function-to-Recv-methods-i.patch

New code gen looks like

// PContentParent.h
class PContentParent :
    public mozilla::ipc::IToplevelProtocol,
    public SupportsWeakPtr<PContentParent>
{
public:
    MOZ_DECLARE_WEAKREFERENCE_TYPENAME(PContentParent)

...

public:
    typedef std::function<void(PipelineId)> AllocPipelineIdResolver;

...

    virtual mozilla::ipc::IPCResult
    RecvAllocPipelineId(AllocPipelineIdResolver&& aResolve) = 0;

// PContentParent.cpp
    case PContent::Msg_AllocPipelineId__ID:
        {
            if (mozilla::ipc::LoggingEnabledFor("PContentParent")) {
                mozilla::ipc::LogMessageForProtocol("PContentParent", OtherPid(), "Received ", ((&(msg__)))->type(), mozilla::ipc::MessageDirection::eReceiving);
            }
            PROFILER_LABEL("PContent", "Msg_AllocPipelineId", js::ProfileEntry::Category::OTHER);

            PContent::Transition(PContent::Msg_AllocPipelineId__ID, (&(mState)));
            int32_t id__ = MSG_ROUTING_CONTROL;

            int32_t seqno__ = (msg__).seqno();
            WeakPtr<PContentParent> self__ = this;
            AllocPipelineIdResolver resolver = [this, self__, id__, seqno__](PipelineId aParam) {
                if ((!(self__))) {
                    NS_WARNING("Not resolving promise because actor is dead.");
                    return;
                }
                if ((mState) == (PContent::__Dead)) {
                    NS_WARNING("Not resolving promise because actor is destroyed.");
                    return;
                }
                bool resolve__ = true;
                PipelineId pipelineId;
                pipelineId = aParam;
                IPC::Message* reply__ = PContent::Reply_AllocPipelineId(id__);
                Write(resolve__, reply__);
                // Sentinel = 'resolve__'
                (reply__)->WriteSentinel(3997392463);
                Write(pipelineId, reply__);
                // Sentinel = 'pipelineId'
                (reply__)->WriteSentinel(4067092043);
                (reply__)->set_reply();
                (reply__)->set_seqno(seqno__);

                if (mozilla::ipc::LoggingEnabledFor("PContentParent")) {
                    mozilla::ipc::LogMessageForProtocol("PContentParent", OtherPid(), "Sending reply ", (reply__)->type(), mozilla::ipc::MessageDirection::eSending);
                }
                bool sendok__ = (GetIPCChannel())->Send(reply__);
                if ((!(sendok__))) {
                    NS_WARNING("Error sending reply");
                }
            };
            if ((!(RecvAllocPipelineId(mozilla::Move(resolver))))) {
                mozilla::ipc::ProtocolErrorBreakpoint("Handler returned error code!");
                // Error handled in mozilla::ipc::IPCResult
                return MsgProcessingError;
            }

            return MsgProcessed;
        }
Attachment #8871698 - Flags: review?(wmccloskey)
(Assignee)

Comment 3

5 months ago
Created attachment 8871699 [details] [diff] [review]
0003-Bug-1368031-Update-AllocPipelineId-method-in-Content.patch
Attachment #8871699 - Flags: review?(sotaro.ikeda.g)
(Assignee)

Updated

5 months ago
Assignee: nobody → kchen
Comment on attachment 8871697 [details] [diff] [review]
0001-Bug-1368031-Do-not-send-reply-if-actor-is-dead-r-bil.patch

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

::: ipc/ipdl/ipdl/lower.py
@@ +4329,5 @@
>                          StmtDecl(Decl(Type.INT32, seqno.name),
>                                   init=ExprCall(ExprSelect(self.msgvar, '.', 'seqno'))),
> +                        StmtDecl(Decl(Type('WeakPtr', T=ExprVar(self.clsname)),
> +                                      selfvar.name),
> +                                 init=ExprVar.THIS),

What's this for?
Attachment #8871697 - Flags: review?(wmccloskey) → review+

Updated

5 months ago
Attachment #8871698 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 5

5 months ago
(In reply to Bill McCloskey (:billm) from comment #4)
> Comment on attachment 8871697 [details] [diff] [review]
> 0001-Bug-1368031-Do-not-send-reply-if-actor-is-dead-r-bil.patch
> 
> Review of attachment 8871697 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/ipdl/ipdl/lower.py
> @@ +4329,5 @@
> >                          StmtDecl(Decl(Type.INT32, seqno.name),
> >                                   init=ExprCall(ExprSelect(self.msgvar, '.', 'seqno'))),
> > +                        StmtDecl(Decl(Type('WeakPtr', T=ExprVar(self.clsname)),
> > +                                      selfvar.name),
> > +                                 init=ExprVar.THIS),
> 
> What's this for?

The WeakPtr will be captured by the resolve lambda function and be used to check if it's still alive.
(Assignee)

Comment 6

5 months ago
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #5)
> (In reply to Bill McCloskey (:billm) from comment #4)
> > Comment on attachment 8871697 [details] [diff] [review]
> > 0001-Bug-1368031-Do-not-send-reply-if-actor-is-dead-r-bil.patch
> > 
> > Review of attachment 8871697 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: ipc/ipdl/ipdl/lower.py
> > @@ +4329,5 @@
> > >                          StmtDecl(Decl(Type.INT32, seqno.name),
> > >                                   init=ExprCall(ExprSelect(self.msgvar, '.', 'seqno'))),
> > > +                        StmtDecl(Decl(Type('WeakPtr', T=ExprVar(self.clsname)),
> > > +                                      selfvar.name),
> > > +                                 init=ExprVar.THIS),
> > 
> > What's this for?
> 
> The WeakPtr will be captured by the resolve lambda function and be used to
> check if it's still alive.

Generated code:

            WeakPtr<PContentParent> self__ = this;
            AllocPipelineIdResolver resolver = [this, self__, id__, seqno__](PipelineId aParam) {
                if ((!(self__))) {
                    NS_WARNING("Not resolving promise because actor is dead.");
                    return;
                }

Updated

5 months ago
Attachment #8871699 - Flags: review?(sotaro.ikeda.g) → review+
(Assignee)

Comment 7

5 months ago
Comment on attachment 8871699 [details] [diff] [review]
0003-Bug-1368031-Update-AllocPipelineId-method-in-Content.patch

Bug 1366915 removed this part.
Attachment #8871699 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Blocks: 1313200
(Assignee)

Comment 8

5 months ago
Created attachment 8872999 [details] [diff] [review]
0003-Bug-1368031-Fix-PProfiler-GatherProfile-r-mstange.patch

The IPDL codegen changed the resolver promise to a resolver function. This patch fixes the PProfiler::GatherProfile.
Attachment #8872999 - Flags: review?(mstange)
Attachment #8872999 - Flags: review?(mstange) → review+
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #0)
> The IPC message is already async so there is no point to
> the added delay of MozPromise.

Well, there is a point, but it seems no user of this needs it at the moment: If the receiver runs on the main thread, and it needs to wait for the network or the file system to compute its answer, then it would want to resolve the promise asynchronously.

If you're enforcing a synchronous reply, I think returning the value would be more obvious than calling a callback function.
Maybe with a mozilla::Result<YourReturnType, IPCResult>.
(Assignee)

Comment 10

5 months ago
(In reply to Markus Stange [:mstange] from comment #9)
> (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #0)
> > The IPC message is already async so there is no point to
> > the added delay of MozPromise.
> 
> Well, there is a point, but it seems no user of this needs it at the moment:
> If the receiver runs on the main thread, and it needs to wait for the
> network or the file system to compute its answer, then it would want to
> resolve the promise asynchronously.
> 
> If you're enforcing a synchronous reply, I think returning the value would
> be more obvious than calling a callback function.
> Maybe with a mozilla::Result<YourReturnType, IPCResult>.

You can wrap the callback function in a promise so it's more flexible.

  MozPromiseHolder<ReturnType> holder;
  RefPtr<ReturnTypePromise> promise = holder.Ensure(__func__);
  promise->Then(AbstractThread::MainThread(), __func__, [aResolve](ReturnType aRv) {
    aResolve(aRv);
  },
  [](){});
Oh, I see, so the callback function can be hold on to and called at a later time. That's great then, thanks.

Comment 12

5 months ago
Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4507e6e48d48
Do not send reply if actor is dead r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a39e2222a47
Pass resolver function to Recv methods instead of a MozPromise r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6f613b7ec08
Fix PProfiler::GatherProfile r=mstange

Comment 13

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4507e6e48d48
https://hg.mozilla.org/mozilla-central/rev/9a39e2222a47
https://hg.mozilla.org/mozilla-central/rev/e6f613b7ec08
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.