1056493 - [Dolphin][Hamachi][v1.4]No icons show for several seconds when back to homescreen from contacts

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[yang.zhao]

Attachment: 2014-08-20-23-15-23.png

SRT:
1.Dolphin or Hamachi with v1.4
2.Push some data by using |./reference-workload/makeReferenceWorkload.sh| under monkey.test folder.
3.Make sure no apps are running in background
4.launch dialer app, then back to homescreen by clicking home button
5.launch sms app,then back to homescreen by clicking home button
6,launch contacts app,before all contacts are loaded,click home button to back to homescreen
After step 6, you'll see the homescreen shows no icons for several seconds, then back to normal.
Check the pid for homescreen, you'll see that there's no change in pid.

Comment 1

[yang.zhao]

Check the log below,you'll see it takes a long time for homescreen to receive and handle visibilitychange event.

>Launch contacts app
08-20 01:20:45.781 E/GeckoConsole( 2313): Content JS LOG at app://system.gaiamobile.org/js/app_window.js:651 in aw__handle_mozbrowservisibilitychange: james->mozbrowservisibilitychange,app=Homescreen,type=background
>Press home button
08-20 01:20:46.591 E/GeckoConsole( 2313): Content JS LOG at app://system.gaiamobile.org/js/app_window_manager.js:409 in awm_handleEvent: james->##################################HOME button
>system receive mozbrowservisibilitychange event
08-20 01:20:47.661 E/GeckoConsole( 2313): Content JS LOG at app://system.gaiamobile.org/js/app_window.js:651 in aw__handle_mozbrowservisibilitychange: james->mozbrowservisibilitychange,app=Contacts,type=background
08-20 01:20:50.021 E/GeckoConsole( 2313): Content JS LOG at app://system.gaiamobile.org/js/app_window.js:651 in aw__handle_mozbrowservisibilitychange: james->mozbrowservisibilitychange,app=Homescreen,type=foreground
>homescreen receive visibilitychange background(homescreen->contacts)
08-20 01:20:50.161 E/GeckoConsole( 2369): Content JS LOG at app://homescreen.gaiamobile.org/js/homescreen.js:130 in mozVisChange: james->visibilitychange*****************,hidden=true
>homescreen receive visibilitychange foreground(contacts->homescreen)
08-20 01:20:50.171 E/GeckoConsole( 2369): Content JS LOG at app://homescreen.gaiamobile.org/js/homescreen.js:130 in mozVisChange: james->visibilitychange*****************,hidden=false
>repaint homescreen.It takes 4 sec from pressing home button to repaint homescreen
08-20 01:20:50.231 E/GeckoConsole( 2369): Content JS LOG at app://homescreen.gaiamobile.org/js/homescreen.js:137 in forceRepaint: james->begin toggle displayed

After oberserving the nice pid, it seems that the pid prority of homescreen doesn't change in time after pressing home button.
If I disable OOP for homescreen, the issue won't reproduce.

Could anyone can help on this issue?

Comment 2

[yang.zhao]

[Blocking Requested - why for this release]:

Comment 3

[Evelyn Hung]

Could you give a more accurate number of "several" seconds?

Comment 4

[yang.zhao]

(In reply to Evelyn Hung [:evelyn] from comment #3)
> Could you give a more accurate number of "several" seconds?
Please see comment 1,it takes 4 secs. I also see sometimes it takes nearly 7 secs.

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

The idea I had about this is that it happens because homescreen gets OOM, but if PID doesn't change then that's unlikely.

Comment 6

[James Zhang (Spreadtrum)]

It's not homescreen OOM, the pid doesn't change.

Comment 7

[Evelyn Hung]

A simpler STR:
1. open dialer, then go back home
2. open contacts
3. press home button to go back home

It takes more than 5 sec to load homescreen icons. Luke is investigating.
@Luke, please update bug for your finding. Thanks.

Comment 8

[Evelyn Hung]

(In reply to Evelyn Hung [:evelyn] from comment #7)
> A simpler STR:
> 1. open dialer, then go back home
> 2. open contacts
> 3. press home button to go back home
> 
> It takes more than 5 sec to load homescreen icons. Luke is investigating.
> @Luke, please update bug for your finding. Thanks.

BTW, other app combination is harder to reproduce the issue.

Comment 9

[Luke Chang [:lchang] (inactive)]

Thanks to Kan-Ru's profiling, we found that this issue might be caused by wrong priority value of communications' process.

In Gaia, for now, dialer and contacts are run in the same process (named "communications" in "b2g-info"). After doing the STR in comment 7, the priority ("nice" value) of communications process will become 0 (the highest priority) which is higher than homescreen process. Thus, homescreen will be not able to acquire enough resource to repaint itself if contact app is under heavy load (say, loading bulk photos).

Comment 10

[yang.zhao]

(In reply to Luke Chang [:lchang] from comment #9)
Yes,we also found that the priority of homescreen was not correct after pressing the home button.Could anyone can resolve this issue.This is a bad user experience, I think. Thank you.

Comment 11

[yang.zhao]

Hi,Evelyn
    Could you tell me if this issue has any progress now? Per comment 9, it seems that this is not a gaia issue,right?
    Does anyone still work on this issue? Thank you!

Comment 12

[Danny Liang [:dliang]]

I am doing further investigation and will keep post.

Comment 13

[Danny Liang [:dliang]]

Update some findings about priority of communcations app.
When we launch SMS then back to homescreen, the priority manager is involved and the call stack as following:

#0  (anonymous namespace)::ParticularProcessPriorityManager::ComputePriority (this=0xa9621c50) at /home/dannyliang/work/codes/b2g_dolphin/gecko/dom/ipc/ProcessPriorityManager.cpp:911
#1  0xb53e4da2 in ResetPriorityNow (this=0xa9621c50) at /home/dannyliang/work/codes/b2g_dolphin/gecko/dom/ipc/ProcessPriorityManager.cpp:836
#2  (anonymous namespace)::ParticularProcessPriorityManager::Notify (this=0xa9621c50, aTimer=<optimized out>) at /home/dannyliang/work/codes/b2g_dolphin/gecko/dom/ipc/ProcessPriorityManager.cpp:859

In this case, ParticularProcessPriorityManager::ComputePriority() is called and it returns PROCESS_PRIORITY_FOREGROUND_HIGH due to there is wake lock by communication app. That's why we saw the nice value of communication app is 0 for a long time.

In my opinion, we might need to discuss if priority of application should be increased when wake lock is held.

Comment 14

[Danny Liang [:dliang]]

Update more, this issue only happen when launch dialer then SMS. That's becasue this sequence caused "HasAppType("critical")" is true by Dialer and "mHoldsCPUWakeLock" is true by SMS.

ProcessPriority
ParticularProcessPriorityManager::ComputePriority()
{
  if ((mHoldsCPUWakeLock || mHoldsHighPriorityWakeLock) &&
      HasAppType("critical")) {
    return PROCESS_PRIORITY_FOREGROUND_HIGH;
  }

Comment 15

[Danny Liang [:dliang]]

Attachment: remove_wakelock_check_when_computer_priority.patch

Update WIP patch in gecko side

Comment 16

[Danny Liang [:dliang]]

(In reply to Danny Liang [:dliang] from comment #15)
> Created attachment 8478195 [details] [diff] [review]
> remove_wakelock_check_when_computer_priority.patch
> 
> Update WIP patch in gecko side

In my opinion, it's not good to increase priority by CPUWakeLock. This is just WIP and we might need more discussion for removing this CPUWakelock check. I don't know if any other design concern for this.

Comment 17

[[:fabrice] Fabrice Desré]

Gabriele, any insights there?

Comment 18

[Danny Liang [:dliang]]

Correct my TYPO on commemt 13 and comment 14, it should be CONTACTS, not SMS.

Comment 19

[Luke Chang [:lchang] (inactive)]

Attachment: Pull Request 23304

According to Danny's finding in comment 14, we found that it's a regression of bug 907013. After that bug landed, we set "expecting-system-message" to all appWindow whether or not it really needs. This attribute causes "CPUWakeLock" in Gecko so we encountered this bug.

Since it's a regression, I prefer to fix it in Gaia first. However, there is still a chance that we made some mistakes in calculating priority. I suggest keeping discussing the issues that Danny mentioned in comment 16.


Hi Alive, Could you please help to take a look at this patch? Thanks.

Comment 20

[Evelyn Hung]

Assign to Luke, it's obviously a Gaia regression we need to fix anyway. (comment 19)

For Danny's question in comment 16, we can open a follow up bug to discuss if CPUWakeLock needs to be a reference when calculating priority. I feel it's a broader discussion on reviewing process priority, which we may not want to change it on 1.4 unless it causes other issues.

Comment 22

[Gabriele Svelto [:gsvelto]]

Scratch my last comment about oom_score_adj values, I had a faulty build.

Comment 23

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8478852 [details] [review]
Pull Request 23304

Could you also have a patch to master?
I think long term we are removing expect-system-message ...do we?

Comment 24

[Luke Chang [:lchang] (inactive)]

Hi Alive, 

Thanks for reviewing.

Regarding master branch, there is already a bug 1046033 which attempts to remove this attribute completely from both Gecko and Gaia. Since this bug didn't occur on master branch, I think we may just wait for that bug.

Comment 25

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8478852 [details] [review]
Pull Request 23304

This patch seems to break the unit test. I'll fix it as soon as possible.

Comment 26

[Luke Chang [:lchang] (inactive)]

TBPL passed: https://tbpl.mozilla.org/?rev=34f76c85241dd040738244fdb63b5e79a912c0f7&tree=Gaia-Try

landed on v1.4: https://github.com/mozilla-b2g/gaia/commit/59e74a8adf64946119deee2f96ed79935f3535f7

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This needs to land on all branches even though there were no symptom on these devices.

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8478852 [details] [review]
Pull Request 23304

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Logic error in bug 907013
[User impact] if declined: OOM_ADJ score will be calculated incorrectly. No visible user impact on hi-mem device, but mid- and low-end device will act really quirky.
[Testing completed]: Already landed on v1.4.
[Risk to taking this patch] (and alternatives if risky): little code change; OOM_ADJ parameters might need readjustment after this patch lands.
[String changes made]: None

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

downlifted to master (v2.2): https://github.com/timdream/gaia/commit/9e2a7c2b82c68231c1afcfcdd8ae9011a38ef611

Comment 30

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/3f7967bd6792b7f4e9798816124c1ecfaf0de57c

Comment 31

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/dd9603646f6324617e1efba29eceedb145763368