1154838 - [Window Management] Can't return to homescreen when PDF view is open via activity

Status: VERIFIED FIXED

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

Description

[David Flanagan [:djf]]

This was originally reported in bug 1152841, which also included a bug in the PDF viewer. 1152841 has a fix for the PDF viewer bug, but I'm filing this bug for the more serious window management issue.

See http://mzl.la/1OAsj0c for a video of the bug.

STR:

1) put a pdf file onto your sdcard somewhere

2) visit the Marketplace and install the "File Manager" app

3) Open File Manager, navigate to your pdf file, tap on it, and then tap open

4) The PDF viewer will open.  If you have the patch from bug 1152841 it should display the PDF file. If you don't have that patch, it will be blank. But that does not seem to affect this bug.

5) Press and hold the home button to bring up the task switcher and swipe up to kill the app.

6) Notice that the PDF viewer is still displayed.  And notice that if you tap the home button you don't go to the home screen.  If you press and hold the home button, it says "your recent app windows show up here".

Comment 1

[David Flanagan [:djf]]

Making this bug a 2.2 blocker because I'm spinning it off from a 2.2+ bug.

Comment 2

[David Flanagan [:djf]]

Alive: The PDF viewer app is a system app. It defines a View and an Open activity that are the same except that one is inline and the other has window disposition.  I'm pretty sure that the File Manager app is invoking the Open activity which is the window disposition one.

My guess is that the window management and task manager code never expects to see a regular app window with role=system.

As you can see in the video of this bug, when the PDF viewer is displayed and you bring up the task manager, it shows the file manager screenshot, not the pdf viewer screenshot.  There are two apps running, but only one is recognized.  When you kill it, my guess is that the window manager things that all apps are dead so the homescreen must be showing, not realizing that the pdf viewer window is still on top.

I will see if I can work around the bug by changing the open activity to be inline instead of window.  But since I don't know what might be relying on that, I think it would be better to have a patch that fixed this in the window management code.

On the other hand, maybe it just doesn't make sense for a role=system app to ever have a disposition=window activity. Maybe the system app should have code to convert to inline activities for role=system apps?

Comment 3

[David Flanagan [:djf]]

Johan: this is the new bug I filed for the other part of 1152841

Comment 4

[David Flanagan [:djf]]

I have confirmed that the STR cause the pdf viewer app to be launched via the "open" activity which has disposition=window.

And I've confirmed that killing the PDF viewer process resolves the bug and makes the homescreen start acting normally.

Comment 5

[David Flanagan [:djf]]

I have confirmed that modifying the open activity so it is disposition:inline instead of disposition:window fixes this bug.

I'm reluctant to use that as a workaround because I suspect that the pdf viewer uses a lot of memory and using an inline activity would not allow the initiating app to be killed, so we will be more likely to get OOMs if we make this switch.

The open activity was added by Aus as part of bug 1009780, so maybe he'll have an opinion about the safety of switching to disposition:inline

Comment 6

[David Flanagan [:djf]]

And I can confirm that making the PDF viewer app a normal app that is not role=system also fixes the bug. We can't use this as a workaround, however, because the PDF viewer does not work as a standalone app. It is only good for responding to activities.

I suppose maybe the fix here is to create a new app role. The PDF viewer isn't really a system app in any deep sense. It is just an app that should not appear on the homescreen but otherwise should be treated like an ordinary app by the task manager, etc.  So maybe if it was role=activity_handler instead of role=system or something we could resolve the bug easily.

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] davidflanagan:bug1154838 > mozilla-b2g:master

Comment 8

[David Flanagan [:djf]]

Comment on attachment 8592966 [details] [review]
[gaia] davidflanagan:bug1154838 > mozilla-b2g:master

This attachment works around the bug, at least on master.  I worry that switching from window dispostion to inline will have undesireable side effects like increased memory usage. So I'd prefer to land a system app fix rather than this workaround.  But if we need to, this workaround should fix things up for 2.2

Comment 9

[David Flanagan [:djf]]

Johan,

I've tested the attached patch on master, but have not tried it on 2.2. Would you confirm that it fixes the bug in 2.2?  And if it does, would you check to see whether it introduces any problems with viewing PDFs from the Browser app and download manager?  We don't want to make the pdf viewer work from 3rd party file manager apps if we break it in the built-in Browser app.

You'll need to test the Browser app itself, the download manager notification thing, and also the Settings->Downloads panel

Comment 10

[David Flanagan [:djf]]

Johan,

Also note that since the attached patch modifies the manifest of the pdf viewer app you can't just apply it and push the app to the phone. A full make reset-gaia is required in order to have the patch take effect.

Comment 11

[David Flanagan [:djf]]

Oh wow. This is really bad. It turns out that we don't need a 3rd party file manager to reproduce this bug. We can do it with the Settings->Downloads panel, too.

STR:

1) Open the browser app and download a PDF file

2) Open the Settings app and tap on Downloads

3) Tap on the PDF file and wait for it to display in the PDF viewer

4) Long press the Home button to bring up the task manager. Swipe up to kill all of the displayed apps.

5) Notice that you are automatically returned to the PDF viewer is still displayed and that the Home button now does not work.

Comment 12

[Aus Lacroix [:aus]]

Comment on attachment 8592966 [details] [review]
[gaia] davidflanagan:bug1154838 > mozilla-b2g:master

lgtm!

Comment 13

[David Flanagan [:djf]]

Taking this bug so I don't forget about it and so it is not an unowned blocker.

Alive: if you think you can fix this in the window manager, please take it from me. Or if you don't want to try to fix it, let me know and I can land this workaround patch assuming that Johan's testing goes well.

Comment 14

[Clément Lefèvre]

So, looking at the explanations you gave here on this bug, I allow myself to point out this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1110118 that was closed earlier.

The Privacy Panel part was fixed, but not the FTE part, and it looks to me that this is exactly the same bug as the behavior is the same and probably is it, the same way, an "app opened from another app".

Comment 15

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

This looks really bad :(

From the video, here is my guess:
The opened window activity(pdf viewer) is killed, and the desired behavior of a killed "active" app is going back to the activity caller app(file manager).

We should check what happened to block the home button after the caller app is opened due to the killing of a window disposition activity callee in this situation.

If we cannot avoid the second case, maybe we should not open the activity caller when there is task manager shown.

Could any of you check it in your spare time? Thanks.

Comment 16

[David Flanagan [:djf]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #15)
> This looks really bad :(
> 
> From the video, here is my guess:
> The opened window activity(pdf viewer) is killed, and the desired behavior
> of a killed "active" app is going back to the activity caller app(file
> manager).
> 

I don't think that is what is happening. The file manager is the primary app, and it opens the pdf viewer via a disposition=window activity. Then, when we bring up the task manager, we see the file manager, but we do not see the pdf viewer in the task manager at all. (I assume that is because the pdf viewer has role=system).  So what the user sees and kills in the task manager app is the file manager app.  The pdf viewer remains running.  But because all of the apps that the task manager knows about are now killed, the task manager exits.

I think the code is assuming that in this situation (where there are no apps displayed in the task manager) that the homescreen is displayed by default. The window manager has forgotten about the fact that the pdf viewer is displayed.  And since it thinks that it is already at the homescreen, pressing the home button does not do anything.

In any case, you can use adb shell b2g-ps to verify that the pdf viewer is not killed.  If you use adb shell kill to kill the pdf viewer, then you return to the homescreen and everything is working again.


> We should check what happened to block the home button after the caller app
> is opened due to the killing of a window disposition activity callee in this
> situation.
> 
> If we cannot avoid the second case, maybe we should not open the activity
> caller when there is task manager shown.
> 
> Could any of you check it in your spare time? Thanks.

Comment 17

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

David, thanks for the detail info. I will check what's really happening tomorrow. I don't have clue for what you described now, but anyway, I suggest not to change the behavior of window manager or task manager in a v2.2+ bug.

NI myself to open followup bug for it.

Comment 18

[Pi Wei Cheng [:piwei] (inactive)]

I'll be testing the patch on v2.2 since QAwanted was also tagged on comment 9.

Comment 19

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: logcat opening a PDF via File Manager

I can't seem to be able to view any PDF file from File Manager app after applying the patch. It just displays a blank PDF viewer. The same PDFs can be viewed BEFORE applying the patch. The same PDFs can be viewed after applying the patch using built-in PDF viewer in the system (accessed via Settings > Downloads or via tapping on notification).

Otherwise the patch works good on v2.2. No problem occurred following STR at comment 0 or comment 11. Homescreen can always be accessed after killing the PDF viewer in task manager.

Not sure if it helps but attaching a logcat opening a pdf from File Manager.

Tested with PDFs from irs.gov website.

Comment 20

[David Flanagan [:djf]]

Pi Wei,

Thanks for testing this. The issue with not being able to see the PDF was recently fixed in 1152841. That fix should be in the nightly generated tonight.

With your tests and mine, I'm confident that this patch fixes the bug. The patch changes the behavior of the open activity for PDFs (from an inline activity to a window activity) and I'm a little worried that it might affect other flows that can display PDFs.  So if I land this patch (and I think I have to) I need QA to be alert for regressions in anything that displays PDFs. Like receiving a PDF via email, mms (if that is possible), or bluetooth and viewing it.

Comment 21

[Pi Wei Cheng [:piwei] (inactive)]

I meant to say I was able to view PDF before the patch using File Manager, but after the patch I was no longer able to (view the PDF via File Manager). And after patch I can still view PDF from Settings > Downloads, just not from File Manager. So I thought something is odd there.

Comment 22

[David Flanagan [:djf]]

Alive: I agree that we shouldn't change the system app for this in 2.2. I'll land my workaround, and you can file a followup bug to sort this out in the window manager or task manager for 3.0.

Meanwhile, I've convinced myself that switching the activity to disposition:inline is the correct thing to do for the PDF viewer. Because the PDF Viewer is only useful as an activity handler we never want it to run with disposition:window. When the user is done reading their PDF, there is nothing else they can do with the app. There is no way to open up any other file, so they want to click the X icon and have the PDF viewer exit which is what we get with an inline activity.

Aus says that he didn't have any special reason for making the open activity disposition:window, and he doesn't think that the PDF viewer uses much memory so is not concerned about OOMs.  Andrew Sutherland says that the email app does not care what the disposition of the activity is.

So I'm going to go ahead and land this workaround and request uplift.

Comment 23

[David Flanagan [:djf]]

(In reply to Pi Wei Cheng [:piwei] from comment #21)
> I meant to say I was able to view PDF before the patch using File Manager,
> but after the patch I was no longer able to (view the PDF via File Manager).
> And after patch I can still view PDF from Settings > Downloads, just not
> from File Manager. So I thought something is odd there.

That does sound odd. I'll double-check that before landing.

Comment 24

[David Flanagan [:djf]]

I can't reproduce Pi Wei's finding from comment #21. My guess is that he had a nightly build with the fix for bug 1152841 in it, and could see the PDF. But then he applied the patch to an out-of-date 2.2 branch of gaia, and when he ran make reset-gaia he lost the patch for that bug.  If I apply this patch to an up-to-date 2.2 branch and run make reset-gaia, I see that both bugs are fixed, as expected.

Comment 25

[Pi Wei Cheng [:piwei] (inactive)]

David, I'm not sure how that works but I applied the patch from the clone of your Gaia repository. Is that method somehow doing an outdated action? But anyway as long as it works good on your end.

Comment 26

[David Flanagan [:djf]]

I've tested my patch with 2.2 and confirm that it fixes the bug.

Viewing PDFs received by email still works.

Viewing a PDF received by bluetooth does not work, but that appears to be an issue with the bluetooth download manager: it just does not know how to deal with pdf files.  It doesn't even attempt to open the pdf viewer, so I haven't broken anything here.

I can send a PDF via the SMS app. Though once I've sent it, I can't open it from the sms app (it doesn't know the type). And I can't open it when it is received on my android phone either, so maybe pdf support just isn't that good on Android either.

Comment 27

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/29d3a84bbe41d4a3d0496660ab6e97d2352cc8c2

Comment 28

[David Flanagan [:djf]]

Comment on attachment 8592966 [details] [review]
[gaia] davidflanagan:bug1154838 > mozilla-b2g:master

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #): I don't think this is a regression. I belive this has been broken ever since the download manager was added and we added the "open" activity with disposition:window here.

[User impact] if declined: it will be possible to get the phone into a state where the home button does not work and a reboot is required.

[Testing completed]: We don't have good tests for the PDF viewer. I've tested manually as has Pi Wei. Hopefully the QA team will be on the alert for any PDF viewer bugs related to this one or caused by this patch.

[Risk to taking this patch] (and alternatives if risky): By switching from a window activity to an inline activity we increase the risk of seeing OOMs when displaying large PDFs because the system has to keep both the viewer and the invoking app alive.  But that is really the right thing to do in this case anyway. And the alternative is to have a broken home button. I discussed teh risk with Aus and Asuth on IRC and they both thought we should make this change. So there is some risk, but there isn't really an alternative.

[String changes made]: none.

Comment 29

[Clément Lefèvre]

I allow myself to remind the comment I did at https://bugzilla.mozilla.org/show_bug.cgi?id=1154838#c14
This comment was pointing out another (closed) bug that was presenting the exact same problem. Half of this bug was really solved (the Privacy Panel part), but the other half concerning FTE panel was not.

Maybe is it possible to have a general fix for all of this, like at the task manager or window manager.
Otherwise, I guess it require to fix it as the way it's displayed like for PDF app (here it's opened by settings app)

Comment 30

[Johan Lorenzo [:jlorenzo]]

Sorry, I couldn't test the patch, yesterday. Thank for taking it Pi Wei!

(In reply to David Flanagan [:djf] from comment #24)
> I can't reproduce Pi Wei's finding from comment #21. My guess is that he had
> a nightly build with the fix for bug 1152841 in it, and could see the PDF.

I couldn't reproduce it too. I agree with your guess.

Comment 31

[bhavana bajaj [:bajaj]]

:ktucker, can we get some exploratory testing around this to see if there are fallouts? Thanks!

Comment 32

[Joshua Mitchell (Inactive)]

Verified Fixed on today's 3.0 nightly (full-flash, 319mem)

Actual Results: After swiping up to kill the File Manager app (step 5 in STR) the pdf app is no longer present (is killed with the File Manager app). 
Device returns to homescreen, no file manager or pdf screens remain.

Build ID: 20150420010204
Gaia: cb41d8421da5dc4f16ea566ea2917a9b7f828154
Gecko: 50b95032152c
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

Comment 33

[KTucker [:KTucker][Inactive 3/4/2016]]

Adding verifyme to check 2.2 once it has been uplifted.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/c57cedbd23988cf25cdc8419e0caca7efebee08e

Comment 35

[Shally Li（leave from Mozilla）]

This bug has been verified as pass on latest Nightly build of Flame v2.2 by the STR in Comment 0.

Actual results: After swiping up to kill the File Manager app (step 5 in STR) the pdf app is no longer present (is killed with the File Manager app). Device returns to homescreen, no file manager or pdf screens remain.

See attachment: verified_v2.2.mp4
Reproduce rate: 0/5

Device: Flame v2.2 build(Pass)
Build ID               20150513002507
Gaia Revision          e048df68f6f4853b5826a8816e143d95258149de
Gaia Date              2015-05-12 19:10:26
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0e6b4aab2b94
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150513.041549
Firmware Date          Wed May 13 04:15:58 EDT 2015
Bootloader             L1TC000118D0

Comment 36

[Shally Li（leave from Mozilla）]

Attachment: verified_2.2.mp4