952875 - [B2G] [SMS] Not able to be notified of a new message from navigator.mozMobileMessage.onreceived before any request sent from content process to chrome process.

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

Sample code:
#1  var mm = navigator.mozMobileMessage;
#2  mm.addEventListener('received',
#3                       function handleReceivedMMS(e) {
#4    var message = e.message;
#5    if (message.delivery === "received") {
#6      ok(true, "Received MMS success.");
#7      SimpleTest.finish();
#8    }
#9  });

Symptom:
The callback of handleReceivedMMS(e) will never be invoked unless we add this snippet code before line#2:
    mm.getThreads();
After more investigation, the problem is that the IPC between MobileMessageManger and chrome process will only be established after a request was sent from content process to parent process.

Hence, file this bug to track this problem.

Comment 1

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Patch

Hi Bevis, would you help try this patch? Thanks!

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8351121 [details] [diff] [review]
Proposed Patch

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

Hi Patrick,

It seems working now.
After apply this patch, no need to send any request to chrome process first to be able to receive the "received" event.

Thanks!

Comment 3

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8351121 [details] [diff] [review]
Proposed Patch

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

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +71,5 @@
>  MobileMessageManager::Init(nsPIDOMWindow *aWindow)
>  {
>    BindToOwner(aWindow);
>  
> +  // Create MmsService if it is not already exists.

// Initialize the MmsService if it hasn't existed yet.

We often use "already" to represent something happened, instead of in a negative expression.

@@ +72,5 @@
>  {
>    BindToOwner(aWindow);
>  
> +  // Create MmsService if it is not already exists.
> +  // If we are in content process, this will register observer on chrome side.

I don't think you need to explain the back-end behaviours in this level, which shouldn't be aware of how the IPC works, so I'd prefer dropping this comment.

@@ +73,5 @@
>    BindToOwner(aWindow);
>  
> +  // Create MmsService if it is not already exists.
> +  // If we are in content process, this will register observer on chrome side.
> +  nsCOMPtr<nsIMmsService> mmsService = do_GetService(MMS_SERVICE_CONTRACTID);

Also, please check the availability of |mmsService| and return true/false value for this Init(), since other functions do the similar things.

In Navigator::GetMozMobileMessage(), use NS_ENSURE_TRUE(...) to check the return of |mMobileMessageManager->Init(mWindow)|.

Also, do we need to worry about the same issue for smsService?

::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +110,5 @@
>  {
>    Preferences::AddStrongObservers(this, kObservedPrefs);
>    mMmsDefaultServiceId = getDefaultServiceId(kPrefMmsDefaultServiceId);
>    mSmsDefaultServiceId = getDefaultServiceId(kPrefSmsDefaultServiceId);
> +  GetSmsChild(); // Create SmsParent/Child.

...
(one black line)
// Construct the Sms{Parent,Child} when initializing the IPC service so that
// we can handle the unsolicited events without calling any functions to
// establish the IPC tunnel first.
GetSmsChild();

Comment 4

[Gene Lian (I already quit Mozilla)]

Btw, nice work! Patrick!

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Patch

Still failure in Gaia UI test: returning NULL changes the behavior on b2g desktop. Need to figure out how to fix the failure.

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

This bug is blocking our ability to provide tests for OEM device certification. Can this be bumped in priority?

Comment 7

[Malini Das [:mdas] - Away, not checking bugmail]

Correction: it's not a blocker per se, since we can use the work around in the first comment. But the workaround doesn't correctly replicate the behavior of a normal application, and we need our apps (and how our apps use APIs) to be as close to native apps as possible.

Comment 8

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Block bug 958738 because I'm to eliminate MOZ_WEBSMS_BACKEND usage from Navigator.cpp, which solves bug 958738 as a side effect.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch

Hi Bent,

In this patch I want to reconstruct the call path that once removed in bug 889898.  The problem we have since then is no IPC transaction is ever invoked at the time |navigator.mozMobileMessage| is returned.  The parent process will never realize that a new PSms child is waiting for further unsolicited messages until any MobileMessage API is called.  The solution, as Patrick has illustrated, is to call GetSmsChild() before HasMobileMessageSupport() returns.

However, I'd like to call GetSmsChild() for some reason, not just a blind call.  In bug 916605, DOM API entries should be hidden if that's not supported.  This patch gives a definition to "supported" as "having an valid nsISmsService".  So, for a child process, before instantiating a SmsIPCService object, it has to check if SendPSmsConstructor() returns true first, and this IPC call resolves the bug.

PSms then becomes a sync protocol because it has to return something.  We can't avoid a sync IPC call here because there is no other way to return such information with only one or more async calls.  We can keep PSms async, but we still have to add another sync HasSupport() call to PSms, and that has to be called right after SendPSmsConstrutor() inside HasMobileMessageSupport().

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Attachment: test case

Supposed test case.  But somehow SpeciallPowers::pushPermissions doesn't work in OOP mode, so this test case will always timeout.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Full try for attachment 8433130 [details] [diff] [review] only: https://tbpl.mozilla.org/?tree=Try&rev=1f4ab8610ac3

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hi Vicamo,

I think we should really avoid sync messages/protocols whenever possible since they will block the app entirely. Especially something that could happen on an app startup path.

Does |do_GetService(SMS_SERVICE_CONTRACTID)| work in child processes and return the same null-or-not-null result as in the parent? If so then I think a better way to avoid the sync protocol would be to do the check in the child process first, returning true or false in |Navigator::HasMobileMessageSupport| if the service is null or not. Then in the parent in |ContentParent::AllocPSmsParent| you just return null if the service doesn't exist, forcing the child to be killed. You basically assume that the child process is behaving and then kill it if you get evidence that it isn't behaving properly.

As for fixing this bug, you could override |DOMEventTargetHelper::AddEventListener| in |MobileMessageManager| to send the PSMS constructor there... Doesn't that sound cleaner?

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #14)
> Hi Vicamo,
> 
> I think we should really avoid sync messages/protocols whenever possible
> since they will block the app entirely. Especially something that could
> happen on an app startup path.
> 
> Does |do_GetService(SMS_SERVICE_CONTRACTID)| work in child processes and
> return the same null-or-not-null result as in the parent?

We don't know from child process's point of view. A better way to describe it might be "we don't wanna know".

MobileMessage DOM/IPC APIs are always compiled along with Gecko itself. This is a directly result from the request to eliminate preprocessor usage.  There is no way to conditionally turn off PSms, neither. So, in the child process side, from MobileMessageManager or PSmsChild's point of view, we don't know whether there has been some service registered for SMS_SERVICE_CONTRACTID or not in the parent process side.  I return nulled SmsIPCService only if a sync IPC call returns so.

Over all, the answer for the problem in question is true.  But a deeper problem is how to implement this behaviour under current conditions.

The original lines in bug 889898 were:

  bool supported = false;
  nsCOMPtr<nsISmsService> service =
    do_GetService(SMS_SERVICE_CONTRACTID);
  if (!service ||
      NS_FAILED(service->HasSupport(&supported)) ||
      !supported) {
    return false;
  }

nsISmsService::HasSupport was a sync IPC call. And you can also find it called every time |Navigator::HasMobileMessageSupport| was invoked.  We have to bury one sync call somewhere because that's how we get some information from parent process, but the details can be fine tuned.

So I make this sync IPC call called only once per child process.  From the patch I keep the reference of a SmsIPCService instance, so we don't need to invoke a second sync IPC call for further |Navigator::HasMobileMessageSupport| calls.  The performance impact exists in the first call only.

> If so then I think
> a better way to avoid the sync protocol would be to do the check in the
> child process first, returning true or false in
> |Navigator::HasMobileMessageSupport| if the service is null or not. Then in
> the parent in |ContentParent::AllocPSmsParent| you just return null if the
> service doesn't exist, forcing the child to be killed. You basically assume
> that the child process is behaving and then kill it if you get evidence that
> it isn't behaving properly.

Is |if (!navigator.mozMobileMessage) { return; }| an evil step that we should even kill the app? This break Gaia's mock function for automation tests. I think |Navigator::HasMobileMessageSupport| should just work as a probe for a navigator attribute, permission control should go to |Navigator::GetMobileMessageManager| instead.

> As for fixing this bug, you could override
> |DOMEventTargetHelper::AddEventListener| in |MobileMessageManager| to send
> the PSMS constructor there... Doesn't that sound cleaner?

I wan planning to get rid of those ugly getDefaultServiceId() for next.  With a sync PSms, I can return something more as contructor parameters for SmsIPCService.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> (In reply to ben turner [:bent] (use the needinfo? flag!) from comment #14)
> > Hi Vicamo,
> > 
> > I think we should really avoid sync messages/protocols whenever possible
> > since they will block the app entirely. Especially something that could
> > happen on an app startup path.
> 
> We don't know from child process's point of view. A better way to describe
> it might be "we don't wanna know".
> 
> I wan planning to get rid of those ugly getDefaultServiceId() for next. 
> With a sync PSms, I can return something more as contructor parameters for
> SmsIPCService.

I think I just come up solutions for both issues from more brain storm here. Thank you.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> We don't know from child process's point of view.

Ok, then I guess the only other choice we have is that we could send the information eagerly when we launch the process. Several of us have talked about this in the past but we don't have a generic mechanism for this kind of thing yet.

> The performance impact exists in the first call only.

Yes, I understand. The worry is that this first call will happen somewhere on the app startup path and will hurt startup time.

> Is |if (!navigator.mozMobileMessage) { return; }| an evil step that we
> should even kill the app?

Oh, no. If we were able to know the status of the service from the child then the DOM would return null here and never send the PSms message to the parent. The child would only be killed if it still somehow managed to send a PSms message (e.g. it got hacked somehow).

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> I think I just come up solutions for both issues from more brain storm here.
> Thank you.

Oh, so I should not review the current patches in this bug yet?

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8433130 [details] [diff] [review]
patch

Thank you. I'm to provider a simpler patch as you said in comment 14.

Comment 20

[BMO Automation]

Firefox OS is not being worked on