904934 - Not able to access navigator.mozMobileConnection if the test is running in chrome context

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Anshul]

I have a test that is running in chrome context that is trying to access the navigator.mozMobileConnection object. As soon as the test script tries to access mozMobileConnection.voice it hangs.

If I don't run the script in the chrome context and use SpecialPowers.addPermission("mobileconnection", true, document); then the script works fine. So assuming this is a permission issue in other words is there a way to add the permission to "mobileconnection" in the test script running in chrome context?

Comment 1

[Jonathan Griffin (:jgriffin)]

I don't know if mozMobileConnection is exposed to chrome scripts; you'd need to ask the webapi guys.

Comment 2

[Anshul]

Hey Jonathan, would you know a POC in the webapi team?

Comment 3

[Jonathan Griffin (:jgriffin)]

I'd ask Andrew Overholt; he would either know or could point you to who would know.

Comment 4

[Anshul]

Andrew, would you be able to help.

Comment 5

[Andrew Overholt [:overholt]]

I'm on it.

Comment 6

[Mounir Lamouri (:mounir)]

Permission checks do not happen for system principals which are used for chrome elevated privileges. If you do not have a system principal, you need to have the permission explicitly set.

See:
https://mxr.mozilla.org/mozilla-central/source/dom/network/src/MobileConnection.cpp#94
https://mxr.mozilla.org/mozilla-central/source/dom/network/src/MobileConnection.cpp#153
https://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#988
https://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#1008

Comment 7

[Anshul]

Hello Mounir, once I get System Principal, how do I use it to get access to navigator.mozMobileConnection. If I am calling an eval using the sandbox then I get an error saying navigator is not defined. Alternatively how do I add the correct permission to the script running in chrome context.

Comment 8

[Michael Schwartz [:m4]]

Trying to add permissions explicitly from a Marionette script in the chrome context using:

 SpecialPowers.addPermission("mobileconnection", true, document);

Results in:

 TEST-UNEXPECTED-FAIL | test_call.js | JavascriptException: TypeError: arg.nodePrincipal.URI is null
        stacktrace:
        execute_async_script @test_call.js
        inline javascript, line 1484
        src: "undefined"

Jonathan or Mounir, any insight on how best to set "permission explicitly" as suggested in c6 for a Marionette test running in the chrome context?

Comment 9

[Jonathan Griffin (:jgriffin)]

I don't know if this is possible, let's ask mounir.  The problem with trying to do this in chrome context is that 'document' is associated with a XUL window, not a DOMWindow.  I don't know if it's possible to add permissions to a XUL document.

Comment 10

[Mounir Lamouri (:mounir)]

What are you exactly trying to do? The only information I have from this bug is that you are trying to access navigator.mozMobileConnection from a chrome context which should work. Why are you doing that? What are you trying to actually achieve?

Comment 11

[Anshul]

Mounir, we have a test script that runs in chrome context and tries to test if the information sent from RIL is reflected correctly in gaia using the mozMobileConnection. When we call  mozConnection.addEventListener("voicechange", function onVoiceChange() {} the script hangs and times out. If we don't run this script in chrome context it works fine.

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to Anshul from comment #11)
> Mounir, we have a test script that runs in chrome context and tries to test
> if the information sent from RIL is reflected correctly in gaia using the
> mozMobileConnection. When we call 
> mozConnection.addEventListener("voicechange", function onVoiceChange() {}
> the script hangs and times out. If we don't run this script in chrome
> context it works fine.

What makes you think that this behaviour is related to the lack of permission? I mean, is the permission actually not granted? Also, what do you call hanging? Is that a script test timeout or an actual Firefox hang?

Comment 13

[Anshul]

(In reply to Mounir Lamouri (:mounir) from comment #12)
> (In reply to Anshul from comment #11)
> > Mounir, we have a test script that runs in chrome context and tries to test
> > if the information sent from RIL is reflected correctly in gaia using the
> > mozMobileConnection. When we call 
> > mozConnection.addEventListener("voicechange", function onVoiceChange() {}
> > the script hangs and times out. If we don't run this script in chrome
> > context it works fine.
> 
> What makes you think that this behaviour is related to the lack of
> permission? I mean, is the permission actually not granted? Also, what do
> you call hanging? Is that a script test timeout or an actual Firefox hang?
Script is timing out as the voicechange listener is not getting invoked. I am not really sure if this is a permission issue. I found this script http://mxr.mozilla.org/mozilla-central/source/dom/network/tests/marionette/test_call_barring_get_option.js#8 that made me think it was some permission issue.

Comment 14

[Michael Schwartz [:m4]]

Any update here?

Comment 15

[Andrew Overholt [:overholt]]

Gene or Fernando may be able to help.

Comment 16

[Gene Lian (I already quit Mozilla)]

My plate is really full (multi-sim and read report for V1.3). I can help find someone in our team to help out. Keep the needinfo to myself to track.

Comment 17

[Gene Lian (I already quit Mozilla)]

Hi Ken, could you please find someone to take this?

Comment 18

[Ken Chang[:kenkai]]

Jessica may have some experiences in this field.

Comment 19

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

Duplicate bug 926824?

Comment 20

[Anshul]

I went through bug 926824 and it does seem like this is the same bug. However we are seeing the same issue on 1.2 and 1.3 as well.

Comment 21

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

I tested with SpecialPowers.addPermission("", true, document) in chrome context and the exception is thrown as well (like in comment 8). I don't think this is a mobileconnection bug.

Comment 22

[Anshul]

Vicamo, this is really blocking a lot of our script and we would really appreciate if you or someone you know can help here?

Comment 23

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

Sorry, I really don't have idea how the principal thing work.  But I guess Bobby might know.

@Bobby, the thing is, we have a very simple marionette test case:

  MARIONETTE_TIMEOUT = 10000;
  MARIONETTE_CONTEXT = "chrome";
  SpecialPowers.addPermission("", true, document);

and it throws an exception as in comment 8.  Do you have idea what's going on here or have another person recommended?

I've also updated the bug title to reflect the fact that this is actually not bounded to one specific component.

Comment 24

[Bobby Holley (:bholley)]

If you're running in a chrome context, you should have System Principal (which has all the privileges imaginable), and shouldn't need anything like SpecialPowers.addPermission (which, as jgriffin points out, was not designed to be used in this context). I think the old summary was more accurate.

I think the whole discussion of permissions here is probably a wild goose chase, which appears to have been set off by the speculation in comment 0.

We need someone to debug this and figure out why navigator.mozMobileConnection.voice is hanging.

Comment 25

[Anshul]

Thanks Bobby, wondering if you knew who could help debug why navigator.mozMobileConnection.voice is hanging?

Comment 26

[Bobby Holley (:bholley)]

(In reply to Anshul from comment #25)
> Thanks Bobby, wondering if you knew who could help debug why
> navigator.mozMobileConnection.voice is hanging?

Vicamo, presumably.

Comment 27

[Anshul]

Vicamo, would you be able to help. We would really appreciate your help.

Comment 28

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

(In reply to Anshul from comment #27)
> Vicamo, would you be able to help. We would really appreciate your help.

Do you have some simplified test script or it just doesn't work any way?

Comment 29

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

Attachment: test case: access mozMobileConnection.voice in chrome context

Successful pass.  So what's the problem actually?

Comment 30

[Michael Schwartz [:m4]]

Attachment: test_moz_conn.js

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #29)
> Created attachment 827990 [details] [diff] [review]
> test case: access mozMobileConnection.voice in chrome context
> 
> Successful pass.  So what's the problem actually?

Thanks Vicamo.  Looks like the bug got off track a little.  The issue is that we're not getting callbacks in the chrome context eg. voicechange.  I'm attaching a test that demonstrates the issue; it's reproducible on moz ril as well.

Comment 31

[Michael Schwartz [:m4]]

Note, the file is a simple hack to reproduce the issue.  I can modify it for regression if you like.

Comment 32

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

Attachment: test_moz_conn : v2

Add some debug messages in MobileConnection DOM code.

Comment 33

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

Attachment: adb logcat for attachment 829142

In previous patch, I add a utility function nsEventListenerManager::CountListenersFor() to get num of current registered voicechange event handlers.  In the log file from line 1255 to 1269, I find the number became "1" after |addEventListener| in test script, but then it went back to "0" immediately.  As a result when RILContentHelper delivered "voicechange" event to mozMobileConnection in line 1525, there is no event handler at all.

1255 I/Gecko   (   45): MARIONETTE LOG: INFO: Test assumes that airplane mode is off - so make sure
1256 I/Gecko   (   45): MARIONETTE TEST RESULT:TEST-PASS | test_mobile_connection_voicechange_chrome.js | navigator.mozMobileConnection - [object MozMobileConnection] was true, expected true
1257 I/Gecko   (   45): MARIONETTE TEST RESULT:TEST-PASS | test_mobile_connection_voicechange_chrome.js | navigator.mozMobileConnection.voice - [xpconnect wrapped nsIDOMMozMobileConnectionInfo] was true, expecte     d true
1258 I/Gecko   (   45): MobileConnection::GetVoice - 0x46065580:1
1259 I/Gecko   (   45): MARIONETTE TEST RESULT:TEST-PASS | test_mobile_connection_voicechange_chrome.js | navigator.mozMobileConnection.voice - [xpconnect wrapped nsIDOMMozMobileConnectionInfo] was true, expecte     d true
1260 I/Gecko   (   45): MobileConnection::GetVoice - 0x4602de80:0

1517 I/Gecko   (   45): -*- RILContentHelper: Received message 'RIL:VoiceInfoChanged': {"clientId":0,"data":{"connected":false,"emergencyCallsOnly":false,"roaming":false,"network":null,"cell":null,"type":null,"s     ignalStrength":null,"relSignalStrength":null,"state":"notSearching"}}
...
1527 I/Gecko   (   45): MobileConnection::NotifyVoiceChanged - 0x4602de80:0

Comment 34

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

1258 I/Gecko   (   45): MobileConnection::GetVoice - 0x46065580:1
1260 I/Gecko   (   45): MobileConnection::GetVoice - 0x4602de80:0

0x46065580 and 0x4602de80 are memory addresses of MobileConnection::GetExistingListenerManager(), and the number behind is num of voicechange event handlers.  Somehow the nsEventListenerManager changed, don't know why yet.

Comment 35

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #34)
> 1258 I/Gecko   (   45): MobileConnection::GetVoice - 0x46065580:1
> 1260 I/Gecko   (   45): MobileConnection::GetVoice - 0x4602de80:0

BTW, I turned off all Gaia |addEventListener('voicechange', ...)| lines as well.

Comment 36

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

Attachment: test_moz_sms_received.js

mozMobileMessage::onreceived is immune from this issue.

Comment 37

[Anshul]

Thanks for looking into this Vicamo. Did you figure out the root cause of the issue?

Comment 38

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

(In reply to Anshul from comment #37)
> Thanks for looking into this Vicamo. Did you figure out the root cause of
> the issue?

Not really.  I have to be honest this is not a critical issue for Firefox OS so it won't be on top of my queue.  Sorry.

Comment 39

[Anshul]

Sure Vicamo, we understand. This is however blocking a lot of our scripts. So we would appreciate if you could look at it whenever you get a chance.

Comment 40

[Anshul]

Hsin-Yi, were you able to find the owner of this bug? I am requesting this to be 1.3? as because of this issue our test  scripts are failing and therefore we won't be able to catch any regressions on 1.3.

Comment 41

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

Ken, need your help here.

Comment 42

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #41)
> Ken, need your help here.

After talking to Aknow, he is able to take a look first. Thanks, Aknow :)

Comment 43

[Preeti Raghunath(:Preeti)]

Aknow,

Any idea on how we plan to proceed ahead?

Comment 44

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

Attachment: test_chrome_telephony.js

Event for mozTelephony in chrome context is ok

Comment 45

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

:Preeti,

I have worked on this today, but still not figure out the root cause. From comment 36 and 44, both mozMobileMessage and mozTelephony work well in chrome context. Looks like we only have problem on mozMobileConnection?

For mozMobileConnection, it indeed called the event dispatch in mobile connection dom. Not sure why the event callback is not triggered. However, if I trigger the event manually in the test script, mobile connection is able to receive it...

==============================

let connection = window.navigator.mozMobileConnections[0];

let callback = function() {
  log("Received 'radiostatechange' event");
  ok(true);
  finish();
};

connection.addEventListener('radiostatechange', callback);
//connection.setRadioEnabled(false);  // should receive the event. work fine in content context.
connection.dispatchEvent(new Event('radiostatechange'));

Comment 46

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

I got the root cause. It's permission. Actually, we got "too much permission" when running in chrome context.

http://mxr.mozilla.org/mozilla-central/source/dom/network/src/MobileConnection.cpp#105

We have two permissions.
- mobilenetwork
- mobileconnection

When running in chrome, We got both of them. Therefore the program will not enter the 'if' block. That means the mozMobileConnection object we use in test script does not register to the provider. So it could not received any message from the provider.

However, I have no idea about how to keep the same permission policy but also resolve this issue. Does anyone have some suggestions?

Comment 47

[Michael Schwartz [:m4]]

Why !CheckPermission("mobilenetwork")?

Comment 48

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

Hi Fabrice,

Do you know the answer for Comment 47?
Why we need to have this check "!CheckPermission("mobilenetwork")" ?
http://mxr.mozilla.org/mozilla-central/source/dom/network/src/MobileConnection.cpp#105

The problem is that we could register mobileConnection message if we have "mobileconnection" permission, but not when we have "mobileconnection" + "mobilenetwork" (In this case, the app should be more powerful...).

Ref: App permissions | https://developer.mozilla.org/en-US/Apps/Developing/App_permissions

Comment 49

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

(In reply to Szu-Yu Chen [:aknow] from comment #48)
> Hi Fabrice,
> 
> Do you know the answer for Comment 47?
> Why we need to have this check "!CheckPermission("mobilenetwork")" ?
> http://mxr.mozilla.org/mozilla-central/source/dom/network/src/
> MobileConnection.cpp#105
> 
> The problem is that we could register mobileConnection message if we have
> "mobileconnection" permission, but not when we have "mobileconnection" +
> "mobilenetwork" (In this case, the app should be more powerful...).
> 
> Ref: App permissions |
> https://developer.mozilla.org/en-US/Apps/Developing/App_permissions

If I remember everything right, "mobilenetwork" permission is for "marketplace" app or other privileged applications. They are allowed to access mozMobileConnection.lastKnownNetwork and mozMobileConnection.lastKnownHomeNetwork only.

Comment 50

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #49)
> (In reply to Szu-Yu Chen [:aknow] from comment #48)
> > Hi Fabrice,
> > 
> > Do you know the answer for Comment 47?
> > Why we need to have this check "!CheckPermission("mobilenetwork")" ?
> > http://mxr.mozilla.org/mozilla-central/source/dom/network/src/
> > MobileConnection.cpp#105
> > 
> > The problem is that we could register mobileConnection message if we have
> > "mobileconnection" permission, but not when we have "mobileconnection" +
> > "mobilenetwork" (In this case, the app should be more powerful...).
> > 
> > Ref: App permissions |
> > https://developer.mozilla.org/en-US/Apps/Developing/App_permissions
> 
> If I remember everything right, "mobilenetwork" permission is for
> "marketplace" app or other privileged applications. They are allowed to
> access mozMobileConnection.lastKnownNetwork and
> mozMobileConnection.lastKnownHomeNetwork only.

So the problem seems be what we should do if an APP somehow has both mobilenetwork and mobileconnection permissions? Because the APP is authorized with mobileconnection permission to receive message from RIL, should we simply handle this case as

 if (CheckPermission("mobileconnection")) {
     DebugOnly<nsresult> rv = mProvider->RegisterMobileConnectionMsg(mClientId, mListener);
     NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
                      "Failed registering mobile connection messages with provider");
 
     printf_stderr("MobileConnection initialized");
   }

Comment 51

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

Yes. It's my question -- Is there any risk to remove that check in Comment 47?

Comment 52

[[:fabrice] Fabrice Desré]

That look safe to remove the check, yes. That would be good to have tests with only the mobilenetwork permission and make sure that only let access lastKnownNetwork and lastKnownHomeNetwork

Comment 53

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

Attachment: remove mobilenetwork check

Comment 54

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

Comment on attachment 8350505 [details] [diff] [review]
remove mobilenetwork check

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

Indeed good catch :)

Comment 55

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

Attachment: [Final] Remove mobilenetwork check. r=hsinyi

Comment 56

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

https://tbpl.mozilla.org/?tree=Try&rev=f84fd48c1e92

Comment 57

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

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

Comment 58

[Anshul]

Please uplift this to 1.3 as well. Hsin-Yi and Szu-Yu, really appreciate your help in getting this issue resolved!

Comment 59

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

We fix ambiguous permission model in this bug. This fix is simple and risk is low. The fix also benefits auto testing. So, nominate this to 1.3+

Comment 60

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

Set to 1.3+ per Hsinyi's comments.

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f6bc4ec00adc