Behavior change on bring back the browser from back end( from tab page to opened page), and this will cause the gaia_apps.js check app launch mechanism failed.

RESOLVED FIXED

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sho, Assigned: martijn.martijn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments)

MTBF will raise timeout exception below, when we run the browser test case under browser app is already on device background:

raceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/shako/PycharmProjects/MTBF-Driver/mtbf-driver-env/local/lib/python2.7/site-packages/marionette_driver-0.13-py2.7.egg/marionette_driver/marionette.py", line 1551, in execute_async_script
    rv = self._send_message("executeAsyncScript", body, key="value")
  File "/home/shako/PycharmProjects/MTBF-Driver/mtbf-driver-env/local/lib/python2.7/site-packages/marionette_driver-0.13-py2.7.egg/marionette_driver/decorators.py", line 36, in _
    return func(*args, **kwargs)
  File "/home/shako/PycharmProjects/MTBF-Driver/mtbf-driver-env/local/lib/python2.7/site-packages/marionette_driver-0.13-py2.7.egg/marionette_driver/marionette.py", line 711, in _send_message
    self._handle_error(resp)
  File "/home/shako/PycharmProjects/MTBF-Driver/mtbf-driver-env/local/lib/python2.7/site-packages/marionette_driver-0.13-py2.7.egg/marionette_driver/marionette.py", line 752, in _handle_error
    raise errors.lookup(error)(message, stacktrace=stacktrace)
marionette_driver.errors.ScriptTimeoutException: ScriptTimeoutException: timed out


and the root cause is come from the gaia_apps.js, the reference code as below:
launch: function(app, appName, launchPath, entryPoint) {
    if (app) {
      let origin = app.origin;

...
              return GaiaApps.getDisplayedApp().src == (origin + launchPath);
...
  },
Build ID               20150907110522
Gaia Revision          891798f1e345bc2b69e71de42bd524a90b1745c4
Gaia Date              2015-09-07 02:49:56
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/5fe9ed3edd6811a662d40d05e37b0d66e9520d82
Gecko Version          43.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150907.102632
Firmware Date          Mon Sep  7 10:26:39 UTC 2015
Bootloader             s1
Depends on: 1188926
No longer depends on: 1188926
Depends on: 1188926
Shako, which exact MTBF testcase does this error occur?
Flags: needinfo?(sho)
With this test, I can reproduce this bug.
But the problem is that I can also reproduce it with the pull request from bug 1188926 backed out.

Nonetheless, this should work anyhow.
Attachment #8660669 - Attachment mime type: text/x-python-script → text/plain
Bug 1169010 changed behavior (checked in 2015-08-27 02:05:17 PDT) where tapping on the Browser icon in homescreen now opens the existing browser app, instead of opening a new browser window with newtab.html.
This is why the attached test is failing. But this test is failing with and without the pull request from bug 1188926 applied.
What I understand from the MTBF test, it is only happening with the pull request from bug 1188926 applied. But I don't know how that is happening, so I would love to see that test.
Comment on attachment 8660967 [details] [review]
[gaia] mwargers:1203060 > mozilla-b2g:master

This fixes the timeout.
But like I said, bug 1169010 changed the behavior to reuse the existing browser window. I don't know if that's desired. Anyway, that's not part of this bug or what bug 1188926 has changed.

Does this fix the mtbf issue?
Attachment #8660967 - Flags: review?(sho)
Attachment #8660967 - Flags: review?(pyang)
Hi Martijn,

the MTBF test is "test_mtbf_browser_cell_data.py". Basically it pretty similar to test_browser_cell_data.py in gaiatest. And yes, the patch helps.. 

Thanks!!!! it helps a lot!!

But I still need to modify the MTBF test case, because the MTBF test case have to deal with different behavior when the browser already in background or not.
Flags: needinfo?(sho)
I'm not sure in what way you need to have the MTBF test case adjusted. If you need it, we could add a way of creating a new browser instances, no matter if there are already existing browser instances out there.

I noticed that test_mtbf_browser_cell_data.py still uses wait_for_element_present, I removed that in bug 1200392. If you upgrade gaiatest, then I guess you'll run into this problem.

I get the impression that this is a regression from bug 1169010 instead of bug 1188926. Can anyone confirm?
Comment on attachment 8660967 [details] [review]
[gaia] mwargers:1203060 > mozilla-b2g:master

Paul, are you able to review this? No-Jun, I don't know who I can get review on this. Johan is for 2 weeks on holiday and Dave Hunt preferred to not review this.

The test I added would ideally be a unit test, but I need the browser app object and I don't think that should be part of unit tests, app object usage.
The weird !displayedApp.manifestURL usage is because of the behavior I described in bug 1169010, comment 12.

Let me know if you have questions.
Attachment #8660967 - Flags: review?(sho) → review?(npark)
Btw, if this is a regression from bug 1188926, then it should block that bug.
Blocks: 1188926
No longer depends on: 1188926
Comment on attachment 8660967 [details] [review]
[gaia] mwargers:1203060 > mozilla-b2g:master

This looks fine to me from what I can see (it's the one we discussed yesterday about using .origin when manifestURL is null), minus the Shako's comment in the PR. Assuming that you'll incorporate his suggestion, I'll r+ it.
Attachment #8660967 - Flags: review?(npark) → review+
Thanks, sorry, I missed Shako's comment in the PR, I'll respond.
Assignee: nobody → martijn.martijn
Comment on attachment 8660967 [details] [review]
[gaia] mwargers:1203060 > mozilla-b2g:master

see also https://github.com/mozilla-b2g/gaia/pull/31841
line 284 & 291 log by origin but we added manifestURL and should be mentioned as well.
Comment on attachment 8660967 [details] [review]
[gaia] mwargers:1203060 > mozilla-b2g:master

No-Jun, sorry, but could you take another look, I made some changes to the test. gaia_app.js is more or less the same (only changed console.log to include manifestURL).

Shako, this is what you wanted, right?

In the end, I think this artificial distinction between Browser and Search needs to go. There isn't really a difference here, except that Search is a browser window that's still on the start page.
But I'll file that as a follow-up bug.

One thing that I didn't test here is the behavior on tapping on the brower icon in the homescreen. That should respond the same as the .launch() method, I think.
One other thing that we could test is the behavior when we have multiple browser windows open, which one gets displayed when calling .launch() (I think it's the most recent opened browser window that has not the default start page and it seems that you can only have one browser window open with the default start page, btw).
I should also file a follow-up bug on that.
Attachment #8660967 - Flags: review?(sho)
Attachment #8660967 - Flags: review?(npark)
Attachment #8660967 - Flags: review+
(In reply to Martijn Wargers [:mwargers] (QA) from comment #14)
> In the end, I think this artificial distinction between Browser and Search
> needs to go. There isn't really a difference here, except that Search is a
> browser window that's still on the start page.
> But I'll file that as a follow-up bug.

I filed bug 1205262 for this.

> One other thing that we could test is the behavior when we have multiple
> browser windows open, which one gets displayed when calling .launch() (I
> think it's the most recent opened browser window that has not the default
> start page and it seems that you can only have one browser window open with
> the default start page, btw).
> I should also file a follow-up bug on that.

I filed bug 1205264 for this.
Btw, I don't think our code currently can handle multiple browser windows at all, I mentioned that in bug 1205264, comment 2.
Note, while improving the test I noticed and filed bug 1205293.
Comment on attachment 8660967 [details] [review]
[gaia] mwargers:1203060 > mozilla-b2g:master

LGTM
Attachment #8660967 - Flags: review?(npark) → review+
Yes, that's what I want. 
And I agree that the Browser and Search seems don't have too much difference.. 

But the traditional behavior of go_to_url in browser will validate the input_url and the diplayed_app.name. However when you give keyword as input_url the validation will failed, and I filed a bug for it bug 1205523. In this bug you might also need to modify the app.py under search folder to handle different type of input_url...
Attachment #8660967 - Flags: review?(sho) → review+
Comment on attachment 8660967 [details] [review]
[gaia] mwargers:1203060 > mozilla-b2g:master

lgtm
Attachment #8660967 - Flags: review?(pyang) → review+
Created an adhoc job for smoketest with this pull request: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/890/
If that one looks reasonable, I'll merge.
Adhoc job looked good, so merged: https://github.com/mozilla-b2g/gaia/commit/60011f2947e31a92f064502784d7e1b8fc26898c

You need to run python setup.py develop in the tests/python/gaia-ui-tests of your gaia directory to get these changes.

I made sure that jshint tests/atoms/gaia_apps.js didn't give any problems, because Treeherder try seems still busted.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
And I'm making this block bug 1169010, because I think this is a regression from that bug, not from bug 1188926.
Blocks: 1169010
No longer blocks: 1188926
Duplicate of this bug: 1205207
You need to log in before you can comment on or make changes to this bug.