997791 - Investigate and fix tests fail to enable wifi during setup

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Florin Strugariu [:Bebe]]

Description:
Seems that a group of tests are failing because the setup fails to enable wifi

Environment:
Gaia      dadf0e60a6421f5b57ee9fc536c6617212805c19
Gecko     https://hg.mozilla.org/mozilla-central/rev/c55dfb01a027
BuildID   20140417040206
Version   31.0a1
ro.build.version.incremental=324
ro.build.date=Thu Dec 19 14:04:55 CST 2013


Traceback (most recent call last):
Traceback (most recent call last):
File "/var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.2/.env/local/lib/python2.7/site-packages/marionette_client-0.7.5-py2.7.egg/marionette/marionette_test.py", line 145, in run
self.setUp()
File "/var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.2/tests/python/gaia-ui-tests/gaiatest/tests/functional/settings/test_settings_power_save_mode.py", line 17, in setUp
self.data_layer.connect_to_wifi()
File "/var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.2/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 269, in connect_to_wifi
self.enable_wifi()
File "/var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.2/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 258, in enable_wifi
result = self.marionette.execute_async_script("return GaiaDataLayer.enableWiFi()", special_powers=True)
File "/var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.2/.env/local/lib/python2.7/site-packages/marionette_client-0.7.5-py2.7.egg/marionette/marionette.py", line 1162, in execute_async_script
filename=os.path.basename(frame[0]))
File "/var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.2/.env/local/lib/python2.7/site-packages/marionette_client-0.7.5-py2.7.egg/marionette/marionette.py", line 613, in _send_message
self._handle_error(response)
File "/var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.2/.env/local/lib/python2.7/site-packages/marionette_client-0.7.5-py2.7.egg/marionette/marionette.py", line 662, in _handle_error
raise ScriptTimeoutException(message=message, status=status, stacktrace=stacktrace)
ScriptTimeoutException: ScriptTimeoutException: timed out


Reproducible manually: NO

Comment 1

[Florin Strugariu [:Bebe]]

Attachment: logcat of the issue

Eric can you take a look over this issue pls

Comment 2

[Zac C (:zac)]

I'mma gonna look at this right now because it seems quite urgent.

Comment 3

[Zac C (:zac)]

I can't fix this, I don't know what's wrong.

It fails enabling wifi. The setting (wifi.enabled) is set but the WifiManager never seems to pick it up and switch itself to enabled. As if the SettingsObserver never noticed the change of setting. In test_settings_powersave_mode if you comment out disable_wifi and connect_to_cell_data from the setUp steps then the test will pass fine which seems to point to it being a problem with disable and re-enable not working.

I guess this is something like the other problem we had with airplane mode recently.

Comment 4

[Zac C (:zac)]

I also added
self.device_manager.removeDir('/data/misc/wifi/wpa_supplicant.conf')
into the cleanup_data method as it occsaionally seems to fail to forget the networks during the restart.

Comment 5

[Stephen Donner [:stephend] Not actively reading bugmail]

Dimi, can you help us with this one?  Eric or someone from Brian's team should be able to help you get set up; thanks in advance!

Comment 6

[Eric Chang [:ericcc] [:echang]]

I will check with Dimi on this one.

Comment 7

[Dimi Lee [:dimi]]

Attachment: Patch for fixing enable disable testcase failure

Hi Vincent,
  This is related to the bug fixing GAIA & GECKO enable or disable infinite loop.
In testcase, following is the procedure:
1. Wifi is up after rebooting , set ignore enable to True
2. Wifi disable
3. Wifi enable, check ignore enable flag is true so do nothing

Comment 8

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8408771 [details] [diff] [review]
Patch for fixing enable disable testcase failure

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

Redirect the feedback request to Chuck. :-)

Comment 9

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8408771 [details] [diff] [review]
Patch for fixing enable disable testcase failure

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

I think we can't change in this way, because those flags are preventing another settings change is applied before the "echo" coming from system app.
Like:
Wifi Enable(Settings app) -> Wifi Disable(Some unexpected behavior) -> Wifi Enable(echo from System app) -> Wifi Disable(echo from system app)

In such case, we have to ignore both next incoming enable and disable settings(echo from system app).

The problem seems to be there's no System app is Gaia UI Test to send the echo, which is not expected in normal usage.
Since we can't detect this in gecko, I am thinking of changing Gaia UI Test to make it behave like there's a system app.

Comment 10

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Pull Request

Comment 11

[Zac C (:zac)]

Comment on attachment 8408823 [details]
Pull Request

I will pass this onto Bebe to test the functionality.

Chuck, we definitely have the System app running when the UI Tests are running. We could set WiFi by some other method if it were preferable.

It seems a bit unusual (to  my mind) to have to set the setting twice but I don't think it would have any negative effect on the UI tests.

Comment 12

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Zac C (:zac) from comment #11)
> Comment on attachment 8408823 [details]
> Pull Request
> 
> I will pass this onto Bebe to test the functionality.
> 
> Chuck, we definitely have the System app running when the UI Tests are
> running. We could set WiFi by some other method if it were preferable.
> 

Yes, you are right. Then this is not cause of the problem.
I noticed that if I run the test continuously, the ignored settings might be enable or disable.
This makes be think that maybe the ignore flag is left by previous test case, maybe system app doesn't have time to set settings or something like that.

Comment 13

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Sorry I memory massed up system app and settings app, the echo setting is done by settings app, not system app.
So gecko doesn't have to ignore next settings, instead, settings app should prevent set same value to settings.
I'll fixed them ASAP.

Comment 14

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Remove ignore settings flags.

Comment 15

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Regression of Bug 989717

Comment 16

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8408823 [details]
Pull Request

Connectivity module of settings app will tend to set wifi.enabled after receives wifi enable/disable event, this behavior will cause infinite loop between gecko and gaia.
We tend to resolve the loop in gecko, but it turns out that gecko can't handle rapid settings change like bug 989717.
So it might be better to break the look in Connectivity itself.

Current pull request only checks value of |wifi.enable| before set it, but rapid setting change in |tethering.wifi.enabled| still triggers the loop.

Maybe wifi hotspot also need to block enable button before enable/disable is done.

Comment 17

[Robert Chira [:RobertC]]

This issue was also reproduced on 1.4 with automation.

Build info:
Gaia      97b8f9a0a2bd82572422dd84f84016df0e7f56c8
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/e43434b6dd4e
BuildID   20140418000201
Version   30.0a2
ro.build.version.incremental=eng.tclxa.20131223.163538
ro.build.date=Mon Dec 23 16:36:04 CST 2013

test_settings_airplane_mode.py fails intermittently with the same issue.

Comment 19

[Nicolas B. Pierron [:nbp]]

FYI, AreWeFastYet stopped working after this merge:
  http://hg.mozilla.org/integration/mozilla-inbound/rev/43fc4f8b6d49

This merge has some modification in dom/wifi/WifiWorker.js.
Could we just backout Bug 989717?

Comment 20

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Since I have to find another way to resolve bug 989717 and the regression blocks the test, I think backout is a good solution for this bug.

Comment 21

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8408913 [details] [diff] [review]
Remove ignore settings flags.

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

Per our discussion offline with Arthur, we could manipulate request queue to fix this. 
Cancel the review request, and wait for follow up patches.

Comment 22

[Arthur Chen [:arthurcc][inactive after 6/30]]

Clear ni? per our offline discussion earlier.

Comment 23

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Remove ignore settings flags.

Comment 24

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Apply request queue optimization.

Only preserve latest command in command queue, to prevent self-loop and fasten the operation.

Comment 25

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Gecko patches can fix this bug, but it can't fully fix bug presented in bug 989717.
After more experiment, I concluded that bug 989717 has to be fixed both on gecko and gaia side, as provided in this bug.

Comment 26

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8409632 [details] [diff] [review]
0002. Apply request queue optimization.

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

Looks good.

Comment 27

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8408823 [details]
Pull Request

Hi Arthur,
  I found that gaia patch is still required to prevent the command loop, as comment 25. Please review this, thanks!

Comment 28

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

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

Comment 29

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8408823 [details]
Pull Request

Looks good to me. r=me with the nits addressed, thanks!

Comment 30

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8408823 [details]
Pull Request

Sorry I had to cancel the r+ as it involves more than I thought.

Comment 31

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8408823 [details]
Pull Request

r=me. Please create another PR for v1.3T using Settings.getSettings as we don't have SettingsCache in v1.3T. Thanks!

Comment 32

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Pull Request (1.3T)

Comment 33

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8409631 [details] [diff] [review]
0001. Remove ignore settings flags.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 989717
User impact if declined: Can't change Wifi setting after some STR
Testing completed (on m-c, etc.): m-c, 1.3T
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: No

Comment 34

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8409632 [details] [diff] [review]
0002. Apply request queue optimization.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 989717
User impact if declined: Can't change Wifi setting after some STR
Testing completed (on m-c, etc.): m-c, 1.3T
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: No

Comment 35

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8408823 [details]
Pull Request

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 989717
User impact if declined: Must be patched along with gecko part, or changing wifi setting will lead to infiniate loop
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): No
String or IDL/UUID changes made by this patch: No

Comment 36

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

I think they are ready to go, note that gecko patch and gaia patch need to be merged at same time.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9cd74c2fa857
https://hg.mozilla.org/integration/b2g-inbound/rev/b44b035604b4

Master: https://github.com/mozilla-b2g/gaia/commit/32eb30676bb427c3df36be5396f2b739b4291bdf

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9cd74c2fa857
https://hg.mozilla.org/mozilla-central/rev/b44b035604b4

Comment 39

[bhavana bajaj [:bajaj]]

Comment on attachment 8408823 [details]
Pull Request

MAking an exception on 1.4 approval's here as this is a regression and needs to be fixed based on the STR

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/27e349cd7652
https://hg.mozilla.org/releases/mozilla-aurora/rev/d118d3cb2e53

v1.4: https://github.com/mozilla-b2g/gaia/commit/0a299a0f0ef33ed1dce7eb50ab900855b4d8369a

Comment 41

[[:fabrice] Fabrice Desré]

(In reply to Chuck Lee [:chucklee] from comment #36)
> I think they are ready to go, note that gecko patch and gaia patch need to
> be merged at same time.

Can you provide a 1.3t pull request for the gaia part?

Comment 42

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Fabrice Desré [:fabrice] from comment #41)
> (In reply to Chuck Lee [:chucklee] from comment #36)
> > I think they are ready to go, note that gecko patch and gaia patch need to
> > be merged at same time.
> 
> Can you provide a 1.3t pull request for the gaia part?

It's attachment 8410864 [details] [review], or I have to request approval for that pull request?

Comment 43

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Patch for 1.3T is blocked by bug 999388, please hold the check-in until that bug is resolved by partner.
Or this patch will cause another regression.

Comment 44

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Sorry to backout this patch (1e8ce16a1e185b11470f0595c27b8c9703e371b2), Chucklee. 

In v1.4, we didn't use requirejs in Settings app yet. 

Right now, with this patch in v1.4, connectivity.js is broken and wifi related functionalities are not able to use.

Please provide another patch or modify v1.3T patch to make it work in v1.4 ! Thanks :)

Comment 45

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8408823 [details]
Pull Request

We should use pull request for v1.3t to gaia v1.4 instead of this one, per comment 44.

Comment 46

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8410864 [details] [review]
Pull Request (1.3T)

Request approval per comment 44.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 989717
User impact if declined: Must be patched along with gecko part, or changing wifi setting will lead to infiniate loop
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): No
String or IDL/UUID changes made by this patch: No

Comment 48

[Jason Smith [:jsmith]]

Guys - what happened here? Some confusion on backouts & landings has resulted in the patches in this bug causing wifi to be entirely broken. Please back everything

Comment 49

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #48)
> Guys - what happened here? Some confusion on backouts & landings has
> resulted in the patches in this bug causing wifi to be entirely broken.
> Please back everything

Please backout everything out.

Comment 50

[Jason Smith [:jsmith]]

Okay - dug into this - the pvtbuild we have right now does not contain EJ's backout. I'll ask for a respin.

Comment 51

[Ken Chang[:kenkai]]

Before this patch is landed in 1.3T, we need partner to fix bug 999388 first.

Comment 52

[Vincent Chang[:vchang][changyihsin]]

Hi bajaj, can you help to approve approval-gaia-v1.4 per comment 46's request again ? It seems that Chunk requested gaia-v1.4 approval for wrong patch and caused the backout. Per comment 36, the gecko patch has landed to v1.4 and we need to land gaia patch as well.

Comment 53

[bhavana bajaj [:bajaj]]

(In reply to Vincent Chang[:vchang] from comment #52)
> Hi bajaj, can you help to approve approval-gaia-v1.4 per comment 46's
> request again ? It seems that Chunk requested gaia-v1.4 approval for wrong
> patch and caused the backout. Per comment 36, the gecko patch has landed to
> v1.4 and we need to land gaia patch as well.

Please make sure to land this and help verify the 1.4 build as there has been a backout situation already due to a landing in this bug. Thanks!

Comment 54

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to bhavana bajaj [:bajaj] from comment #53)
> (In reply to Vincent Chang[:vchang] from comment #52)
> > Hi bajaj, can you help to approve approval-gaia-v1.4 per comment 46's
> > request again ? It seems that Chunk requested gaia-v1.4 approval for wrong
> > patch and caused the backout. Per comment 36, the gecko patch has landed to
> > v1.4 and we need to land gaia patch as well.
> 
> Please make sure to land this and help verify the 1.4 build as there has
> been a backout situation already due to a landing in this bug. Thanks!

I have tested on Unagi v1.4. Thanks~

Comment 55

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

We have tested tarako after bug 998326 and it seems works fine.
So please checking patches for Gaia v1.4 and Gecko/Gaia v1.3t, thanks

Comment 56

[[:fabrice] Fabrice Desré]

Merged on 1.3t:
https://github.com/mozilla-b2g/gaia/commit/b2c34706b87c85903b4b3083dcf4941475d12963

Comment 57

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/b1242f33981024de59b8b4c26bacff8b876211b1

Fabrice, are you uplifting the Gecko patches to 1.3T as well?

Comment 58

[James Zhang (Spreadtrum)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #57)
> v1.4:
> https://github.com/mozilla-b2g/gaia/commit/
> b1242f33981024de59b8b4c26bacff8b876211b1
> 
> Fabrice, are you uplifting the Gecko patches to 1.3T as well?

Comment 59

[James Zhang (Spreadtrum)]

We have no permission to land patch to gecko, Fabrice, please help.

Comment 60

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/bc7b010f9617
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/8c37369ca7b7

Comment 61

[Michael Henretty [:mikehenrty][:mhenretty]]

Fixed on 1.3t based on comment 60 and comment 56.