Closed Bug 1131022 Opened 9 years ago Closed 9 years ago

Investigate mozIccManager is undefined test failures

Categories

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::UI Tests
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: RobertC, Assigned: martijn.martijn)

References

Details

(Keywords: regression, Whiteboard: [xfail])

Attachments

(1 file)

[gaia] mwargers:moziccmanager > mozilla-b2g:master
46 bytes, text/x-github-pull-request
davehunt
: review+
Details | Review 
http://jenkins1.qa.scl3.mozilla.com/view/UI/job/flame-kk-319.mozilla-central.nightly.ui.functional.non-smoke.1/15/console

test_import_contacts_from_SIM and test_import_edit_export_contact are failing because they return undefined for window.navigator.mozIccManager.

The issue seems to be with the marionette.set_context() method that we use before we interact with the sim.
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L186
If I comment the lines where we switch to CONTEXT_CHROME the test passes, so mozIccManager is visible from the System app.

Dave, do you know of any recent changes to marionette that would affect this? I would prefer not to remove the set_context() methods if they only need to be updated.
Flags: needinfo?(dave.hunt)
QA Whiteboard: [fxosqa-auto-backlog+]
I doubt this is due to a change in Marionette. Please provide a regression range, and if we can identify the change that caused this to regress we should as the author/reviewer of the change to determine if this is intentional.
Flags: needinfo?(dave.hunt)
Whiteboard: [xfail]
This might have been caused by the patch in bug 1116680.
Hmm.. I don't know why this bug has not be updated with information I provided :RobertC with on IRC:

davehunt: I see that it started in build 10
davehunt: so we at least have http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c5f187b65bf&tochange=f8ce4cf2e71d
davehunt: bug 1116680 jumps out immediately
davehunt: you could just download the inbound builds before/after that was introduced to confirm
RobertC: davehunt, will do
davehunt: we also run the non smoke tests against the nightly tinderbox builds, so I can get the range down to http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=03b0004eba32&tochange=e0f56d3f1fc0
davehunt: that still contains the bug mentioned above
Ok, then I'm pretty sure this is caused by bug 1116680. Perhaps Hsin-Yi or Olli can provide an answer as to what has to be fixed to get this working again.
Blocks: 1116680
Flags: needinfo?(htsai)
Flags: needinfo?(bugs)
Keywords: regressionwindow-wanted
So we're running the tests without enabling the stuff for CertifiedApps?
Or should we not have hidden the interface from Privileged apps?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #5)
> So we're running the tests without enabling the stuff for CertifiedApps?
> Or should we not have hidden the interface from Privileged apps?

I think the 2nd one, I think this piece of javascript code is run in the system app: http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_data_layer.js#164
System app should have the privileges to access certifiedapps-only interfaces.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #6)
> (In reply to Olli Pettay [:smaug] from comment #5)
> > So we're running the tests without enabling the stuff for CertifiedApps?
> > Or should we not have hidden the interface from Privileged apps?
> 
> I think the 2nd one, I think this piece of javascript code is run in the
> system app:
> http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_data_layer.js#164

Icc interfaces are certifiedapps-only, and my team was requested to hide them from privileged apps or web content. So bug 1116680 is indeed a way we want to go.

Could you please check UI test environment again? I wonder if there's something missing as I haven't seen issues on a real device.
Flags: needinfo?(htsai)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> Could you please check UI test environment again? I wonder if there's
> something missing as I haven't seen issues on a real device.

I think comment 0 gives the right answer then. Currently, we switch to CONTEXT_CHROME before trying to access mozIccManager. The chrome context is not a certified app, apparently, so we should stay in CONTEXT_CONTENT, because the system app is a certified app, I would think.
There's no guarantee we're in the system app. In fact, I suspect we may be in the homescreen app when these methods are called. If we need to be in the system app then the method should first call marionette.switch_to_frame() like we do in a number of other places. Previously this would have worked because we're running in chrome context, which itself is a privileged space - is it not possible to retain access to mozIccManager from chrome context?
(In reply to Dave Hunt (:davehunt) from comment #10)
> Previously this would have worked
> because we're running in chrome context, which itself is a privileged space
> - is it not possible to retain access to mozIccManager from chrome context?

Yes, I don't understand this either. When you have chrome privileges, shouldn't you be able to access this object?
Flags: needinfo?(htsai)
Flags: needinfo?(bugs)
I don't know how FFOS privileges map to chrome code JS privileges.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #12)
> I don't know how FFOS privileges map to chrome code JS privileges.

Who would know? It seems to me that mozIccManager should be accessible from chrome code.
ehsan or jonas perhaps.
Jonas, do you know the answer to comment 11?
Flags: needinfo?(jonas)
Flags: in-qa-testsuite?
(In reply to Martijn Wargers [:mwargers] (QA) from comment #13)
> (In reply to Olli Pettay [:smaug] from comment #12)
> > I don't know how FFOS privileges map to chrome code JS privileges.
> 
> Who would know? It seems to me that mozIccManager should be accessible from
> chrome code.

Sorry I don't know the exact mapping, either.
Flags: needinfo?(htsai)
Is this code running in a child process? We do limit what child processes can do for security reasons. In general we don't expect chrome code (other than code used to implement DOM APIs) to run in child processes. I.e. chrome "pages" are not expected to work in child processes.
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #17)
> Is this code running in a child process? We do limit what child processes
> can do for security reasons. In general we don't expect chrome code (other
> than code used to implement DOM APIs) to run in child processes. I.e. chrome
> "pages" are not expected to work in child processes.

Yes, apparently it is running in a child process. Every app is running in a child process, right? Even the system app, right? 
If I understand this correctly, self.marionette.set_context(self.marionette.CONTEXT_CHROME) is not really reliable, since this switches to the chrome context of the child process, for which chrome code is not necessarily expected to work, according to comment 17.

Dave, what should be done here, you think? Just remove the CONTEXT_CHROME switching? Or should we somehow switch to a parent process (is that possible?) with CONTEXT_CHROME?
Flags: needinfo?(dave.hunt)
B2G runs the system app in the parent process. I don't know if the test harness runs the system app in the parent or child.

If the test harness runs the system in a child process, then we should fix that since it's not expected that the system app will work correctly in a child process.
(In reply to Jonas Sicking (:sicking) from comment #19)
> If the test harness runs the system in a child process, then we should fix
> that since it's not expected that the system app will work correctly in a
> child process.

I can't imagine that the system app is run in a child process, what makes you think that?
Comment on attachment 8566059 [details] [review]
[gaia] mwargers:moziccmanager > mozilla-b2g:master

This fixes the failures in test_import_contacts_from_SIM and test_import_edit_export_contact.
But I have questions:
- It seems that all calls of self.marionette.set_context(self.marionette.CONTEXT_CHROME) seem to have to be removed, since comment 17 states that chrome "pages" are not expected to work in child process. Do you think we should remove all of them?
- self.marionette.switch_to_frame() switches to the system app, right? If that is correct and the system app is in the parent process, then this code should work, no? self.marionette.switch_to_frame() - self.marionette.set_context(self.marionette.CONTEXT_CHROME) - self.marionette.execute_script("window.navigator.mozIccManager, etc. But this doesn't work. mozIccManager returns undefined here.

Btw, getSIMContacts seems to only remove the contacts-read permission? Why does it do that?
http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_data_layer.js#164
Attachment #8566059 - Flags: review?(dave.hunt)
(In reply to Martijn Wargers [:mwargers] (QA) from comment #21)
> I can't imagine that the system app is run in a child process, what makes
> you think that?

Your comment below made me want to double-check:

(In reply to Martijn Wargers [:mwargers] (QA) from comment #18)
> Yes, apparently it is running in a child process. Every app is running in a
> child process, right? Even the system app, right? 


But no, it is not expected that chrome pages will run correctly in a child process.
Comment on attachment 8566059 [details] [review]
[gaia] mwargers:moziccmanager > mozilla-b2g:master

The problem with switching to the system app is that the calling method needs to know we've switched and has to take care of switching back to the displayed app (and potentially any nested frames). We've made this concession before so I'm not too concerned, but it would be great if we could somehow fork our Marionette session and take care of the switching and executing JS without having to switch back.

I think we may need to switch to the system app in a couple of extra places, but if not please feel free to merge this.
Flags: needinfo?(dave.hunt)
Attachment #8566059 - Flags: review?(dave.hunt) → review+
(In reply to Jonas Sicking (:sicking) from comment #23)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #21)
> > I can't imagine that the system app is run in a child process, what makes
> > you think that?
> 
> Your comment below made me want to double-check:

Note, the word "apparently" that I used. I don't really know much at all how b2g internals work. That's why I'm asking so many questions.

Now I have this question: 
self.marionette.switch_to_frame() -> switches to the system app (which is parent process)
self.marionette.set_context(self.marionette.CONTEXT_CHROME) -> switches to the chrome context
return self.marionette.execute_script("window.navigator.mozIccManager") returns undefined

If I understand correctly, window.navigator.mozIccManager should be accessible, because I'm in the parent process (system app), using the chrome context.
But I can't access it. In fact, not one property/method of window.navigator seems there. Also, in webIDE, I'm not able to see any properties/methods of window.navigator.
Shouldn't I be able to access all properties/methods of window.navigator here in this case?
Jonas, see question in comment 25.
Flags: needinfo?(jonas)
Removed the exclude parts in the manifest.ini for the disabled tests in the pull request and kicked of a test run on Jenkins: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/706/
Assignee: nobody → martijn.martijn
Jenkins test run all passed, to adding checkin-needed.
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
I know very little about how marionette works, so I'm not really sure what
self.marionette.set_context(self.marionette.CONTEXT_CHROME) does.

But there are two separate "pages" in the parent process. Thus two separate 'window' objects. There's an outermost chrome page, which is a mostly empty page which mainly is just used to launch the system app. (This page used to use XUL, but I think that fabrice changed it to use HTML. Either way it's running with chrome privileges).

This chrome page contains an <iframe>. The system app runs inside this <iframe>. Very similar to how on desktop there's a chrome page which contains an <iframe> which renders the actual webpages.

It would not surprise me if the chrome page does not have access to the mozIccManager API. We generally don't do anything at all inside the chrome page, so there's no reason for it to have access to any special APIs.

But the system app, i.e. the page inside the <iframe>, should have access to the mozIccManager API.

Does that answer your question?
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #30)
> I know very little about how marionette works, so I'm not really sure what
> self.marionette.set_context(self.marionette.CONTEXT_CHROME) does.

I think it's setting the context to the chrome context, e.g. any javascript is carried out in the chrome context of the current iframe.

> It would not surprise me if the chrome page does not have access to the
> mozIccManager API. We generally don't do anything at all inside the chrome
> page, so there's no reason for it to have access to any special APIs.

Ok, I'm just worried that more APIs will not be accessible in the future in the chrome context and it seems that our current Gaia UI code is relying for that to work.
From what I now understand, we shouldn't have to use this switching to chrome context at all. I guess it is used as a cheap way to always be sure that one can access these APIs, instead of having to figure out if the iframe has access to it (certified/privileged/not privileged app).

> But the system app, i.e. the page inside the <iframe>, should have access to
> the mozIccManager API.
> 
> Does that answer your question?

Yes, I think it does, thanks. It certainly helps.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #31)
> > It would not surprise me if the chrome page does not have access to the
> > mozIccManager API. We generally don't do anything at all inside the chrome
> > page, so there's no reason for it to have access to any special APIs.
> 
> Ok, I'm just worried that more APIs will not be accessible in the future in
> the chrome context and it seems that our current Gaia UI code is relying for
> that to work.

FWIW, I don't think this is accurate. Gaia UI code does not rely on these APIs working in chrome context. The tests might rely on that, but I'm fairly sure that Gaia itself does not. So ideally the tests wouldn't either.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: