1164248 - Handling of session/sessionEnded for notifyUssdReceived

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Carol Yang [:cyang]]

Hi Hsin-Yi,

After bug 1058397 was merged, there is a wrapper at [1] called by [2] to repackage "sessionEnded" as "session". I was curious why not perform the repackage at [3]?

This is currently breaking our reference ril such that there is no response form  after a network initiated USSD request is received and displayed.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyService.js#1719
[2] https://dxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageManager.js#92
[3] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RILSystemMessenger.jsm#179

Comment 1

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

(In reply to Carol Yang [:cyang] from comment #0)
> Hi Hsin-Yi,
> 
> After bug 1058397 was merged, there is a wrapper at [1] called by [2] to
> repackage "sessionEnded" as "session". I was curious why not perform the
> repackage at [3]?
> 
> This is currently breaking our reference ril such that there is no response
> form  after a network initiated USSD request is received and displayed.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/
> TelephonyService.js#1719
> [2]
> https://dxr.mozilla.org/mozilla-central/source/dom/messages/
> SystemMessageManager.js#92
> [3]
> https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RILSystemMessenger.jsm#179

Since bug Bug 1091307 TelephonyService.js was disabled in caf build that leads to no USSDReceivedWrapper in caf build, either. We need to move that function out and make the wrapper available in both builds.

Comment 2

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

triage: blocking, in order to fix the USSD function in 2.2 for commercial RIL.

Comment 3

[Anshul]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)

> Since bug Bug 1091307 TelephonyService.js was disabled in caf build that
> leads to no USSDReceivedWrapper in caf build, either. We need to move that
> function out and make the wrapper available in both builds.

Hsin-Yi, we were never using TelephonyService.js with or without the fix for bug 1091307. I don't understand very well the purpose of this wrapper. Could you please provide some context? Also, it seems like something that should have been done in DOM instead of RIL. Please advise.

Comment 4

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

(In reply to Anshul from comment #3)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> 
> > Since bug Bug 1091307 TelephonyService.js was disabled in caf build that
> > leads to no USSDReceivedWrapper in caf build, either. We need to move that
> > function out and make the wrapper available in both builds.
> 
> Hsin-Yi, we were never using TelephonyService.js with or without the fix for
> bug 1091307. I don't understand very well the purpose of this wrapper. Could
> you please provide some context? Also, it seems like something that should
> have been done in DOM instead of RIL. Please advise.

We want to have a well-defined USSD interface for gaia. However, what gaia relies on now is the format of a system message not a DOM Event. So, this wrapping is used to provide a well-defined USSD interface wrapped in a system message.

Yes, after the patch, I don't expect changes on RIL side. All you need to do is keeping using TelephonyMessenger to send out a system message. All magic is hidden behind that. Clear to you?

Comment 5

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

Attachment: 1164248.patch

Move USSDReceivedWrapper out of TelephonyService

Comment 6

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> Created attachment 8605231 [details] [diff] [review]
> 1164248.patch
> 
> Move USSDReceivedWrapper out of TelephonyService

Hi Carol,
The patch should be applied cleanly on v2.2. Could you please give it a try? Thanks!

Comment 7

[Carol Yang [:cyang]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> > Created attachment 8605231 [details] [diff] [review]
> > 1164248.patch
> > 
> > Move USSDReceivedWrapper out of TelephonyService
> 
> Hi Carol,
> The patch should be applied cleanly on v2.2. Could you please give it a try?
> Thanks!

Hi Hsin-Yi,

The patch applied cleanly but I get the following compiler error:

mozbuild.preprocessor.Error: ('/local/mnt/workspace/cyang/builds/LF.BR.1.2.3/gecko/b2g/installer/package-manifest.in', 473, 'UNDEFINED_VAR', 'RESPATH')

Line 473 looks like where you added @RESPATH@/components/USSDReceivedWrapper.js

Can you please help with resolving this error?

Thanks,
Carol

Comment 8

[Anshul]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> We want to have a well-defined USSD interface for gaia. However, what gaia
> relies on now is the format of a system message not a DOM Event. So, this
> wrapping is used to provide a well-defined USSD interface wrapped in a
> system message.
> 
> Yes, after the patch, I don't expect changes on RIL side. All you need to do
> is keeping using TelephonyMessenger to send out a system message. All magic
> is hidden behind that. Clear to you?

Hsin-Yi as I understand the intent of system message is to wake up the app and then the app needs to query for the data. Seems like we are hijacking the system message here. The concern is not merely that qcril has no access to this wrapper but also that this wrapper looks like a hack. Is there a plan to improve this in future?

Comment 9

[Ken Chang[:kenkai]]

Apparently, Hsinyi is working on this bug. Thanks, Hsinyi.

Comment 10

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

(In reply to Anshul from comment #8)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> > We want to have a well-defined USSD interface for gaia. However, what gaia
> > relies on now is the format of a system message not a DOM Event. So, this
> > wrapping is used to provide a well-defined USSD interface wrapped in a
> > system message.
> > 
> > Yes, after the patch, I don't expect changes on RIL side. All you need to do
> > is keeping using TelephonyMessenger to send out a system message. All magic
> > is hidden behind that. Clear to you?
> 
> Hsin-Yi as I understand the intent of system message is to wake up the app
> and then the app needs to query for the data. Seems like we are hijacking
> the system message here. The concern is not merely that qcril has no access
> to this wrapper but also that this wrapper looks like a hack. Is there a
> plan to improve this in future?

You are right, the original intention for introducing system message is to wake up the app and to query the pending data. But, what gaia expects from system message is more than this as time goes. That said, when gaia receives a system message, it's sometimes expected to get a object to execute directly from the message. So, no, this isn't a hack. This is an existing behaviour in the SystemMessage mechanism. This kinda mechanism has been used for other modules such as webactivity.

Does this make sense to you, Anshul?

(In reply to Carol Yang [:cyang] from comment #7)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> > > Created attachment 8605231 [details] [diff] [review]
> > > 1164248.patch
> > > 
> > > Move USSDReceivedWrapper out of TelephonyService
> > 
> > Hi Carol,
> > The patch should be applied cleanly on v2.2. Could you please give it a try?
> > Thanks!
> 
> Hi Hsin-Yi,
> 
> The patch applied cleanly but I get the following compiler error:
> 
> mozbuild.preprocessor.Error:
> ('/local/mnt/workspace/cyang/builds/LF.BR.1.2.3/gecko/b2g/installer/package-
> manifest.in', 473, 'UNDEFINED_VAR', 'RESPATH')
> 
> Line 473 looks like where you added
> @RESPATH@/components/USSDReceivedWrapper.js
> 
> Can you please help with resolving this error?
> 
> Thanks,
> Carol

Let me try compile v2.2 build again and get back to you later.

Comment 11

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> (In reply to Anshul from comment #8)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> > > We want to have a well-defined USSD interface for gaia. However, what gaia
> > > relies on now is the format of a system message not a DOM Event. So, this
> > > wrapping is used to provide a well-defined USSD interface wrapped in a
> > > system message.
> > > 
> > > Yes, after the patch, I don't expect changes on RIL side. All you need to do
> > > is keeping using TelephonyMessenger to send out a system message. All magic
> > > is hidden behind that. Clear to you?
> > 
> > Hsin-Yi as I understand the intent of system message is to wake up the app
> > and then the app needs to query for the data. Seems like we are hijacking
> > the system message here. The concern is not merely that qcril has no access
> > to this wrapper but also that this wrapper looks like a hack. Is there a
> > plan to improve this in future?
> 
> You are right, the original intention for introducing system message is to
> wake up the app and to query the pending data. But, what gaia expects from
> system message is more than this as time goes. That said, when gaia receives
> a system message, it's sometimes expected to get a object to execute
> directly from the message. So, no, this isn't a hack. This is an existing
> behaviour in the SystemMessage mechanism. This kinda mechanism has been used
> for other modules such as webactivity.
> 
> Does this make sense to you, Anshul?
> 

For your reference, this is where the mechanism locates https://dxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageManager.js?from=systemmessagemanager.js#88

Comment 12

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

Attachment: [v2.2] patch

Comment 13

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> Created attachment 8605611 [details] [diff] [review]
> [v2.2] patch

Hi Carol,
Sorry that was a careless compile error.I've uploaded a new v2.2 patch which compiled successfully and worked as expected on my device. Please try again, thank you.

Comment 14

[Anshul]

Hsin-Yi, thanks for your explanation. If system message is now being used to send data as well can we deprecate other events that carry the same information. Seems like there is a lot of duplication of events.

Comment 15

[Carol Yang [:cyang]]

Hi Hsin-Yi,

Your latest patch works!

Thanks!

Comment 16

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

(In reply to Carol Yang [:cyang] from comment #15)
> Hi Hsin-Yi,
> 
> Your latest patch works!
> 
> Thanks!

Awesome! Will ask for review later.

(In reply to Anshul from comment #14)
> Hsin-Yi, thanks for your explanation. If system message is now being used to
> send data as well can we deprecate other events that carry the same
> information. Seems like there is a lot of duplication of events.

I totally agree that we deprecate the duplication. Currently the gaia team is prototyping a new gaia architecture. If the new architecture is feasible, we might not even need system messages in certain cases. I'd suggest we review duplication of DOM events and system messages then to make sure we don't need to flip over again and again, if the duplication isn't something urgent to you. How do you think, Anshul?

Comment 17

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

Attachment: [m-c] 1164248.patch

Hi Edgar,
In this patch I simply move USSDReceivedWrapper stuff out of TelephonyService so that it's accessible even in CAF build.

I also add some things to let the debugger can be enabled at run-time.

Could you help review? Thank you.

Comment 18

[Edgar Chen [:edgar]]

Comment on attachment 8606066 [details] [diff] [review]
[m-c] 1164248.patch

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

r=me with the comment addressed, thank you.

::: dom/telephony/gonk/USSDReceivedWrapper.js
@@ +31,5 @@
> + * Please see SystemMessageManager.js to know how it customizes the wrapper.
> + */
> +function USSDReceivedWrapper() {
> +  this._updateDebugFlag();
> +  Services.prefs.addObserver(kPrefRilDebuggingEnabled, this, false);

removeObserver() when xpcom-shutdown.

Comment 19

[Anshul]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #16)
> I totally agree that we deprecate the duplication. Currently the gaia team
> is prototyping a new gaia architecture. If the new architecture is feasible,
> we might not even need system messages in certain cases. I'd suggest we
> review duplication of DOM events and system messages then to make sure we
> don't need to flip over again and again, if the duplication isn't something
> urgent to you. How do you think, Anshul?
No it's not urgent to us Hsin-Yi. Thanks for understanding. Please involve us in the discussion whenever you have more information.

Comment 20

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

(In reply to Edgar Chen [:edgar][:echen] from comment #18)
> Comment on attachment 8606066 [details] [diff] [review]
> [m-c] 1164248.patch
> 
> Review of attachment 8606066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the comment addressed, thank you.
> 
> ::: dom/telephony/gonk/USSDReceivedWrapper.js
> @@ +31,5 @@
> > + * Please see SystemMessageManager.js to know how it customizes the wrapper.
> > + */
> > +function USSDReceivedWrapper() {
> > +  this._updateDebugFlag();
> > +  Services.prefs.addObserver(kPrefRilDebuggingEnabled, this, false);
> 
> removeObserver() when xpcom-shutdown.

Hi Edgar,
Thanks for the review!

No, We don't need to manually |removeObserver| because in [1] nsPrefBranch listens to NS_XPCOM_SHUTDOWN_OBSERVER_ID and will do |freeObserverList()| when it gets notified. Also, calling |removeObserver()| isn't necessary while during that time nsPreBranch is executing |freeObserverList| because the |removeObserver| function calls are just ignored [2].

[1] https://dxr.mozilla.org/mozilla-central/source/modules/libpref/nsPrefBranch.cpp?from=nsPrefBranch.cpp&case=true#669
[2] https://dxr.mozilla.org/mozilla-central/source/modules/libpref/nsPrefBranch.cpp?from=nsPrefBranch.cpp&case=true#651

Comment 21

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

m-c try https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3972b12dad4
v2.2 try https://treeherder.mozilla.org/#/jobs?repo=try&revision=e42094b26d01

Comment 22

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

Attachment: [v2.2] 1164248_v2_2.patch, r=edgar

add r=edgar, a=2.2+ on commit description

Comment 23

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

https://hg.mozilla.org/integration/b2g-inbound/rev/79ce87dbf4a7

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/79ce87dbf4a7

Comment 25

[Carol Yang [:cyang]]

Comment on attachment 8606795 [details] [diff] [review]
[v2.2] 1164248_v2_2.patch, r=edgar

Hi Bhavana,

Just wanted to make sure this will get into v2.2. Can you please approve?

Thanks,
Carol

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 26

[Ryan VanderMeulen [:RyanVM]]

FYI, Bhavana is no longer with Mozilla.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ea3b70dd2b3f