Closed Bug 904934 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C2/1.4 S2(17jan)
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: anshulj, Assigned: aknow)

References

Details

Attachments

(1 file, 7 obsolete files)

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?
I don't know if mozMobileConnection is exposed to chrome scripts; you'd need to ask the webapi guys.
Hey Jonathan, would you know a POC in the webapi team?
I'd ask Andrew Overholt; he would either know or could point you to who would know.
Andrew, would you be able to help.
Flags: needinfo?(overholt)
I'm on it.
Flags: needinfo?(overholt)
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.
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?
Flags: needinfo?(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.
Flags: needinfo?(jgriffin) → needinfo?(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?
Flags: needinfo?(mounir)
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.
Flags: needinfo?(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?
Flags: needinfo?(mounir)
(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.
Flags: needinfo?(mounir)
Any update here?
Gene or Fernando may be able to help.
Flags: needinfo?(gene.lian)
Flags: needinfo?(ferjmoreno)
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.
Hi Ken, could you please find someone to take this?
Flags: needinfo?(kchang)
Jessica may have some experiences in this field.
Assignee: nobody → jjong
Flags: needinfo?(kchang)
Flags: needinfo?(mounir)
Flags: needinfo?(gene.lian)
Flags: needinfo?(ferjmoreno)
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.
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.
Assignee: jjong → nobody
Vicamo, this is really blocking a lot of our script and we would really appreciate if you or someone you know can help here?
Flags: needinfo?(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.
Flags: needinfo?(vyang) → needinfo?(bobbyholley+bmo)
Summary: Not able to access navigator.mozMobileConnection if the test is running in chrome context → Not able to call SpecialPowers.addPermission if the test is running in chrome context
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.
Flags: needinfo?(bobbyholley+bmo)
Summary: Not able to call SpecialPowers.addPermission if the test is running in chrome context → Not able to access navigator.mozMobileConnection if the test is running in chrome context
Thanks Bobby, wondering if you knew who could help debug why navigator.mozMobileConnection.voice is hanging?
(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.
Flags: needinfo?(vyang)
Vicamo, would you be able to help. We would really appreciate your help.
(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?
Flags: needinfo?(vyang)
Successful pass.  So what's the problem actually?
Attached file test_moz_conn.js (obsolete) —
(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.
Flags: needinfo?(vyang)
Note, the file is a simple hack to reproduce the issue.  I can modify it for regression if you like.
Attachment #827990 - Attachment is obsolete: true
Attached patch test_moz_conn : v2 (obsolete) — Splinter Review
Add some debug messages in MobileConnection DOM code.
Attachment #828999 - Attachment is obsolete: true
Attached file adb logcat for attachment 829142 (obsolete) —
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
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.
Flags: needinfo?(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.
Attached file test_moz_sms_received.js (obsolete) —
mozMobileMessage::onreceived is immune from this issue.
Thanks for looking into this Vicamo. Did you figure out the root cause of the issue?
(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.
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.
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.
Blocks: 942267
blocking-b2g: --- → 1.3?
Flags: needinfo?(htsai)
Ken, need your help here.
Flags: needinfo?(htsai) → needinfo?(kchang)
(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 :)
Flags: needinfo?(kchang)
Aknow,

Any idea on how we plan to proceed ahead?
Flags: needinfo?(szchen)
Attached file test_chrome_telephony.js (obsolete) —
Event for mozTelephony in chrome context is ok
Flags: needinfo?(szchen)
: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'));
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?
Why !CheckPermission("mobilenetwork")?
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
Flags: needinfo?(fabrice)
(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.
(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");
   }
Yes. It's my question -- Is there any risk to remove that check in Comment 47?
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
Flags: needinfo?(fabrice)
Assignee: nobody → szchen
Attached patch remove mobilenetwork check (obsolete) — Splinter Review
Attachment #8350505 - Flags: review?(htsai)
Comment on attachment 8350505 [details] [diff] [review]
remove mobilenetwork check

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

Indeed good catch :)
Attachment #8350505 - Flags: review?(htsai) → review+
Attachment #8350505 - Attachment is obsolete: true
Attachment #8351080 - Flags: review+
Attachment #8348680 - Attachment is obsolete: true
Attachment #829152 - Attachment is obsolete: true
Attachment #829147 - Attachment is obsolete: true
Attachment #829142 - Attachment is obsolete: true
Component: General → RIL
Please uplift this to 1.3 as well. Hsin-Yi and Szu-Yu, really appreciate your help in getting this issue resolved!
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+
Set to 1.3+ per Hsinyi's comments.
blocking-b2g: 1.3? → 1.3+
https://hg.mozilla.org/mozilla-central/rev/ebe5458dd0c6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.