1179089 - [Telephony] Update the IPC architecture to prevent QI failure

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Ben Hsu [:HoPang]]

As mentioned in the comment 35 and comment 37 in Bug 1174243, when passing from the IPC child side to the IPC parent side, callback objects supporting either nsITelephonyCallback or nsITelephonyDialCallback will be QIed into objects supporting nsITelephonyDialCallback which further make the usage of this interface become unreliable and error-prone. To fix this problem, we should create different callback objects on the IPC parent side according to the type of the callback object on the IPC child side.

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Let TelephonyRequestParent hold the callback objects

Instead of implementing callback interface by TelephonyRequestParent itself, we now let it hold two callback objects and pass the correct one to telephony service.

Comment 2

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[Tracking Requested - why for this release]:

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8629865 [details] [diff] [review]
Let TelephonyRequestParent hold the callback objects

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

The main concept looks good to me!

::: dom/telephony/ipc/TelephonyParent.h
@@ +120,5 @@
>    SendResponse(const IPCTelephonyResponse& aResponse);
>  
> +  Callback*
> +  GetCallback() {
> +    return mCallback;

We should use mCallback.get(), no?

@@ +125,5 @@
> +  }
> +
> +  DialCallback*
> +  GetDialCallback() {
> +    return mDialCallback;

ditto.

Comment 4

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> Comment on attachment 8629865 [details] [diff] [review]
> Let TelephonyRequestParent hold the callback objects
> 
> Review of attachment 8629865 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The main concept looks good to me!
> 
> ::: dom/telephony/ipc/TelephonyParent.h
> @@ +120,5 @@
> >    SendResponse(const IPCTelephonyResponse& aResponse);
> >  
> > +  Callback*
> > +  GetCallback() {
> > +    return mCallback;
> 
> We should use mCallback.get(), no?
> 
> @@ +125,5 @@
> > +  }
> > +
> > +  DialCallback*
> > +  GetDialCallback() {
> > +    return mDialCallback;
> 
> ditto.

https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsRefPtr.h?from=nsRefPtr#227

  operator T*() const

All nsRefPtr implicitly acts like a raw pointer. In addition, according to the comment, we prefer to use the implicit conversion rather than call the get().

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Szu-Yu Chen [:aknow] from comment #4)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> > Comment on attachment 8629865 [details] [diff] [review]
> > Let TelephonyRequestParent hold the callback objects
> > 
> > Review of attachment 8629865 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The main concept looks good to me!
> > 
> > ::: dom/telephony/ipc/TelephonyParent.h
> > @@ +120,5 @@
> > >    SendResponse(const IPCTelephonyResponse& aResponse);
> > >  
> > > +  Callback*
> > > +  GetCallback() {
> > > +    return mCallback;
> > 
> > We should use mCallback.get(), no?
> > 
> > @@ +125,5 @@
> > > +  }
> > > +
> > > +  DialCallback*
> > > +  GetDialCallback() {
> > > +    return mDialCallback;
> > 
> > ditto.
> 
> https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsRefPtr.
> h?from=nsRefPtr#227
> 
>   operator T*() const
> 
> All nsRefPtr implicitly acts like a raw pointer. In addition, according to
> the comment, we prefer to use the implicit conversion rather than call the
> get().

Oh, you are right. Thanks for pointing this to me.

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8629865 [details] [diff] [review]
Let TelephonyRequestParent hold the callback objects

I don't know how to test it...

The test needs to go through the ipc part and has some customization in telphonyService. However, it seems to me that it's not feasible in current test framework.

So I'd like to land the patch directly (already verified on real device).

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8629865 [details] [diff] [review]
Let TelephonyRequestParent hold the callback objects

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

Thank you.

Comment 8

[Szu-Yu Chen [:aknow] (inactive)]

try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2e985eef33f

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/55abf3c44b10

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55abf3c44b10