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

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: angelc04, Assigned: jj.evelyn)

Tracking

({regression})

unspecified
All
Gonk (Firefox OS)
regression

Firefox Tracking Flags

(blocking-b2g:koi+, b2g-v1.2 fixed, b2g-v1.3 fixed)

Details

(Whiteboard: [Fugu] [v1.2f-uplift-needed])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Does this reproduce on 1.1?
Keywords: qawanted

Updated

5 years ago
Duplicate of this bug: 943545

Comment 3

5 years ago
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
Keywords: regression, regressionwindow-wanted

Updated

5 years ago
Keywords: qawanted

Updated

5 years ago
QA Contact: gbennett
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

5 years ago
(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
Keywords: regressionwindow-wanted

Updated

5 years ago
blocking-b2g: --- → koi?
koi+ as this is a very basic test case to fail.
blocking-b2g: koi? → koi+
Add Jesse, pls provide your patch to mozilla.

Updated

5 years ago
Flags: needinfo?(james.zhang)

Updated

5 years ago
Duplicate of this bug: 944754
Created attachment 8340611 [details] [diff] [review]
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.
Steven, please ask owner to review this critical issue. Thanks.
Flags: needinfo?(james.zhang)
Attachment #8340611 - Flags: review?(crdlc)
Please do a pull request because I see fails of jslint, 10x
Status: NEW → ASSIGNED
Assignee: nobody → james.zhang
And ask to Alive because you're touching the window manager, thanks
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.
Flags: needinfo?(styang)
Hi James, I like to see the code and test it in my device so I prefer a pr if you're comfortable. Thanks
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
(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.
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
Flags: needinfo?(jesse.ji)

Updated

5 years ago
Component: General → Gaia::Homescreen

Comment 18

5 years ago
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?
Flags: needinfo?(jesse.ji)
> 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
Flags: needinfo?(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.
Flags: needinfo?(alive)
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.
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.
Blocks: 943181

Comment 23

5 years ago
   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.
(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!

Updated

5 years ago
Flags: needinfo?(styang) → needinfo?(jesse.ji)

Comment 25

5 years ago
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.
Flags: needinfo?(jesse.ji)
Duplicate of this bug: 945888
I think we need fix this critical issue before v1.2 branch close.

Updated

5 years ago
Duplicate of this bug: 947489
Here's a screencast on the GP Peak.   It's a 1.2 build.

http://youtu.be/8fvzGK0olvg

Comment 30

5 years ago
Because 1.2 is going to be finished, should we mark this bug from koi+ to 1.3+? Thanks.
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
Attachment #8340611 - Flags: review?(crdlc)
(Assignee)

Comment 32

5 years ago
(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.
(Assignee)

Updated

5 years ago
Assignee: james.zhang → ehung
(Assignee)

Comment 33

5 years ago
(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.
blocking-b2g: koi+ → fugu?
(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?
Flags: needinfo?(khu)
(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+.
blocking-b2g: fugu? → koi+
(Assignee)

Comment 36

5 years ago
(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.
Hi Evelyn,

Please provide an ETA for landing?
Flags: needinfo?(ehung)

Comment 38

5 years ago
(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?
Flags: needinfo?(khu)
(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

5 years ago
(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.
(Assignee)

Comment 41

5 years ago
(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.
Flags: needinfo?(ehung)
(Assignee)

Comment 42

5 years ago
(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)
(Assignee)

Comment 43

5 years ago
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.
Attachment #8340611 - Attachment is obsolete: true
Attachment #8346351 - Flags: review?(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
Attachment #8346351 - Flags: review?(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
(Assignee)

Comment 46

5 years ago
(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.
(Assignee)

Comment 47

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

Thanks for detailed review. Github comment addressed.
Attachment #8346351 - Flags: review?(crdlc)
Yes, I think so too, I am agree with you
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 :)
Attachment #8346351 - Flags: review?(crdlc) → review+
Duplicate of this bug: 949825
Here I second to evelyn's solution and thank to her effort.
Duplicate of this bug: 949838
Merged in master:

https://github.com/evelynhung/gaia/commit/e95ac024020604d71f50c8d33f1fd669d17b031e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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
status-b2g-v1.2: --- → fixed
status-b2g-v1.3: --- → affected
Please it should be in v1.3 and master as well, are you agree?
Flags: needinfo?(ehung)
(Assignee)

Comment 56

5 years ago
(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.
Flags: needinfo?(ehung)
This is on v1.2f

Updated

5 years ago
Whiteboard: [Fugu] [v1.2f-uplift-needed]
Uplifted ac50e4c9a720db2d5da97ab86f7e962dd2c4c576 to:
v1.3: 3775dfa1000513cfdd883187186730521f035edc
status-b2g-v1.3: affected → fixed
You need to log in before you can comment on or make changes to this bug.