Closed Bug 1107139 Opened 9 years ago Closed 9 years ago

[Window Management] Active app window appears blue/grey in card view

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.2+, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S9 (3apr)
blocking-b2g 2.2+
Tracking Status
b2g-v2.1 --- unaffected
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: KTucker, Assigned: sfoster)

References

Details

(Keywords: polish, regression, Whiteboard: [2.2-flame-reduced-run] [systemsfe])

Attachments

(13 files, 7 obsolete files)

12.54 KB, image/png
Details
275.21 KB, text/plain
Details
41.09 KB, text/x-log
Details
7.85 MB, text/plain
Details
4.51 KB, text/plain
Details
62.26 KB, text/plain
Details
7.37 MB, video/3gpp
Details
1.81 MB, text/plain
Details
46 bytes, text/x-github-pull-request
kgrandon
: review+
djabber
: ui-review+
Details | Review
46 bytes, text/x-github-pull-request
Details | Review
123.69 KB, image/png
Details
28.25 KB, image/png
Details
116.66 KB, image/png
Details
Description:
Camera and Gallery appear gray and they missing icons in card view.

Repro Steps:
1)  Updated Flame to Build ID: 20141203040207
2)  Open the camera or gallery app.
3)  Press and hold the home button to enter card view.
4)  Observe the card.

Actual:
The camera and gallery apps are gray and they are missing icons in card view.

Expected:
The camera and gallery apps appear correctly in card view. 

Environmental variables:
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141203040207
Gaia: 725685831f5336cf007e36d9a812aad689604695
Gecko: 2c9781c3e9b5
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Notes:
Repro frequency: 100%
See attached: screenshot, logcat
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
NI to Windows Management Owner to make a blocking call - Notes: This is a regression, but you can still tell what the cards are suppose to represent
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell) → needinfo?(gchang)
This is also occurring in other apps such as Dialer and Calendar but it didn't seem to happen as often as camera and gallery.
NI window management owner for this issue.
Flags: needinfo?(gchang) → needinfo?(hcheng)
I can reproduce this problem when I entered card view from homescreen and the card with wrong display is the most recent app.

My steps: (the error occurs at step 3)
1. open camera (or gallery or dialer or calendar)
2. press home button and then back to homescreen
3. long press home button frome homescreen to enter card view and would get the camera card with wrong display.
4. If I did not enter camera app after step 3 and back to homescreen, the display would be correct after I enter card view again.
Flags: needinfo?(hcheng) → needinfo?(alive)
QA Contact: hcheng
Transfer NI
Flags: needinfo?(sfoster)
Flags: needinfo?(aus)
Flags: needinfo?(alive)
yeah something definitely regressed here, its taking much longer for the screenshots to show up. I suspect this is down in platform, but I'll do some investigation.
Assignee: nobody → sfoster
Flags: needinfo?(sfoster)
Flags: needinfo?(aus)
So I can't reproduce the icons part of this bug, they show up near instantly every time for me. However the screenshot itself is slow to show up - sometimes 5 or more seconds after the cards view displays. 

With some logging I can see that the request for the screenshot *and* the request success and assignment of the blob: url to the backgroundImage for each card is all taking place with the expected timing. However, we don't *see* that screenshot until many seconds later.
When I reproduce this problem, I got below message in logcat and only got it at 1st time.

*message in logcat:
W/GeckoConsole(  203): [JavaScript Warning: "Error in parsing value for 'visibility'.  Declaration dropped." {file: "app://system.gaiamobile.org/index.html" line: 0 column: 12 source: "visibility: undefined"}]

*step:
1. restart phone (flame-kk, build id: 20141211160202)
2. start gallery app
3. tap home button to homescreen
4. long press home button to card view and got gallery card with gray display
Flags: needinfo?(sfoster)
I've filed bug 1110932 for the visibility warning, I believe it is unrelated and due to having no initial value for favoriteButtonVisibility when the card template is rendered. I'll get that fixed to eliminate it as a suspect
Flags: needinfo?(sfoster)
I've not had success yet profiling this issue - my local build wouldnt start up and nothing useful I could spot in logcat. Probably I missed something or messed it up - will try again first thing next week (Please steal it though if you figure it out before me.) Marking this as a 2.2 blocker in the meantime.
blocking-b2g: --- → 2.2?
Depends on: 1097822
Aus and I dug into this a bit and here's what I see happening: 

STR: 
1. Launch gallery app (arbitrary, any app should do).
2. Tap home to return to homescreen
3. Long tap home to bring up task manager

At step 2 we actually take and store the screenshot in a _screenshotBlob property on the AppWindow instance. You can get this object from the console by e.g.

  appWindowManager.getApp('app://gallery.gaiamobile.org')

The screenshot blob is stored in a _screenshotBlob property on the instance. The instance also an instanceID property which you can use to find its rendered representation in the DOM. If you inspect the DOM of the AppWindow representing your app, (it will match #windows .appWindow) you'll see a .appWindow > browser-container -> screenshot-overlay element. At step 2 it has visibility:hidden and no backgroundImage property. 

The task manager uses -moz-element to show us the .appWindow div. In step 3, the cardviewbeforeshow event handler calls _showScreenshotOverlay on the AppWindow instance, which toggles the visibility of this screenshot-overlay, and puts the screenshot blob in the style.backgroundImage. If you watch the DOM at this point, you can see this property show up on the element long before the screenshot is painted. Sometimes the lag is seconds, sometimes it seems to never get painted. 

Benoit: AFAICT all the system app/js and CSS is doing the right thing here. For some reason there is a variable, long period between assigning the backgroundImage on the appwindow and it getting painted in the -moz-element'd task manager div. Can you help or direct this to someone that can?
Flags: needinfo?(bgirard)
Summary: [Window Management] Camera and Gallery appear gray and they are missing icons in card view → [Window Management] Inactive app windows extremely slow to paint in card view
Why not start with getting a profile of the whole process? With a custom build use:
$ ./profile.sh start -p b2g -t GeckoMain,Compositor -f stackwalk,js
$ ./profile.sh start -p <Camera PID> -f stackwalk,js
Type but don't submit $ ./profile.sh capture
Run the animation and *immediately* after the slow execution period press return. If you submit the capture command with more than 1 second delay reboot the phone and retry the steps.

Once collected attach profile_captured.sym. You can have a look yourself if you'd like:
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler
Flags: needinfo?(bgirard)
(In reply to Benoit Girard (:BenWa) from comment #14)
> Why not start with getting a profile of the whole process? 

Thanks for the details. I'm getting an error when I ./profile.sh capture: http://pastebin.mozilla.org/8054175 which I've had no luck troubleshooting. Not sure if this is a python env problem on my machine, or some other peculiarity, but the profile_captured.sym ends up empty.
Flags: needinfo?(bgirard)
That's bug 1111830. Make sure you're have that patch in your tree, it's on central now.
Flags: needinfo?(bgirard) → needinfo?(sfoster)
Attached file profile_cardview_from_home.sym (obsolete) —
This capture should include the 2-3 seconds after tapping hold-home when the Gallery screenshot *should* have been displayed in the card view. A few seconds after the capture it finally showed up, I hope this is useful.
Flags: needinfo?(sfoster)
I don't really know how to interpret these profiles, but it looks to me like not much of anything is going on while we wait for that screenshot. Is it more likely that we're just not invalidating that -moz-element area for re-paint?
Flags: needinfo?(bgirard)
Whiteboard: [2.2-flame-reduced-run] → [2.2-flame-reduced-run] [systemsfe]
Let's try hard to fix this, but we won't block the release on it.
blocking-b2g: 2.2? → ---
Keywords: polish
The profile here is no good. It doesn't record any interesting activity so there isn't anything to interpret. Either the activity is happening in a different process/thread or it didn't happen over the last 4 seconds after you type ./profile.sh capture.

If it's happening in a different process try profiling an additional process or two:
./profile.sh start -p b2g && sleep 1 && ./profile.sh start -p Gallery
<interact>
./profile.sh capture (immediately after)
Flags: needinfo?(bgirard)
Attached file profile_2174_Gallery.sym (obsolete) —
Ok this time I used  ./profile.sh start -p b2g && sleep 1 && ./profile.sh start -p Gallery. I did the holdhome to show cardview from the homescreen, with just Gallery running in the background. I waited approx 30s before the card showed the gallery app, and hit capture immediately after. Looks like there are some spikes at the end of the profule that correspond to what I saw, but again not much of anything going on before then to explain the long 30s wait. Still, I hope this is useful. Let me know if I can do anything more to help.
Attachment #8538777 - Attachment is obsolete: true
Flags: needinfo?(bgirard)
Do you have a file called profile_captured.sym? This should be the merged output of all processes. If you got the timing right you're executing is happening in another process/thread, possibly the b2g (system) process' main thread.
Flags: needinfo?(bgirard)
Attached file profile_captured.sym
Scratch that last one. This one also includes the Homescreen
Attachment #8545417 - Attachment is obsolete: true
We talked on IRC. All the profiles indicate that the CPU is idle. I don't think this is a CPU performance problem. I think we need to debug what requests are being made and if any threads are being blocked.
Attached file bug-1107139-top.txt
Heres the output from running top -t -m 10 -n 5 while waiting for that gallery card preview area to show up. I tried a few other scenarios and noticed that while all in-active apps are a little slow to show up (1-2s) only the fullscreen apps have this extreme problem. I'll keep digging.
Could it be we're hitting a -moz-element invalidation bug? I've noticed on the status bar that there used to be situations where when it was showing via -moz-element, it would stop animating (e.g. when it's connecting to a network), but certain operations would force it to update.

Have you tried logging when the screenshots are actually returned/loaded to see that they're in line with when they get displayed?
(In reply to Chris Lord [:cwiiis] from comment #26)
> Could it be we're hitting a -moz-element invalidation bug? 

That was my thought also, but I think BenWa thought not?  

> Have you tried logging when the screenshots are actually returned/loaded to
> see that they're in line with when they get displayed?

Yes I tried this - at least on the Gaia side. The browser_mixin's getScreenshot callback is called when you would expect, but it never paints, or paints many seconds later.
Does anyone else reproduce this with the severity I'm seeing? For me, when switching to the task manager from (e.g.) the homescreen, the screenshots for the other apps take *many* seconds to display. Sometimes 30s or more. This bug was last triaged as not-blocking, but unless my experience is unique, it seems like a blocker to me?
blocking-b2g: --- → 2.2?
blocking on regression
blocking-b2g: 2.2? → 2.2+
See Also: → 1086594
Regression range is being investigated in bug 1086594
According to the comments in bug 1086594, this bug has existed in 2.0 and 2.1 as well, and because it is intermittent, getting regression range is difficult.  Any actions that QA can take to speed things up?
The expexted sequence should be: 

1. Open full screen app (e.g. Gallery)
2. Tap home to return to homescreen
3. Long-tap home to launch task-manager
4. Task manager shows "cards" with title/name above, screenshot and icon below. 

At step 4 the effect I was seeing was quite severe - the card rectangle with title and icon would be visible almost immediately, but the screenshot area was black for many seconds. Sometimes the screenshot would take as much as a minute to show up - if at all. Today I cant reproduce though - so maybe this was fixed? If QA cant reproduce the severe effect we can resolve as worksforme.
Flags: needinfo?(npark)
I can reproduce that with the today's nightly build. But problem with this bug is that it is quite intermittent, that sometimes it takes a number of effort to reproduce it once.  On other days, it happens on the first attempt.
Flags: needinfo?(npark)
Flags: needinfo?(nhirata.bugzilla)
I think it might have something to do with the time it takes a screenshot and what's in memory.

I get a larger percent hit of seeing gray when I go from homescreen to camera and long tap while the camera app is loading versus camera already launched and the long tap.

Would it be worth capturing a profile for the camera app in that condition?  homescreen -> camera => long tap?
Attached file logcat.txt
I found that there's actually logging for screen shots in the debug.

I/GeckoDump( 4586): [system] [AppWindow][Video][AppWindow_6][-5019.321] getScreenshot timeout!
I/GeckoDump( 4586): [system] [AppWindow][Settings][AppWindow_5][-5019.290] close with immediate


Also to note, I think there's a greater chance that this might happen more on freshly flashed devices.
(In reply to Sam Foster [:sfoster] from comment #33)
> 1. Open full screen app (e.g. Gallery)
> 2. Tap home to return to homescreen
> 3. Long-tap home to launch task-manager
> 4. Task manager shows "cards" with title/name above, screenshot and icon
> below. 
> 
> At step 4 the effect I was seeing was quite severe - the card rectangle with
> title and icon would be visible almost immediately, but the screenshot area
> was black for many seconds. Sometimes the screenshot would take as much as a
> minute to show up - if at all.

I'm still unable to reproduce this, with (relatively) freshly flashed Flame, moz-central/master with 319MB. 
But the original STR do produce a blank screenshot area for the camera app if you holdhome quickly while the Camera app is still loading/initializing 
I
Summary: [Window Management] Inactive app windows extremely slow to paint in card view → [Window Management] App windows appear blue/grey in card view
Summary: [Window Management] App windows appear blue/grey in card view → [Window Management] Active app window appears blue/grey in card view
I've updated the title of this bug. Sorry about the confusion. We're focusing on the original STR in the description. You can repro quite easily by launching camera app and immediately long-tap the home button to show cards view.  

The blue/grey is a background-color added in https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L1035,L1039, its a symptom of there being no screenshot available yet to display. 

We've run into this before. There's no event fired when a new screenshot does become available, so if requestScreenshotUrl returns null (_screenshotBlob is not yet populated) we dont recover - we just see that background color via the -moz-element which sits behind the screenshot "layer". In the debugger, at least some of the time you can see that a screenshot later becomes available; in the console, do:

appWindowManager.getActiveApp().requestScreenshotURL()

.. so it might help to publish an event in AppWindow which the card can listen for if app.isActive(), but app.requestScreenshotURL() returns falsey.
Comment on attachment 8568683 [details] [review]
[gaia] sfoster:task-manager-screenshotupdate > mozilla-b2g:master

Aus, you've run into this same issue with screenshots in task-manager. Can I get your feedback on this before I go further?
Flags: needinfo?(aus)
Ive left logging in there which shows we actually get 2 callbacks and 2 screenshots. This is a bug? I'm using a needsScreenshot flag on the Card to just update on the first one.
Hi Sam,

We had seen this before for sure. The screenshot was actually taken and available but was simply not getting repainted until the card was somehow dragged which causes the surface to force repaint. Simply setting the new screenshot as the background doesn't seem to always cause the invalidation of the drawing surface. I have no idea why. It's a bug, it's kind of nasty. :(
Flags: needinfo?(aus)
> We had seen this before for sure. The screenshot was actually taken and
> available but was simply not getting repainted until the card was somehow
> dragged which causes the surface to force repaint. Simply setting the new
> screenshot as the background doesn't seem to always cause the invalidation
> of the drawing surface. I have no idea why. It's a bug, it's kind of nasty.
> :(

Ok, well I'm asserting this patch fixes at least some manifestations of this bug. I hope this additional asynchrony of the appscreenshotupdate listener doesn't complicate too much. I'm going to look at some tests and put it in for review
status: currently going around in circles on this one. The problem is that if you holdhome while an app is still initializing, the iframe.getScreenshot is slow to respond and we timeout before it returns. The 
best outcome I've had so far turns out to be simply bumping up the timeout at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/task_manager.js#L532,L534
.. but that swings us back to a situation where the UI appears slow to respond to the long home button tap. 
 
The screenshotupdate approach does succeed make a screenshot available in the task manager, but by the time we see it, we've also watched the transition to closed via the -moz-element and it ends up looking very flickery and disjointed - particularly when the screenshot doesn't look anything like the last state we saw in the window. 

FWIW I didn't get a integration test to fail (I tried skipping the waitUntilScreenshotable and doing taskManager.show()) to test the patch against. So I'm 0/2 on this right now.
(thinking out loud)

At https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#904
We see: 

// Try to wait for the event loop to go idle before we take the screenshot,
// but once we've waited maxDelayMS milliseconds, go ahead and take it
// anyway.

maxScreenshotDlayMS defaults to 2000ms, so this accounts for the timeout we often see when trying to get a screenshot of an app that is busy initializing. I don't think we want to wait much more than the 400ms we currently do before giving up and showing the task manager anyway. The question is, what to do then? Another screenshot is created when the app closes, we could use that but we'll get a flicker as we redraw the background. 

Currently requestScreenshotURL can give us an old screenshotBlob in the case where the getScreenshot call times out, which may look completely different to what the user last saw of the app before they long-tapped home. Maybe we could leave up the identificationOverlay if we've nothing to show in the screenshotOverlay?
Flags: needinfo?(etienne)
(In reply to Sam Foster [:sfoster] from comment #47)
> (thinking out loud)
> 
> At
> https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/
> BrowserElementChildPreload.js#904
> We see: 
> 
> // Try to wait for the event loop to go idle before we take the screenshot,
> // but once we've waited maxDelayMS milliseconds, go ahead and take it
> // anyway.
> 

Ha! This explains a lot, good find!

> Currently requestScreenshotURL can give us an old screenshotBlob in the case
> where the getScreenshot call times out, which may look completely different
> to what the user last saw of the app before they long-tapped home. Maybe we
> could leave up the identificationOverlay if we've nothing to show in the
> screenshotOverlay?

We should check with UX but I think this is the best alternative.
Flags: needinfo?(etienne)
(In reply to Etienne Segonzac (:etienne) from comment #48)
> > Currently requestScreenshotURL can give us an old screenshotBlob in the case
> > where the getScreenshot call times out, which may look completely different
> > to what the user last saw of the app before they long-tapped home. Maybe we
> > could leave up the identificationOverlay if we've nothing to show in the
> > screenshotOverlay?
> 
> We should check with UX but I think this is the best alternative.

What do you think, Francis?
Flags: needinfo?(fdjabri)
(In reply to Sam Foster [:sfoster] from comment #49)
> (In reply to Etienne Segonzac (:etienne) from comment #48)
> > > Currently requestScreenshotURL can give us an old screenshotBlob in the case
> > > where the getScreenshot call times out, which may look completely different
> > > to what the user last saw of the app before they long-tapped home. Maybe we
> > > could leave up the identificationOverlay if we've nothing to show in the
> > > screenshotOverlay?
> > 
> > We should check with UX but I think this is the best alternative.
> 
> What do you think, Francis?

Hi Sam, as we discussed, this sounds good to me.
Flags: needinfo?(fdjabri)
Attachment #8568683 - Attachment is obsolete: true
Comment on attachment 8576945 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-bug-1107139 > mozilla-b2g:master

Status update - this is fiddly stuff. The patch expresses what I *want to happen* but its not quite there. At cardviewbeforeshow, if the app doesn't get have a screenshotBlob, we want to show the notification overlay and watch that scale via the closeAppTowardsCardView anim. Right now it swaps in after that animation is done. Does it jump out at you Etienne?

When we select a card in the task manager and re-open, right now the identification overlay is hidden immediately. That ends up looking ok to me.
Attachment #8576945 - Flags: feedback?(etienne)
Comment on attachment 8576945 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-bug-1107139 > mozilla-b2g:master

(hopefully helpful) comments on github!
And f+ for the logic/structure of the patch (minus all the debugging logs ;))
Attachment #8576945 - Flags: feedback?(etienne) → feedback+
Comment on attachment 8576945 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-bug-1107139 > mozilla-b2g:master

This is performing pretty well for me. You'll often see the identification-overlay - when going to task manager from an active app the first time. But if the screenshot comes before the timeout we display it, and using cardviewshown can remove the .no-screenshot class so you get the expected transition when hiding the task-manager. 
I nudged down the timeout for the screenshot in the task manager because of this - I think we can trade up for a bit more responsiveness with this arrangement. 

Left one new this.debug statement in, happy to remove though if you prefer
Attachment #8576945 - Flags: review?(etienne)
Comment on attachment 8576945 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-bug-1107139 > mozilla-b2g:master

Looking good, r=me with quick unit tests for the AppWindow/AppWindowManager changes.
Attachment #8576945 - Flags: review?(etienne) → review+
I just noticed that in master branch, card view is shown in portrait mode only, and I can't seem to repro this bug anymore.  was this orientation fix required to fix this bug?

I welcome the change, just that I couldn't find the discussion specific to the orientation change.
(In reply to No-Jun Park [:njpark] from comment #56)
> I just noticed that in master branch, card view is shown in portrait mode
> only, and I can't seem to repro this bug anymore.  was this orientation fix
> required to fix this bug?
> 
> I welcome the change, just that I couldn't find the discussion specific to
> the orientation change.

We locked to portrait (or default) in bug 1118390 - which restored the behavior we had in earlier versions of the card view. Eventually we want to support landscape and portrait card view but its quite a large effort and out of scope for 2.2
Figured out why my unit tests were failing :/ I've updated the PR with what I hope to get to pass once bug 1143886 lands.
Depends on: 1143886
Gaia-Try looks good, landing.
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
sorry had to back this out since this caused in mozilla-inbound when landing with the merges test failures like https://treeherder.mozilla.org/img/logviewerIcon.svg
Status: RESOLVED → REOPENED
Flags: needinfo?(sfoster)
Resolution: FIXED → ---
(In reply to Carsten Book [:Tomcat] from comment #61)
> sorry had to back this out since this caused in mozilla-inbound when landing
> with the merges test failures like
> https://treeherder.mozilla.org/img/logviewerIcon.svg

err wrong link :) https://treeherder.mozilla.org/logviewer.html#?job_id=7746599&repo=mozilla-inbound is better :)
Comment on attachment 8579462 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-v2-bug-1107139 > mozilla-b2g:master

I've updated the test in question. I'll attempt to re-land once the Gaia-Try results go green for the new PR. Carrying Etienne's r+.
Flags: needinfo?(sfoster)
Attachment #8579462 - Flags: review+
Attachment #8576945 - Attachment is obsolete: true
Comment on attachment 8579462 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-v2-bug-1107139 > mozilla-b2g:master

Former version of this patch had r+ from :etienne. I've fixed the test that caused the backout. Looking for a review stamp to get this landed again.
Attachment #8579462 - Flags: review+ → review?(kgrandon)
Comment on attachment 8579462 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-v2-bug-1107139 > mozilla-b2g:master

Leaving a review stamp in case you want to leverage autolander. Sorry about the workflow pain.
Attachment #8579462 - Flags: review?(kgrandon) → review+
I'm told autolander will run Gij before attempting to merge, so lets try it.
Keywords: checkin-needed
Comment on attachment 8579462 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-v2-bug-1107139 > mozilla-b2g:master

I want to see if autolander will process a new attachment.
Attachment #8579462 - Attachment is obsolete: true
Attachment #8580385 - Flags: review+
Let's try this one more time. Sorry for the churn.
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Please uplift to v2.2. Thanks!
Keywords: verifyme
(In reply to Hermes Cheng[:hermescheng] from comment #72)
> Please uplift to v2.2. Thanks!

This got backed out earlier this week. I would like to let it bake on master for at least a day before uplifting. I'll request uplift before the end of the week
We're seeing problems in tests with:

JavascriptException: TypeError: this.element is null in app://system.gaiamobile.org/gaia_build_defer_index.js line: 2337

I'll link in the bug I'm filing presently with more info, but we think it's a pretty good chance that's coming from https://github.com/mozilla-b2g/gaia/commit/413a4dea46b52afcba7cb110542edb4c4c1f188d since it just started this morning.
(In reply to Geo Mealer [:geo] from comment #75)
> We're seeing problems in tests with:
> 
> JavascriptException: TypeError: this.element is null in
> app://system.gaiamobile.org/gaia_build_defer_index.js line: 2337
> 
> I'll link in the bug I'm filing presently with more info, but we think it's
> a pretty good chance that's coming from
> https://github.com/mozilla-b2g/gaia/commit/
> 413a4dea46b52afcba7cb110542edb4c4c1f188d since it just started this morning.

There is an unguarded this.element access at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L1322 which *could* be the problem? I can't imagine when we would have a .screenshotOverlay but not a .element. cc me though on the bug and I'll look into it. Or back out this one.
Well, I looked further, and the tests I have that bracket the issue have the same Gaia and different Geckos. Unfortunately, I don't have an easy way to bisect Gaia UI tests over Gecko, so that's a tough one.

Bracketing tinderbox builds are:

tinderbox-builds/b2g-inbound-flame-kk-eng/20150319180839
https://hg.mozilla.org/integration/b2g-inbound/rev/069d3d6722ad

tinderbox-builds/b2g-inbound-flame-kk-eng/20150319193241
https://hg.mozilla.org/integration/b2g-inbound/rev/ed6fdb6e238e

In both cases, Gaia is:

https://github.com/mozilla-b2g/gaia/commit/6e24aff11fbb385a6fb1651536a97263457b6c50

Still working on the logs/stacks on the primary bug, will update here when filed.
I won't block/depends, since the bracket suggests a Gecko change rather than Gaia
Blocks: 1145875
Comment on attachment 8580385 [details] [review]
[gaia] KevinGrandon:bug_1107139_retry_autoland > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Task Manager / Tab View
[User impact] if declined: Some apps can appear blank in card view 
[Testing completed]: Tested on device, new integration tests
[Risk to taking this patch] (and alternatives if risky): Low risk 
[String changes made]: None
Attachment #8580385 - Flags: approval-gaia-v2.2?
Hi Sam, I follow this steps to try to repro it on flame 2.2, I'm not sure if this issue is fixed,the details as follow:
1. Open full screen app (e.g. Camera)
2. Tap home to return to homescreen
3-1. If we Long-tap home button in short time after backing to homescreen, there is a grey screenshort of camera displayed.
3-2. If we waiting for 2-3 sec after backing to homscreen, and then long tap home button,a corrected screenshort of camera displayed.

Please check the attachmet: verify1.3gp and logcat324.txt.

Environmental variables:
Flame 2.2:
Build ID               20150323162503
Gaia Revision          e54c4ed1cc188f70ddf1156534d364005dc45490
Gaia Date              2015-03-23 19:09:26
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7ba1778d237b
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150323.200543
Firmware Date          Mon Mar 23 20:05:54 EDT 2015
Bootloader             L1TC000118D0
Attached file logcat324.txt
Flags: needinfo?(sfoster)
Alissa, the patch has not been landed to 2.2 yet and is still waiting for approval (comment 79). I think the build will be ready for verification in a couple of days.

Clearing NI for Sam.

(In reply to Alissa from comment #80)
> Hi Sam, I follow this steps to try to repro it on flame 2.2, I'm not sure if
> this issue is fixed,the details as follow:
> 1. Open full screen app (e.g. Camera)
> 2. Tap home to return to homescreen
> 3-1. If we Long-tap home button in short time after backing to homescreen,
> there is a grey screenshort of camera displayed.
> 3-2. If we waiting for 2-3 sec after backing to homscreen, and then long tap
> home button,a corrected screenshort of camera displayed.
> 
> Please check the attachmet: verify1.3gp and logcat324.txt.
> 
> Environmental variables:
> Flame 2.2:
> Build ID               20150323162503
> Gaia Revision          e54c4ed1cc188f70ddf1156534d364005dc45490
> Gaia Date              2015-03-23 19:09:26
> Gecko Revision        
> https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7ba1778d237b
> Gecko Version          37.0
> Device Name            flame
> Firmware(Release)      4.4.2
> Firmware(Incremental)  eng.cltbld.20150323.200543
> Firmware Date          Mon Mar 23 20:05:54 EDT 2015
> Bootloader             L1TC000118D0
Flags: needinfo?(sfoster)
Depends on: 1146680
bug 1146680 turns out to be a regression caused by attachment #8580385 [details] [review] that landed. Will backout and create a new updated PR - as we want to uplift this one, that seems the cleaner route.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8580385 [details] [review]
[gaia] KevinGrandon:bug_1107139_retry_autoland > mozilla-b2g:master

Will need a new patch for 2.2 uplift, cancelling approval request.
Attachment #8580385 - Flags: approval-gaia-v2.2?
Comment on attachment 8582674 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-bug-1107139 > mozilla-b2g:master

This is same patch as before (cset 413a4dea46b52afcba7cb110542edb4c4c1f188d) with fix/un-regressing bug 1146680 and fixed test that should have caught that.
Attachment #8582674 - Flags: review?(kgrandon)
Test run is shiny green. Just needs r+ to land
Comment on attachment 8582674 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-bug-1107139 > mozilla-b2g:master

Review stamping this based on Etienne's review earlier.
Attachment #8582674 - Flags: review?(kgrandon) → review+
Keywords: checkin-needed
Attachment #8580385 - Attachment is obsolete: true
Attachment #8582610 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Comment on attachment 8583592 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-v2-bug-1107139 > mozilla-b2g:v2.2

Treeherder still unhappy. Michael, could you possibly push this over the line when v2.2 branch tests get cleared up? I had requested uplift before, but as the master patch got backed out, we'll need to do it again. Note that this PR includes the patch to fix AppWindow element unit test stubs, which is tracked in bug 1143886. I guess it will need flattening before landing, but I wanted to see a clean test run in the meantime as I've not been able to land that.
Flags: needinfo?(mhenretty)
Comment on attachment 8583592 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-v2-bug-1107139 > mozilla-b2g:v2.2

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Task Manager / Tab View
[User impact] if declined: Some apps can appear blank in card view 
[Testing completed]: Tested on device, new integration tests
[Risk to taking this patch] (and alternatives if risky): Low risk 
[String changes made]: None

Note: this was originally landed and then backed out of master. Bug 1143886 (test-only) was landed on v2.2 recently to address that problem, so this is ready to land on v2.2 again.
Flags: needinfo?(mhenretty)
Attachment #8583592 - Flags: approval-gaia-v2.2?
(In reply to Sam Foster [:sfoster] from comment #93)
> Comment on attachment 8583592 [details] [review]
> [gaia] sfoster:task-manager-no-screenshot-v2-bug-1107139 > mozilla-b2g:v2.2
> 
> Treeherder still unhappy. Michael, could you possibly push this over the
> line when v2.2 branch tests get cleared up? I had requested uplift before,
> but as the master patch got backed out, we'll need to do it again. Note that
> this PR includes the patch to fix AppWindow element unit test stubs, which
> is tracked in bug 1143886. I guess it will need flattening before landing,
> but I wanted to see a clean test run in the meantime as I've not been able
> to land that.

Sam, unfortunately we aren't going to get green runs on Gaia-Try vor v2.2 until they uplift a bunch of task cluster stuff to that branch. This is an unfortunate side effect of the TC migration, so the best we can do is make sure it's working on master for now, uplift, and watch.
Target Milestone: --- → 2.2 S9 (3apr)
Parul, can you please help verify this on master ?
Flags: needinfo?(pmathur)
Verified not fixed on master.

Repro Steps:
1)  Open the Browser app.
2)  Open the Camera app and then the Gallery app.
3)  Press and hold the home button to enter card view.
4)  Observe the cards.

Actual:
Gallery app in card view appears gray and then Gallery app closes.
Camera app in card view first appears gray for a brief flash, then shows the correct screenshot, then sometimes shows a black screenshot with camera controls.

Expected:
The camera and gallery apps appear correctly in card view. 

Test Environment:
Gaia-Rev        67ad91f3f660b1f16b354ee4c5159ddc5a74d149
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/385840329d91
Build-ID        20150329160203
Version         39.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150329.192807
FW-Date         Sun Mar 29 19:28:16 EDT 2015
Bootloader      L1TC000118D0
Flags: needinfo?(pmathur)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Parul Mathur [:pragmatic] from comment #97)
> Actual:
> Gallery app in card view appears gray and then Gallery app closes.
> Camera app in card view first appears gray for a brief flash, then shows the
> correct screenshot, then sometimes shows a black screenshot with camera
> controls.

I think this may be a separate issue with the Gallery app - it seems to crash if its closed before it finishes initializing, I'll look into it and file a bug if necessary. The camera app behavior is expected - it is changing state as it opens and as it transitions to card view. I'll check its doing the right thing but I don't want to boil the ocean with this bug.
Attached image CardView.png
Here's what I found on today's nightly build:

1) When entering the card view, the Camera app always shows the gray screen with the icon in the center.
2) When entering the card view, the Gallery app shows the screenshot correctly when the Gallery app is open with no or a few other apps open in the background.
3) When entering the card view, the Gallery app shows the gray screen with the icon in the center when several other apps are open in the background.
4) When entering card view while the Gallery app is still loading images, the card view is shown for a brief moment, and the app and card view close. The user is returned to the homescreen.

I'm not sure if any of these symptoms are expected behaviors. Please let me know if there has to be a new bug filed.

Environmental Variables:
Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150330010204
Gaia: be25b16efa19bab8d54be08f8fe45dcc93bf93d0
Gecko: dfe60814eda7
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
I am not seeing any of these issues stated in Comment 99 occur on today's master build besides number 4. The gallery app is suppose to force close if the user leaves the app while it is loading. This is by design unless it has been changed. 

Piwei, can you please double check the results as well in Comment 99? We are seeing conflicting results so there might be something else going on here.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage-]
Flags: needinfo?(ktucker) → needinfo?(pcheng)
(In reply to KTucker [:KTucker] from comment #100)
> I am not seeing any of these issues stated in Comment 99 occur on today's
> master build besides number 4. The gallery app is suppose to force close if
> the user leaves the app while it is loading. This is by design unless it has
> been changed. 

If that is the case then card view is working as expected. If the app terminates and it is the only app running then there's nothing to show and we go back to homescreen. Note that the premise for this bug is all about timing, so we expect some variation, especially if there are several apps running. The fix implemented in the patch is to show the identification overlay - the icon in the middle - if no screenshot was immediately available *for the active app* when going into card view.
This patch already landed on master. If we determine it doesn't fix the problem, we need to either back out the patch (and re-open the bug) or file follow-up bugs. In the meantime it remains resolved but not verified.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
I am hesitant to uplift on 2.2 given QA's feedback. Sam, can you confirm you cannot reproduce the issue on latest master? Has this been tested by you on the device ? Looks like QA is using the exact STR in the description.
Flags: needinfo?(sfoster)
(In reply to bhavana bajaj [:bajaj] from comment #103)
> I am hesitant to uplift on 2.2 given QA's feedback. Sam, can you confirm you
> cannot reproduce the issue on latest master? Has this been tested by you on
> the device ? Looks like QA is using the exact STR in the description.

I agree we should clear up any questions before uplifting. For my part I've tested this extensively on device, on master. I'll also do some testing with the patch on v2.2. 

It looks like gallery will close itself if we go into card view while it is still scanning for images/videos. This is the 'backgroundScanKiller' function being called as the app is closed. I'm not sure if that is new behavior or if it should be considered a regression caused by this patch. As the original STR use gallery specifically, lets get clarification from David on that. 

David: What this patch changes is that it no longer waits as long for the event loop to idle before showing the card view and transitioning the active app (Gallery in this case) to "closed". This is to avoid blank cards in cards view, and to ensure the transition to card view is smooth and responsive. If we are seeing expected behavior from Gallery, then both comment #97 and comment #99 indicate verification that this bug is fixed.
Flags: needinfo?(sfoster) → needinfo?(dflanagan)
Flags: needinfo?(pcheng) → needinfo?(pcheng)
On devices with < 512mb of memory, the Gallery app intentionally closes itself when sent to the background if it is still scanning images. This is to prevent a background scan from using memory and slowing down the entire system.  If you test with a Flame with 512mb or more you should see that the gallery does not exit even if it is still scanning when you go to the card view.

I don't know about the camera app, but it would not suprise me if it is not possible to get a screenshot that includes the contents of the viewfinder. There might be too much magic going on with that, so the gray screen for camera in the cards view might be what is expected.

I don't know anything about the timing around displaying the card view, so I can't comment on that. I will add that you can configure the timing of the camera app killing itself if that would improve things.  Currently it waits 500ms after getting its visibilitychange event. If you changed that to 3 seconds or something you might not notice the app exiting in the cards view as often. See gallery.js:776
Flags: needinfo?(dflanagan)
Please see bug 1039943 like David said this is intentional in regards to the gallery app being closed.
As per comment 100's Needinfo request, I'm posting my testing results.

(In reply to Yeojin Chung [:YeojinC] from comment #99)
> 1) When entering the card view, the Camera app always shows the gray screen
> with the icon in the center.

I can get this to reliably occur if Camera is the only app in Card View and I enter Card View while in Camera.

> 2) When entering the card view, the Gallery app shows the screenshot
> correctly when the Gallery app is open with no or a few other apps open in
> the background.

I also see this.

> 3) When entering the card view, the Gallery app shows the gray screen with
> the icon in the center when several other apps are open in the background.

I have only seen this once after I'd taken ~10 pictures using the Camera (I originally had ~4 in Gallery), then I went to Gallery, and entered card view once it finished loading. I couldn't reproduce this afterward, with the same steps.

> 4) When entering card view while the Gallery app is still loading images,
> the card view is shown for a brief moment, and the app and card view close.
> The user is returned to the homescreen.

This behavior is expected and I can reproduce as well.

I'm on the same build as Comment 99. Full flashed 319MB memory.
Flags: needinfo?(pcheng)
So these are all vindications of the patch doing the thing it was supposed to do. Instead of showing an empty card when - for whatever reason - no screenshot of the app was available, it shows the icon (identification overlay) The horizontal task switcher is on v2.2, but this patch is not, so we can evaluate the behavior meaningfully by comparing master to v2.2. 

This patch cant fix all the many ways an app can end up looking different in the card view, but it does try to make it not look broken. After trying out the 4 cases from comment #107, I get the same results in v2.2, except that I see an empty-looking blue/grey space where I would expect a screenshot - vs on master where I see the app's icon on the translucent overlay. I don't think we have regressed anything here?
Naoki, can you confirm this is verified, and is there anything else we want to do before getting approval to land on v2.2?
Flags: needinfo?(nhirata.bugzilla)
Attached image 2015-03-31-20-34-40.png
So I tested on 2.2 and Master.  2.2 doesn't have the patch, and master does.  
1) the gallery app closing in task manager is intentional.  I don't really like it because it makes the app look like it crashed.
2) if the app is in a state that's not ready, this patch doesn't fix that issue; ie camera, and gallery with lots of images.
3) if he app has the data base loaded and the state is ready, this patch will address that issue.

So what most of QA is seeing I think is issue 2, and what sfoster fixed is issue 3.  I think we should split issue 2 into a separate bug.

To note, you can get in a state where you get a bad screenshot of things not fully loaded even though the app has fully loaded based on timing of the screenshot taken.
The patch doesn't address 100 % of the issues.  If we are to keep the patch, we should probably split this off into two bugs; app has database loaded and app doesn't have database loaded.

In the condition that the app doesn't have the database loaded or is ready when the home button is hit, it tends to capture screenshots of black screens.

In the condition that we have no screenshot, we have a replacement of the icon of the app so we can tell which app it is easier ( though we do have subtitles for the app ).  This is similar to what we did with Tarako, I think?  This is what part of the fix is.

In the condition that we have the database loaded and we have a screenshot we show the appropriate image.

I don't have a strong opinion either way for it landing on 2.2 now.  Personally I think we should just do away with screenshoting altogether because it slows down performance and just do app cards...
Flags: needinfo?(nhirata.bugzilla)
Has anyone spoken to anyone on the gfx team about improving the screenshot ability? I think currently it renders the whole app off-screen, and there used to be some odd behaviour with fixed-pos content (which might have been fixed?), but it seems there's no need for this to be so slow.

I suggest that if we aren't doing this already (and I assume we aren't, as screenshotting is slow), it'd be much more efficient for screenshots to just do a composite + readback from GPU. Not knowing what the API is like, maybe there are some complications I haven't considered, but given how fast composition is, screenshotting really oughtn't be something so slow (unless the slow part is image encoding/decoding?)
Let me clarify the scope of this bug and the patch: 

Sometimes, when showing the task manager from an app (vs. the homescreen), the card in the task manager may appear an empty blue/grey color. There are multiple reasons an app may appear different in card view, this bug focusses on an entirely missing screenshot - the blue/grey color is the background normally obscured by the image. 

STR: 
In principal any application could exhibit this behavior, but it is easier to reproduce with apps that do a lot of work at initialization.
1. Open Camera (or Contacts, or Gallery - but see comment #110)
2. Quickly long-tap the home button while the app is still starting up

Expected: The task manager shows a single card with some recognizable representation of the last active app

Actual: The task manager shows the title and icon/button, but an empty space where the screenshot should be. 

The root cause of this issue is that when an app first starts, we have no screenshot for it yet. When task manager shows, we have been requesting and waiting for one. That gave us bug 1081321. The fix for that was to timeout and bail on the screenshot if its was taking too long - which left the card empty. 

This patch takes the approach that if we have no screenshot already, we don't want to wait too long for one, so instead we show the identification overlay - the semi-opaque background with icon in the middle. So the expected result now is that the active app should be represented in task manager by the icon, if a screenshot was not immediately available. 

Note that this change alters the timing of the show task manager sequence. As we no longer block on the screenshot (which waits for the event loop to idle before timing out), apps may be transitioned to hidden faster than before. That is good for overall UX as the long-press on home appears more responsive. But we may see some fallout with apps like Gallery that watch and react to visibility changes - they may get during their startup. In the case of Gallery, it closes itself if it gets hidden while still scanning for media, to avoid that background processing lagging the experience. With this change we are more likely to see that and the result is that Gallery starts up (STR step1) and then quits (disappears) at step 2.
Comment on attachment 8582674 [details] [review]
[gaia] sfoster:task-manager-no-screenshot-bug-1107139 > mozilla-b2g:master

Can UX chime in on:

a) if the patch we landed adequately fixes the problem outlined in original STR and in comment #113
b) if we need to continue to block v2.2 on this 
c) if the current fix is something we want for sure on v2.2

A simple before/after comparison can be had by testing current master vs. current v2.2, or by applying attachment #8583592 [details] [review] to a v2.2 build
Attachment #8582674 - Flags: ui-review?(fdjabri)
I have filed bug 1150168 to address the issue observed at comment 107 that is specific to Camera app.
I filed bug 1150278 for the condition of an app still loading the database and the screenshot takes an image.


To note: the condition where the screenshot hasn't had time to be taken is fixed under this bug and shows an icon instead.
I agree with the approach that Sam outlines in comment #113. If a screenshot is not ready, then we should show the title and an icon. The responsiveness of the home button should be the paramount concern, and if that means that we might not show a screenshot, I think that's a tradeoff worth making. Also, in reality, I think users will take a second or two to register a screen before users access task manager - so the fallout that Sam mentions should be very rare IMO. Overall, I'm happy with the patch landing.
Attachment #8582674 - Flags: ui-review?(fdjabri) → ui-review+
Naoki/QA, can you summarize where we stand with this now. Can it be verified fixed? Ready/appropriate for uplift? (comment #103)
Flags: needinfo?(nhirata.bugzilla)
We filed the other two bugs as fall outs from this one, we got approval from ux to land in 2.2; so I guess you just need to get landing approval for 2.2 and land on 2.2
Flags: needinfo?(nhirata.bugzilla) → needinfo?(sfoster)
Attachment #8583592 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
I've rebased and picked up a late fix from the master PR, v2.2 PR is updated. I'll merge shortly
Flags: needinfo?(sfoster)
This issue is verified fixed on the latest Nightly Flame 3.0 and 2.2 builds.

Actual Results: The card view shows screenshots if available or shows a card for the last app containing that app's icon and name instead.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150403010203
Gaia: 7969b367a7da62877c3a24a26d3cb5fda89d766c
Gecko: 70a113676b21
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20150403002503
Gaia: 022eeb91197ba4a9adfd67bd6db5aa03cc69eb31
Gecko: 77fdc6fbcae9
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage-] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Depends on: 1165049
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: