943317 - [Fugu][Buri] unable to launch any app after return to homescreen in the process of loading one app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[Peipei Cheng (needinfo if you need my action)]

Test build:
Gaia:     264c6044b941437ac3c4b28fe4ca392d2bc78445                           
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/21e2ad082d85   
BuildID   20131126004001                                                      
Version   26.0  

Reproduce steps:
1. connect to wifi
2. Launch Marketplace 
3. Tap on home button and return to homescreen when Marketplace is still loading
4. Try to launch any app
   => no app can be launched. User need to tap on Home button again or lock and unlock screen to workaround this.

in step 2, you can also use other app such as Gallery which take some time to load.

Comment 1

[Jason Smith [:jsmith]]

Does this reproduce on 1.1?

Comment 3

[Natalya Kot [:nkot]]

seems to be a regression within 1.2, cannot reproduce this issue on Buri v1.2 11/02 build

BuildID: 20131102004003
Gaia: 2e5dc6b9600bf2bc2ff3fc359120e2883ef50e38
Gecko: 1ed04b3ea320
Version: 26.0
Firmware Version: v1.2_20131115

Comment 4

[Jason Smith [:jsmith]]

Let's say I wait a little bit after I hit the home button right after launching an app. Can I still launch apps in this case? What I'm trying to find out is if there's targeted STR that requires doing this sequence while the app is loading always or not.

Comment 5

[gbennett]

(In reply to Jason Smith [:jsmith] from comment #4)
> Let's say I wait a little bit after I hit the home button right after
> launching an app. Can I still launch apps in this case? What I'm trying to
> find out is if there's targeted STR that requires doing this sequence while
> the app is loading always or not.

You cannot, also you can repro this issue with an already loaded app instead of only launching it for the first time. Heads up you might be able to get some information from bug https://bugzilla.mozilla.org/show_bug.cgi?id=933561 as that was fixed on 11/21 which is when this last worked this time around.

.:Last Working Build:.
Environmental Variables:
Device: Buri 1.2 comRIL
BuildID: 20131121004002
Gaia: ce276842c9ac1746073271fb736dfdb626a89240
Gecko: 36c4c667b9f2
Version: 26.0
Firmware Version: 20131115

.:First Broken Build:.
Environmental Variables:
Device: Buri 1.2 comRIL
BuildID: 20131122004001
Gaia: 7b24f1d1f167fc40b1e887f4ff3746d383af3c2f
Gecko: 866496ed1f26
Version: 26.0
Firmware Version: 20131115

Comment 6

[Preeti Raghunath(:Preeti)]

koi+ as this is a very basic test case to fail.

Comment 7

[James Zhang (Spreadtrum)]

Add Jesse, pls provide your patch to mozilla.

Comment 9

[James Zhang (Spreadtrum)]

Attachment: 943317.patch

The root cause is when entry apps, home screen disable tap. When press home key quickly, window manager can't trigger the 'visibility change' event, and home screen can't enalbe tap and lunch apps again.

Comment 10

[James Zhang (Spreadtrum)]

Steven, please ask owner to review this critical issue. Thanks.

Comment 11

[Cristian Rodriguez (:crdlc)]

Please do a pull request because I see fails of jslint, 10x

Comment 12

[Cristian Rodriguez (:crdlc)]

And ask to Alive because you're touching the window manager, thanks

Comment 13

[James Zhang (Spreadtrum)]

Add Steven Yang,

Hi Steven, 
As we talking last weekly meeting, we're suffering low network speed, can you help to review and merge by patch?

Hi Cristian,
Can we review by patch? You can add some comment inline.

Comment 14

[Cristian Rodriguez (:crdlc)]

Hi James, I like to see the code and test it in my device so I prefer a pr if you're comfortable. Thanks

Comment 15

[Cristian Rodriguez (:crdlc)]

I added some comments in the patch. Basically, I am wondering why we have to touch the homescreen's code. I mean, the code is ready for this case

// 2. Users click on home button quickly while app are opening
window.addEventListener('hashchange', enableTap);

If this bug is reproducible, my question is: aren't we receiving the home button event? If so, why?

Thanks a lot for working on it

Comment 16

[James Zhang (Spreadtrum)]

(In reply to Cristian Rodriguez (:crdlc) from comment #15)
> I added some comments in the patch. Basically, I am wondering why we have to
> touch the homescreen's code. I mean, the code is ready for this case

// 2.
> Users click on home button quickly while app are opening
> window.addEventListener('hashchange', enableTap);

If this bug is
> reproducible, my question is: aren't we receiving the home button event? If
> so, why?

Thanks a lot for working on it

see comment 9.
We're receiveing the home button event and we can reproduce it with 100% rate.
Window manager should handle the home key properly in window transition.

Comment 17

[Cristian Rodriguez (:crdlc)]

The homescreen is listening for both events, sorry but I don't understand why we have to change the logic in page.js

https://github.com/mozilla-b2g/gaia/blob/v1.2/apps/homescreen/js/page.js#L858

Comment 18

[Jesse]

Cristian,

1) I think the bug should be set to 'GAIA::System' component due to window_manager problems.

  The root cause of the bug is that 'visibilitychange' event is not fired when pressing home button   immediately after clicking an app icon on homescren.

2) The reason why I change the logic in 'page.js' is that I notice the following five lines in 'page.js'.

    // We are going to enable the tapping feature under these conditions:
    // 1. The opened app is in foreground
    document.addEventListener('visibilitychange', enableTap);
    // 2. Users click on home button quickly while app are opening
    window.addEventListener('hashchange', enableTap);

   I thought the second case is a workaround to fix the bug caused by the possibility that 'visibilitychange' event is not fired.

   The workaround is not necessary any more after the 'window_manager' bug is fixed.


3) Another change is an exchange of the following 2 lines to prevent the event is fired before starting to listen.

      icon.app.launch();
      this.disableTap();

4) I would like to know the fails of jslint in the patch code. What tools do you use?

Comment 19

[Cristian Rodriguez (:crdlc)]

> 2) The workaround is not necessary any more after the 'window_manager' bug is fixed.

I think that it is not a workaround. Maybe if users try to open an app but immediately they click on home button, the app shouldn't be launched and then the visibilitychange doesn't be fired but the home button should be dispatched by the homescreen. If this bug is happening is because of the home button event is not received in homescreen app at that moment thought. What do you think Alive?

> 3) Another change is an exchange of the following 2 lines to prevent the event is fired before starting to listen

It makes sense for me

> 4) I would like to know the fails of jslint in the patch code. What tools do you use?

Jslint, when you try to do a commit in gaia, it will be done for free in you machine

Comment 20

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

(In reply to Cristian Rodriguez (:crdlc) from comment #19)
> > 2) The workaround is not necessary any more after the 'window_manager' bug is fixed.
> 
> I think that it is not a workaround. Maybe if users try to open an app but
> immediately they click on home button, the app shouldn't be launched and
> then the visibilitychange doesn't be fired but the home button should be
> dispatched by the homescreen. If this bug is happening is because of the
> home button event is not received in homescreen app at that moment thought.
> What do you think Alive?
> 

Sorry but I would rather not to let homescreen deal with 'quickly tapping' problems. I mean homescreen doesn't need to disable launching after it's launched because it's system's duty to prevent that.

Also in this case I don't think change homescreen's hash is logical because we're just transitioning, nobody could guarant
ee where we are and it's strange to move homescreen to page#1 when app is opening/closing because from the code we don't know 'how immediate' it is.

Comment 21

[Cristian Rodriguez (:crdlc)]

The problem is that the homescreen does not receive the 'home' event and there is a hack with hashchange. From homecreen point of view, when users click on icon, the rest of apps cannot be launched when they try to click some of them at the same time. When the first one is clicked the grid should be disabled until the homescreen is in background and while the app is transitioning. If window manager could do it for homescreen, cool.

Comment 22

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

Lemme clarify: what we could do is put pointer-events: none; on homescreen's iframe's container in system app when it's transitioning. So there might be a "very short" timing before it is covered, but it is really an edge case.

Comment 23

[Jesse]

   Maybe it's a good idea that we do some special work on system app. The system and homescreen app should not be tightly coupled.

   What we should do is simply making homescreen app receive 'visibilitychange' event correctly. It's another thing as to whether we need to prevent double click on homescreen icons.

Comment 24

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

(In reply to Jesse from comment #23)
>    Maybe it's a good idea that we do some special work on system app. The
> system and homescreen app should not be tightly coupled.
> 
>    What we should do is simply making homescreen app receive
> 'visibilitychange' event correctly. It's another thing as to whether we need
> to prevent double click on homescreen icons.

NO! I don't think homescreen needs to know visibilitychange even when the transition is not complete. You would see significant UI flicking!

Comment 25

[Jesse]

All right. I didn't see any flicking after my patch was applied.

Using 'pointer-events' style sounds good.

Feel free to find a better solution.

Comment 27

[James Zhang (Spreadtrum)]

I think we need fix this critical issue before v1.2 branch close.

Comment 29

[Tony Chung [:tchung]]

Here's a screencast on the GP Peak.   It's a 1.2 build.

http://youtu.be/8fvzGK0olvg

Comment 30

[Kai-Chih Hu [:khu]]

Because 1.2 is going to be finished, should we mark this bug from koi+ to 1.3+? Thanks.

Comment 31

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8340611 [details] [diff] [review]
943317.patch

I don't see the solution suggested by Alive. Ask him and me for a review when it is ready, 10x

Comment 32

[Evelyn Hung]

(In reply to Jesse from comment #25)
> All right. I didn't see any flicking after my patch was applied.
> 
> Using 'pointer-events' style sounds good.
> 
> Feel free to find a better solution.

So pointer-events:none should be applied on every iframe before another new opened iframe is transitioning. In this case, we add this CSS rule on homescreen when an app is opening. If user interrupts at this time point (click home button), we will remove this CSS rule immediately while receiving home-button-press event, so homescreen will behave normal.
Before the launching app is ready, homescreen won't be pushed to background(still visible), so it doesn't make sense to file visibilitychange during this period.

I will start working on this bug.

Comment 33

[Evelyn Hung]

(In reply to Kevin Hu [:khu] from comment #30)
> Because 1.2 is going to be finished, should we mark this bug from koi+ to
> 1.3+? Thanks.

I think so.. nominate fugu? because it seems a must-have to fugu.

Comment 34

[Ben Francis [:benfrancis]]

(In reply to Kevin Hu [:khu] from comment #30)
> Because 1.2 is going to be finished, should we mark this bug from koi+ to
> 1.3+? Thanks.

If this was marked as koi+ but 1.2 is "finished" then by definition it can't have been a 1.2 blocker. If it was a blocker we would hold the release until it was fixed.

Should we ship a 1.2.1 to fix this bug or was it decided that it wasn't actually a 1.2 blocker after all?

Comment 35

[Jason Smith [:jsmith]]

(In reply to Ben Francis [:benfrancis] from comment #34)
> (In reply to Kevin Hu [:khu] from comment #30)
> > Because 1.2 is going to be finished, should we mark this bug from koi+ to
> > 1.3+? Thanks.
> 
> If this was marked as koi+ but 1.2 is "finished" then by definition it can't
> have been a 1.2 blocker. If it was a blocker we would hold the release until
> it was fixed.
> 
> Should we ship a 1.2.1 to fix this bug or was it decided that it wasn't
> actually a 1.2 blocker after all?

We aren't done with 1.2. Kevin's comment is incorrect. We're still fixing koi+ blockers until December 20th. On that date we'll evaluate what to do here. As for this bug, this is definitely still a blocker, so moving back to a koi+.

Comment 36

[Evelyn Hung]

(In reply to Jason Smith [:jsmith] from comment #35)
> 
> We aren't done with 1.2. Kevin's comment is incorrect. We're still fixing
> koi+ blockers until December 20th. On that date we'll evaluate what to do
> here. As for this bug, this is definitely still a blocker, so moving back to
> a koi+.

Thanks for clarification. Hopefully I can give a patch by this week.

Comment 37

[Preeti Raghunath(:Preeti)]

Hi Evelyn,

Please provide an ETA for landing?

Comment 38

[Kai-Chih Hu [:khu]]

(In reply to Jason Smith [:jsmith] from comment #35)
> (In reply to Ben Francis [:benfrancis] from comment #34)
> > (In reply to Kevin Hu [:khu] from comment #30)
> We aren't done with 1.2. Kevin's comment is incorrect. We're still fixing
> koi+ blockers until December 20th. On that date we'll evaluate what to do
> here. As for this bug, this is definitely still a blocker, so moving back to
> a koi+.

Thanks for your clarification, Jason. 
So, after 12/20, all opened koi+ blockers should be marked as 1.3+, right?

Comment 39

[Jason Smith [:jsmith]]

(In reply to Kevin Hu [:khu] from comment #38)
> (In reply to Jason Smith [:jsmith] from comment #35)
> > (In reply to Ben Francis [:benfrancis] from comment #34)
> > > (In reply to Kevin Hu [:khu] from comment #30)
> > We aren't done with 1.2. Kevin's comment is incorrect. We're still fixing
> > koi+ blockers until December 20th. On that date we'll evaluate what to do
> > here. As for this bug, this is definitely still a blocker, so moving back to
> > a koi+.
> 
> Thanks for your clarification, Jason. 
> So, after 12/20, all opened koi+ blockers should be marked as 1.3+, right?

I don't think it's that cut and dry. We'll need to evaluate the current blocker list at that point & determine if any of the remaining bugs are still blockers at a high blocking+ bar. Let's plan on reviewing the blocker list after 12/20 to see where we stand.

Note - we should push hard to drive that blocker list count to zero.

Comment 40

[Kai-Chih Hu [:khu]]

(In reply to Jason Smith [:jsmith] from comment #39)
> (In reply to Kevin Hu [:khu] from comment #38)
> > (In reply to Jason Smith [:jsmith] from comment #35)
> > > (In reply to Ben Francis [:benfrancis] from comment #34)
> > > > (In reply to Kevin Hu [:khu] from comment #30)
> > > We aren't done with 1.2. Kevin's comment is incorrect. We're still fixing
> > > koi+ blockers until December 20th. On that date we'll evaluate what to do
> > > here. As for this bug, this is definitely still a blocker, so moving back to
> > > a koi+.
> > 
> > Thanks for your clarification, Jason. 
> > So, after 12/20, all opened koi+ blockers should be marked as 1.3+, right?
> 
> I don't think it's that cut and dry. We'll need to evaluate the current
> blocker list at that point & determine if any of the remaining bugs are
> still blockers at a high blocking+ bar. Let's plan on reviewing the blocker
> list after 12/20 to see where we stand.
> 
> Note - we should push hard to drive that blocker list count to zero.

Yes, that matches my understanding. Thanks. 
By the way, I feel that some drivers may not be aware that we can fix koi+ until 12/20. It would be great if we could have some announcement. Just a suggestion. Thank you.

Comment 41

[Evelyn Hung]

(In reply to Alive Kuo [:alive][NEEDINFO] from comment #22)
> Lemme clarify: what we could do is put pointer-events: none; on homescreen's
> iframe's container in system app when it's transitioning. So there might be
> a "very short" timing before it is covered, but it is really an edge case.

Well, I tried but the *very short* time is long enough to double click or tap on two icons simultaneously. So I decide to drop my proposal in comment 32, and set a timer on disableTap instead, it's simpler.

Comment 42

[Evelyn Hung]

(In reply to Preeti Raghunath(:Preeti) from comment #37)
> Hi Evelyn,
> 
> Please provide an ETA for landing?

I think I can have it done this week. (12/15)

Comment 43

[Evelyn Hung]

Attachment: point to https://github.com/mozilla-b2g/gaia/pull/14594

In this patch, I just add a time to expire tapping lock. I think it's a safer solution to fix this problem, instead of adding some state change and checking.
Please note this patch is for v1.2. Can be uplift to master (after conflict resolved) if reviewer suggests.

Comment 44

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8346351 [details] [review]
point to https://github.com/mozilla-b2g/gaia/pull/14594

Hi, it was our first version (you can see the vi-train branch). I did some important suggestions in github. Please ask again for a new review when they are addressed. Thanks

Comment 45

[Cristian Rodriguez (:crdlc)]

(In reply to Evelyn Hung [:evelyn] from comment #43)
> Created attachment 8346351 [details] [review]
> point to https://github.com/mozilla-b2g/gaia/pull/14594
> 
> In this patch, I just add a time to expire tapping lock. I think it's a
> safer solution to fix this problem, instead of adding some state change and
> checking.
> Please note this patch is for v1.2. Can be uplift to master (after conflict
> resolved) if reviewer suggests.

The reviewer cannot suggest this kind of thinks. If this is reproducible in master, you should make a pull request for this one and after merging in master, some driver should uplift to v1.2. If there will be conflicts then they will request you to fix them for the cherry-pick. Thanks a lot

Comment 46

[Evelyn Hung]

(In reply to Cristian Rodriguez (:crdlc) from comment #45)
> (In reply to Evelyn Hung [:evelyn] from comment #43)
> > Created attachment 8346351 [details] [review]
> > point to https://github.com/mozilla-b2g/gaia/pull/14594
> > 
> > In this patch, I just add a time to expire tapping lock. I think it's a
> > safer solution to fix this problem, instead of adding some state change and
> > checking.
> > Please note this patch is for v1.2. Can be uplift to master (after conflict
> > resolved) if reviewer suggests.
> 
> The reviewer cannot suggest this kind of thinks. If this is reproducible in
> master, you should make a pull request for this one and after merging in
> master, some driver should uplift to v1.2. If there will be conflicts then
> they will request you to fix them for the cherry-pick. Thanks a lot

It's very hard to reproduce on master, but from code logic, it *should* be reproducible. That's why I'd like to hear app owner's suggestion.

Comment 47

[Evelyn Hung]

Comment on attachment 8346351 [details] [review]
point to https://github.com/mozilla-b2g/gaia/pull/14594

Thanks for detailed review. Github comment addressed.

Comment 48

[Cristian Rodriguez (:crdlc)]

Yes, I think so too, I am agree with you

Comment 49

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8346351 [details] [review]
point to https://github.com/mozilla-b2g/gaia/pull/14594

Great work! It looks good to me. Thanks a lot. We have to wait for a green Travis or at least noting related to this patch :)

Comment 51

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

Here I second to evelyn's solution and thank to her effort.

Comment 53

[Cristian Rodriguez (:crdlc)]

Merged in master:

https://github.com/evelynhung/gaia/commit/e95ac024020604d71f50c8d33f1fd669d17b031e

Comment 54

[Cristian Rodriguez (:crdlc)]

Sorry, merged in v1.2 (NO MASTER):

https://github.com/evelynhung/gaia/commit/e95ac024020604d71f50c8d33f1fd669d17b031e

(In reply to Cristian Rodriguez (:crdlc) from comment #53)
> Merged in master:
> 
> https://github.com/evelynhung/gaia/commit/
> e95ac024020604d71f50c8d33f1fd669d17b031e

Comment 55

[Cristian Rodriguez (:crdlc)]

Please it should be in v1.3 and master as well, are you agree?

Comment 56

[Evelyn Hung]

(In reply to Cristian Rodriguez (:crdlc) from comment #55)
> Please it should be in v1.3 and master as well, are you agree?

Yes, thanks for the review and merge effort. :)

Merge commit: 
- master: https://github.com/mozilla-b2g/gaia/commit/ac50e4c9a720db2d5da97ab86f7e962dd2c4c576
- v1.2: https://github.com/mozilla-b2g/gaia/commit/b70cfc20fbf1e6e1366ed88d2ad426d26813e843

For v1.3 uplift, please use master version (just one comment line edited though). Thanks.

Comment 57

[John Ford [:jhford] CET/CEST Berlin Time]

This is on v1.2f

Comment 58

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted ac50e4c9a720db2d5da97ab86f7e962dd2c4c576 to:
v1.3: 3775dfa1000513cfdd883187186730521f035edc