1144132 - [Activity] Properly adjust the priority of activity openers to prevent them from being killed by the LMK when they are hidden

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #892371 +++

Currently the process priority manager doesn't take into account the status of an activity caller. Because of this the system app is forced to flag activity openers as visible even though they're hidden to prevent their priority to be lowered and then be killed by the LMK if we're short on memory.

In bug 892371 I landed a workaround that solves the issue of activity chains by adjusting oom_score_adj values of the visible apps in LRU order. This does not solve problems related to visibility such as bug 846850 or bug 1072874.

Activity information is already available in gecko (it landed as part of bug 887650 IIRC) so we should use it in the process priority manager to properly adjust the priority of the activity opener. At the same time we should let the system app flag the opener as hidden as that would solve the remaining problems.

Comment 1

[Gabriele Svelto [:gsvelto]]

I've been looking through the system app and the various gecko internals and I still don't fully understand how to properly utilize the additional information provided by bug 887650 which doesn't seem to be used anywhere in gaia or gecko. Fabrice, Alive, I need some extra information on how to handle this and I can't figure it out myself.

- I need a way to track activity openers. I've noticed that the system app tracks application -> browser frame mappings so I was wondering if the right way to do this is to have the system app add an attribute to the browser frame to indicate that it's an activity opener. However I don't know if this is the right way to approach it or if gecko should be tracking activity openers on its own. Provided this is the right approach, where should the tracking take place? When we create an ActivityWindow object?

- The second issue is that we need to adjust the visibility to mark the opener's visibility as hidden. I used to know where this happened by the relevant code was changed recently so I'm a bit lost right now. I know we keep the activity opener as visible as a workaround for this but I'm unsure where this is happening. Alive, could you point out where this is happening now?

My plan here is to develop two patches, one for gaia and one for gecko that would work in unison to solve this problem for good and decide if we want them in v2.2 depending on how complex they end up to be.

Comment 2

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

(In reply to Gabriele Svelto [:gsvelto] from comment #1)
> I've been looking through the system app and the various gecko internals and
> I still don't fully understand how to properly utilize the additional
> information provided by bug 887650 which doesn't seem to be used anywhere in
> gaia or gecko. Fabrice, Alive, I need some extra information on how to
> handle this and I can't figure it out myself.
> 
> - I need a way to track activity openers. I've noticed that the system app
> tracks application -> browser frame mappings so I was wondering if the right
> way to do this is to have the system app add an attribute to the browser
> frame to indicate that it's an activity opener. However I don't know if this
> is the right way to approach it or if gecko should be tracking activity
> openers on its own. Provided this is the right approach, where should the
> tracking take place? When we create an ActivityWindow object?
> 

I am not sure if Fabrice like this way - but if he agree, you are right to specify it in ActivityWindow.render() by referring this.rearWindow.manifestURL

> - The second issue is that we need to adjust the visibility to mark the
> opener's visibility as hidden. I used to know where this happened by the
> relevant code was changed recently so I'm a bit lost right now. I know we
> keep the activity opener as visible as a workaround for this but I'm unsure
> where this is happening. Alive, could you point out where this is happening
> now?

See what I did here
https://github.com/mozilla-b2g/gaia/pull/28864/files

> 
> My plan here is to develop two patches, one for gaia and one for gecko that
> would work in unison to solve this problem for good and decide if we want
> them in v2.2 depending on how complex they end up to be.

Comment 3

[Gabriele Svelto [:gsvelto]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #2)
> I am not sure if Fabrice like this way - but if he agree, you are right to
> specify it in ActivityWindow.render() by referring
> this.rearWindow.manifestURL

Yeah, I'm not sure if this is the right way to convey this information, but it's the only one I can think of right now. This would also require marking the opener as not an opener anymore when the window is closed. I suppose this can be done in AttentionWindow.close().

> See what I did here
> https://github.com/mozilla-b2g/gaia/pull/28864/files

Yes, that's what I was looking for.

Comment 4

[[:fabrice] Fabrice Desré]

I'm fine with annotating the activity opener manifest url, but I think this can be done by gecko itself. When the page calls |new MozActivity()| we know the principal of the page and from here the appId which is a 1 to 1 mapping to the manifest URL. The only tricky part will be to propagate that up to the system message dispatching.

Comment 5

[Gabriele Svelto [:gsvelto]]

I've finally found some time to work on this. For now I'm poking ActivitiesService.jsm to see if I can use it to track the openers; since it resides in the parent process it seems like a good place to start because the process priority manager also lives in the parent process.

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Adjust the priority of an activity opener so that it's above other background applications

This is a first crude fix for this problem. I'm using the observer service to surface when an activity is launched and when it's done through the activities manager. I've chosen two points that conservatively bracket the activity lifecycle so that the process priority manager is informed that an app is an activity opener before its visibility status might change. Similarly when the activity is closed we inform the process priority manager "late" in the process and we schedule the priority change with the standard delay so that again the visibility of the app has time to change before we adjust priorities.

At the core what this patch does is ensure that an activity opener's priority will be kept above the background level for as long as the activity is open and then some.

I've tested this using attachment 8577170 [details] [review] and it seems to work as expected.

If this is the right approach I'll complete it by adding tests and reverting the change in bug 892371 that puts the foreground apps in an LRU list (as that wouldn't be needed anymore). I would instead add the LRU behavior to the background perceivable priority level so that in chained activities (e.g. contacts -> SMS -> gallery) the older activity opener is killed first.

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 8615325 [details] [diff] [review]
[PATCH WIP] Adjust the priority of an activity opener so that it's above other background applications

Review of attachment 8615325 [details] [diff] [review]:
-----------------------------------------------------------------

That looks fine to me.

::: dom/activities/ActivitiesService.jsm
@@ +460,5 @@
>          break;
>      }
> +  },
> +
> +  removeCaller: function activities_removeCaller(id) {

nit: no need to name functions anymore

::: dom/ipc/ProcessPriorityManager.cpp
@@ +341,5 @@
>    ProcessPriority mPriority;
>    uint32_t mLRU;
>    bool mHoldsCPUWakeLock;
>    bool mHoldsHighPriorityWakeLock;
> +  bool mActivityOpener;

nit: mIsActivityOpener to be consistent with mHoldsXXX

Comment 8

[Gabriele Svelto [:gsvelto]]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> That looks fine to me.

Thanks, I wasn't sure because I've always thought that using the observer service to move information from/to different places within gecko was sometimes frowned upon :) I was originally considering to try and detect within the ContentParent object if an app was an activity opener or not but couldn't find a way to do so. The only thing I wish I could improve is the filtering: currently every particular priority manager is informed of *all* activities and then decides if it's its own activity or not. That's a bit crude but again I couldn't find another straightforward way of mapping them.

> nit: no need to name functions anymore

OK, old habits die hard :)

> nit: mIsActivityOpener to be consistent with mHoldsXXX

Right, it's clearer this way. I'll cook up a better patch with tests added and attach Alive's PR so that we can land both together.

Comment 9

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2 WIP] Adjust the priority of an activity opener so that it's above other background applications

Second take at this patch, now with BACKGROUND_PERCEIVABLE tasks kept in an LRU queue, FOREGROUND tasks not handled in LRU mode anymore and the resulting logic being somewhat simplified.

I've encountered two issues whoever I wanted to discuss prior to continuing. The first question is for Alive: I've tested my code using attachment 8577170 [details] [review] but I found out that it doesn't seem to cover all the cases. With your patch attached if I open the SMS app from the contacts app then contacts goes into the background as expected. If I proceed and try to open the gallery app to attach an image then the SMS app doesn't go into the background.

The second issue is more of a general problem I've encountered while writing tests for this (which I haven't finished). Using the manifest URL for identifying which content process is the activity opener seems to work for apps but it will certainly not work for processes which are not an app and thus don't have a manifest. To tackle this issue I was wondering if I could use a different approach for this: would it be sensible to listen to incoming Activity:Start messages and snoop outgoing Activity:FireSuccess/FireError messages right into the ContentParent object? That would allow me to identify directly which ContentParent is the opener irrespective of the manifest.

However I'm not familiar enough with how the message managers work and I'm not sure if it's possible to use them to listen to both a message that's been sent from the child to the parent (Activity:Start) and vice-versa (Activity:FireSuccess/FireError). Alternatively one might replace the latter part with a regular observer and a local notification sent when Activity:FireSuccess/FireError messages are sent.

Comment 10

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

Investigation so far: the _opened event from the 2nd activity window is dispatched to the app who opens the 1st activity window. Register event in capturing phase does not work. Will update if I find how to fix.

Comment 11

[[:fabrice] Fabrice Desré]

Gabriele, I'm not sure what you expect from me there. Activity:Start is in the parent, and fires back FireSuccess/FireError in the child that called |new MozActivity()|. We already keep an id to track the objects, so why not just use that here too? Or I'm missing something?

Comment 12

[Gabriele Svelto [:gsvelto]]

(In reply to Fabrice Desré [:fabrice] from comment #11)
> Gabriele, I'm not sure what you expect from me there. Activity:Start is in
> the parent, and fires back FireSuccess/FireError in the child that called
> |new MozActivity()|. We already keep an id to track the objects, so why not
> just use that here too? Or I'm missing something?

My problem is figuring out which content parent corresponds to the activity opener. With my patch I can identify one using the manifestURL but this doesn't work if the process doesn't have a manifest. See the code here, the manifest can be null:

http://hg.mozilla.org/mozilla-central/file/3d11cb4f31b9/dom/activities/ActivityProxy.js#l51

What I was wondering is if there's a way to intercept the Activity:Start message in the content parent so that I'd immediately know who's opening a new activity.

Comment 13

[[:fabrice] Fabrice Desré]

At this point you are in the right process. So if you send an observer notification that is observed by ContentParent you're guaranteed that the right ContentParent will get it.

Comment 14

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

(In reply to (inactive) Alive Kuo [:alive] from comment #10)
> Investigation so far: the _opened event from the 2nd activity window is
> dispatched to the app who opens the 1st activity window. Register event in
> capturing phase does not work. Will update if I find how to fix.

I think I fixed the issue. It's because AppWindowManager does not send the launchactivity request to the right person (should send to Message but mistakenly send to Contact). So the ChildWindowFactory put Contact into background. Now your case should work, lemme know if there's other issues.

Comment 15

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

(In reply to (inactive) Alive Kuo [:alive] from comment #14)
> (In reply to (inactive) Alive Kuo [:alive] from comment #10)
> > Investigation so far: the _opened event from the 2nd activity window is
> > dispatched to the app who opens the 1st activity window. Register event in
> > capturing phase does not work. Will update if I find how to fix.
> 
> I think I fixed the issue. It's because AppWindowManager does not send the
> launchactivity request to the right person (should send to Message but
> mistakenly send to Contact). So the ChildWindowFactory put Contact into
> background. Now your case should work, lemme know if there's other issues.

New PR is here 
https://github.com/mozilla-b2g/gaia/pull/30536

Comment 16

[Gabriele Svelto [:gsvelto]]

(In reply to (inactive) Alive Kuo [:alive] from comment #15)
> New PR is here 
> https://github.com/mozilla-b2g/gaia/pull/30536

It's working like a charm! Let's get this landed together with the gecko patch, or do you prefer opening a separate bug for the gaia one?

Comment 17

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v3] Adjust the priority of an activity opener so that it's above other background applications

Updated patch which now properly handles processes without a manifest URL. I pass along with the rest of the procedure the child ID of each process so that the process priority manager can easily match the openers with their ContentParent objects. I've also reverted the changes landed as part of bug 892371 because they were workarounds for this problem and we don't need them anymore. I've also added mochitests covering these scenarios.

Comment 18

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v4] Adjust the priority of an activity opener so that it's above other background applications

My patch still had a major gotcha: it didn't work in the single process case because I wasn't what ContentChild::GetSingleton() was returning. I've made a small change to amend it.

Comment 19

[[:fabrice] Fabrice Desré]

Comment on attachment 8620948 [details] [diff] [review]
[PATCH v4] Adjust the priority of an activity opener so that it's above other background applications

Review of attachment 8620948 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/activities/ActivitiesService.jsm
@@ +409,5 @@
>  
>      switch(aMessage.name) {
>        case "Activity:Start":
> +        Services.obs.notifyObservers(null, "activity-opened",
> +                                     msg.childID.toString());

nit: no need for toString()

@@ +464,5 @@
> +  },
> +
> +  removeCaller: function activities_removeCaller(id) {
> +    Services.obs.notifyObservers(null, "activity-closed",
> +                                 this.callers[id].childID.toString());

nit: same here

::: dom/activities/moz.build
@@ +36,5 @@
>  LOCAL_INCLUDES += [
>      '/dom/base',
>  ]
>  
> +include('/ipc/chromium/chromium-config.mozbuild')

why do we need that?

Comment 20

[Gabriele Svelto [:gsvelto]]

Thanks for the review! I'll attach a patch with the nits addressed.

(In reply to Fabrice Desré [:fabrice] from comment #19)
> > +include('/ipc/chromium/chromium-config.mozbuild')
> 
> why do we need that?

Apparently including ContentChild.h requires a bunch of generated headers which come with it. Building without fails claiming PContentBridgeChild.h cannot be found. I saw that other dom/ directories addressed the problem this way and copy-pasted it but I'm not 100% this is the right way.

Comment 21

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v5] Adjust the priority of an activity opener so that it's above other background applications r=fabrice

Patch with nits addressed, carrying over the r+ flag.

Comment 22

[Gabriele Svelto [:gsvelto]]

Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5413f31c79d0

Comment 23

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Activity opener visibility state

And here's Alive PR as an attachment. Alive shall we get it reviewed and land it alongside my gecko patch so that the issue is  fixed for good after we close this bug?

Comment 24

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

Let's get this reviewed. I will update some tests and request review from Tim or Etienne.

Comment 25

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

Comment on attachment 8621981 [details] [review]
[PULL REQUEST] Activity opener visibility state

Does this convince you?
1. Use setVisible with a special argument which indicates do-not-propagate
2. Turn on/off visibility once the front activity window is opened/closing.

Comment 26

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

(In reply to (inactive after 6/18) Alive Kuo [:alive] from comment #25)
> Comment on attachment 8621981 [details] [review]
> [PULL REQUEST] Activity opener visibility state
> 
> Does this convince you?
> 1. Use setVisible with a special argument which indicates do-not-propagate
> 2. Turn on/off visibility once the front activity window is opened/closing.

BTW I feel we should deprecate VisibilityManager and use HierarchyManager to manage the visibility state soon.

Comment 27

[Etienne Segonzac (:etienne)]

Comment on attachment 8621981 [details] [review]
[PULL REQUEST] Activity opener visibility state

Sounds about right. r=me with the test-related comments addressed!

Comment 28

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

Looks like the sms test about draft saving is failed but I don't really know why.
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=f9d95f8f2d72d7f674c334cce367450b5a33d734

Julien, are you able to help? I am not sure what is done in sms and the test here.

Comment 29

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

(In reply to (inactive after 6/18) Alive Kuo [:alive] from comment #28)
> Looks like the sms test about draft saving is failed but I don't really know
> why.
> https://treeherder.mozilla.org/#/
> jobs?repo=gaia&revision=f9d95f8f2d72d7f674c334cce367450b5a33d734
> 
> Julien, are you able to help? I am not sure what is done in sms and the test
> here.

The root cause is: message draft was saved once when the attachment picker is opened, so when the test tries to click back button in message app it will not show 'Save as draft' but 'Replace existing draft'.

However there is a problem here about tapping the attachment adding button; it's intermittent to lose the tap event. I tried to change tap() to click() everything works. :/

Comment 30

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

Gabriele, I think we need to check in the gecko patch before the gaia patch. Could you help once it's r+ed?

Comment 31

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

Comment on attachment 8621981 [details] [review]
[PULL REQUEST] Activity opener visibility state

Steve please help to check the sms test change!

Comment 32

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

(In reply to (inactive after 6/18) Alive Kuo [:alive] from comment #30)
> Gabriele, I think we need to check in the gecko patch before the gaia patch.
> Could you help once it's r+ed?

Also we need to tell dev-gaia now all activity opener is expected to be background. If there is any workaround for this behavior it's time to change. And maybe we need to watch any regressions after patches are merged.

Comment 33

[Oleg Zasypkin [:azasypkin]]

(In reply to (inactive after 6/18) Alive Kuo [:alive] from comment #31)
> Comment on attachment 8621981 [details] [review]
> [PULL REQUEST] Activity opener visibility state
> 
> Steve please help to check the sms test change!

Please, note that we don't have "messagesApp.Composer.attachButton.tap();" on master anymore, Tim has changed this in bug 1174062 - so you'll likely need to rebase your PR.

Comment 34

[Steve Chung [:steveck]]

I'm fine with the test changes(after you rebased), but I just have one question for the UX:

Hi Jenny, this patch might change some behavior in message app that picking the attachment will force the message app into background and triggered draft saving. So it lead every new message with attachment will be saved while picking the attachment and shows replace option while pressing back button. It seems a little bit confusing that showing replace option for this case. Do you think we should create a follow up to address this issue?

Comment 35

[Steve Chung [:steveck]]

Comment on attachment 8621981 [details] [review]
[PULL REQUEST] Activity opener visibility state

r+ for the test changes

Comment 36

[Jenny Lee]

Hi Steve,

You are right, "Replace draft" should only appear after user leave a "new message" and come back again. The flow described in comment 34 doesn't make sense. Thanks!

Comment 37

[Gabriele Svelto [:gsvelto]]

(In reply to (inactive after 6/18) Alive Kuo [:alive] from comment #30)
> Gabriele, I think we need to check in the gecko patch before the gaia patch.
> Could you help once it's r+ed?

Yes, let's land the gecko patch (attachment 8621979 [details] [diff] [review]).

The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5413f31c79d0

We'll keep the bug open until the gaia part lands, I'll also send an heads up to the dev-gaia and dev-b2g mailing lists so that we look out for regressions.

Comment 38

[Julien Wajsberg [:julienw]]

(In reply to Jenny Lee from comment #36)
> Hi Steve,
> 
> You are right, "Replace draft" should only appear after user leave a "new
> message" and come back again. The flow described in comment 34 doesn't make
> sense. Thanks!

But we should _still_ save the draft in that case !

Maybe we should just remove the dialog as we discussed in the past.

Comment 39

[Gabriele Svelto [:gsvelto]]

Whoops, I forgot the checkin-needed flag. As per comment 37, we need to land attachment 8621979 [details] [diff] [review]

The related try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5413f31c79d0

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/d983ba03e272

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/a5fcb169083e

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d983ba03e272
https://hg.mozilla.org/mozilla-central/rev/a5fcb169083e

Comment 43

[Gabriele Svelto [:gsvelto]]

Etienne, Steve, is there anything missing or can we merge Alive's PR and close this bug?

Comment 44

[Etienne Segonzac (:etienne)]

(In reply to Gabriele Svelto [:gsvelto] from comment #43)
> Etienne, Steve, is there anything missing or can we merge Alive's PR and
> close this bug?

All my comments have been addressed :)

Comment 45

[Steve Chung [:steveck]]

I create a follow up bug in bug 1176976 and let's land this PR first.

Comment 46

[Gabriele Svelto [:gsvelto]]

Excellent, in it goes then and if something goes horribly wrong in the meantime we'll back it out tomorrow.

Comment 47

[Gabriele Svelto [:gsvelto]]

Merged to gaia/master eb0d4aefa62b20420d6fa0642515a110daca5d97

https://github.com/mozilla-b2g/gaia/commit/eb0d4aefa62b20420d6fa0642515a110daca5d97

Let's hope this sticks.