Closed Bug 1179089 Opened 4 years ago Closed 4 years ago

[Telephony] Update the IPC architecture to prevent QI failure

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog, firefox42 fixed)

RESOLVED FIXED
FxOS-S4 (07Aug)
tracking-b2g backlog
Tracking Status
firefox42 --- fixed

People

(Reporter: bhsu, Assigned: aknow)

References

Details

Attachments

(1 file)

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.
See Also: → 1174243
Assignee: nobody → szchen
Instead of implementing callback interface by TelephonyRequestParent itself, we now let it hold two callback objects and pass the correct one to telephony service.
Attachment #8629865 - Flags: feedback?(htsai)
[Tracking Requested - why for this release]:
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.
Attachment #8629865 - Flags: feedback?(htsai) → feedback+
(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().
(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 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).
Attachment #8629865 - Flags: review?(htsai)
Comment on attachment 8629865 [details] [diff] [review]
Let TelephonyRequestParent hold the callback objects

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

Thank you.
Attachment #8629865 - Flags: review?(htsai) → review+
https://hg.mozilla.org/mozilla-central/rev/55abf3c44b10
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S4 (07Aug)
You need to log in before you can comment on or make changes to this bug.