1116670 - Add [AvailableIn="CertifiedApps"] and [CheckPermissions="sms"] to MobileMessage interfaces

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

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

Attachment: patch

to add "AvailableIn" and "CheckPermissions"

Hi Olli,
May I have your review? Thanks.

Comment 2

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

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

Need to do the same for MozSmsEvent, MozMmsEvent ...

Comment 3

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

Attachment: patch

The summary of this patch:
1) Add [AvailableIn="CertifiedApps"] and [CheckPermissions="sms"] to MobileMessage/sms/mms interfaces:
MozMobileMessageManager.webidl,
MozSmsEvent.webidl,
MozMmsEvent.webidl,
MozMessageDeletedEvent.webidl,
DOMMobileMessageError.webidl

2) Remove those interfaces from test_interfaces.html as they are no longer exposed to every webpage

Hi Olli,
May I have your review, thank you?

Comment 4

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

test_sms_basics.html fails. I am working on revising that test.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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

>-// IMPORTANT: Do not change this list without review from a DOM peer!
>-    "MozMmsEvent",
yikes, how did we end up exposing this.

>-    "MozSmsEvent",
and this




>+ AvailableIn="CertifiedApps",
What is the reason to be so strict? Why not PrivilegedApps?
Please explain this.

And don't we want to change also
#ifdef MOZ_WEBSMS_BACKEND
partial interface Navigator {
  [CheckPermissions="sms", Pref="dom.sms.enabled"]
  readonly attribute MozMobileMessageManager? mozMobileMessage;
};
#endif
and I think this should be fixed.

Comment 6

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

(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 8549520 [details] [diff] [review]
> patch
> 
> >-// IMPORTANT: Do not change this list without review from a DOM peer!
> >-    "MozMmsEvent",
> yikes, how did we end up exposing this.
> 
> >-    "MozSmsEvent",
> and this
> 
> 

Hi Olli,
Thanks for the comment.
MozMobileMessage API has been designed as certified-apps-only API [1] since day 1 AFAIK due to it's security and billing concern. And there's no plan to expose it to 3rd-party apps so far. We still want to hide it from privileged for now, see discussion with Jonas [1]. 

[1] https://developer.mozilla.org/en-US/docs/Web/API/MozSmsManager
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1102062#c25

> 
> 
> >+ AvailableIn="CertifiedApps",
> What is the reason to be so strict? Why not PrivilegedApps?
> Please explain this.
> 
> And don't we want to change also
> #ifdef MOZ_WEBSMS_BACKEND
> partial interface Navigator {
>   [CheckPermissions="sms", Pref="dom.sms.enabled"]
>   readonly attribute MozMobileMessageManager? mozMobileMessage;
> };
> #endif
> and I think this should be fixed.

You are absolutely right. Will do in the revision.

Comment 7

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

Hi Jonathan,
After applying the patch, the mochitest test_sms_basics.html fails due to undefined SmsEvent and undefined mozMobileMessage. I suppose it's because mochitest doesn't run on certified context. I tried to make this mochitest become a chrome mochocitest and ran with ./mach test_sms_basics.html, but still encountered same errors. I am not sure if the mechanism (run on certified context) is supported. Could you please shed some light here? Thank you.

Comment 8

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> Hi Jonathan,
> After applying the patch, the mochitest test_sms_basics.html fails due to
> undefined SmsEvent and undefined mozMobileMessage. I suppose it's because
> mochitest doesn't run on certified context. I tried to make this mochitest
> become a chrome mochocitest and ran with ./mach test_sms_basics.html, but
                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^ 
                                           I meant, ./mach mochitest-chrome-remote ...

> still encountered same errors. I am not sure if the mechanism (run on
> certified context) is supported. Could you please shed some light here?
> Thank you.

Comment 9

[Jonathan Griffin (:jgriffin)]

There is some support for chrome tests in B2G, see bug 797164.  But those tests still don't get executed in a certified context; does this API require being inside a certified app?  Mochitest currently does not support that.

Comment 10

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

(In reply to Jonathan Griffin (:jgriffin) from comment #9)
> There is some support for chrome tests in B2G, see bug 797164.  But those
> tests still don't get executed in a certified context; does this API require
> being inside a certified app?  

MobileMessage API is for certified apps only.

> Mochitest currently does not support that.

Oh, I found an interesting thing. 

|RequestSyncManager| API is also guarded by [AvailableIn=CertifiedApps]. However, it looks possible to have a requestSyncManager mochitest run in a certified context because [1] passes, isn't it? Also, if I do some hack in test_webidl.html, i.e. add additional checks on |MozSmsEvent in window| and |mozMobileMessage in frames[0].navigator|, same what have been done in test_sms_basics.html, then those checks succeed. 

It looks some configuration missing for test_sms_basics.html...

[1] https://dxr.mozilla.org/mozilla-central/source/dom/requestsync/tests/test_webidl.html?from=test_webidl.html#34

Comment 11

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> (In reply to Jonathan Griffin (:jgriffin) from comment #9)
> > There is some support for chrome tests in B2G, see bug 797164.  But those
> > tests still don't get executed in a certified context; does this API require
> > being inside a certified app?  
> 
> MobileMessage API is for certified apps only.
> 
> > Mochitest currently does not support that.
> 
> Oh, I found an interesting thing. 
> 
> |RequestSyncManager| API is also guarded by [AvailableIn=CertifiedApps].
> However, it looks possible to have a requestSyncManager mochitest run in a
> certified context because [1] passes, isn't it? Also, if I do some hack in
> test_webidl.html, i.e. add additional checks on |MozSmsEvent in window| and
> |mozMobileMessage in frames[0].navigator|, same what have been done in
> test_sms_basics.html, then those checks succeed. 
> 
> It looks some configuration missing for test_sms_basics.html...
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/requestsync/tests/
> test_webidl.html?from=test_webidl.html#34

Hi Jonathan,
Sorry for pining you again. I tried to trace the code to understand how dom/requestsync/tests/test_webidl.html and test_sms_basics.html behave so differently, i.e. why |MozSmsEvent in window| is defined in test_webidl.html but not in test_sms_basics.html. Unfortunately, I still don't get it. Do you happen to know the answer? Thank you.

Comment 12

[Jonathan Griffin (:jgriffin)]

I don't know, sorry.  The tests are executed in exactly the same way.  Maybe you can contact the author of the other test?

Comment 13

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

(In reply to Jonathan Griffin (:jgriffin) from comment #12)
> I don't know, sorry.  The tests are executed in exactly the same way.  Maybe
> you can contact the author of the other test?

Thank you Jonathan. I'll contact the author, thanks for your time again :)

Comment 14

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

Attachment: patch (v2)

This revision does:
1) addressed reviewer's comment, the reply in comment 6
2) in addition to test_interfaces.html, modify other related mochitests, simply by adding a pref |dom.ignore_webidl_scope_checks| to ignore webidl scope check

Comment 15

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

Comment on attachment 8558960 [details] [diff] [review]
patch (v2)

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

Hi Olli,

This revision does:
1) address your comment, the reply in comment 6
2) in addition to test_interfaces.html, modify other related mochitests, mainly by adding a pref |dom.ignore_webidl_scope_checks| to ignore webidl scope check

Please help review again, thank you.

Comment 16

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9616f2056d5

Thanks for the review!

Comment 17

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

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ecf46651014f