1032068 - [Activity Management] Drop background activity on new request

Status: RESOLVED FIXED

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

Description

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

See 
https://bugzilla.mozilla.org/show_bug.cgi?id=1028502#c2
https://bugzilla.mozilla.org/show_bug.cgi?id=1028800#c12

Comment 2

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

After thinking again...the idea might be tough.
Because we won't know who is requesting the activity while we get activity-choice event,
we could just only guess: the top most window or the system app itself.

Even we know the request is coming from system app, it's hard to say it's related to current active app window or not because anchor link will trigger browse activity which is called from b2g shell.js
We could not tell it's because user click a link during he is performing some activity task or not.

Still trying to figure out if we have a good enough workaround..

Comment 3

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

Attachment: https://github.com/mozilla-b2g/gaia/pull/21258

This is gaia workaround of bug 931339.
You could test with sheet-app-1/sheet-app-2/sheet-app-3.

In bug 916709, we didn't kill the activity on home button pressed;
so now if you launch activity of sheet-app-1 in sheet-app-2 and then go to sheet-app-3 to launch sheet-app-1 as inline activity, you will see the close button of the later activity of sheet-app-1 disappears. This is because gecko is sending system message to the first matching window using manifestURL + pageURL.

Before gecko fixes the way identifying window, to enable bug like bug 1028502 to work correctly, here is my proposal to workaround it in gaia side:

* Maintain a list of instanceID in activityWindowManager.
* Once there's any new activityrequesting(from activities.js which is wrapper of activity-choice event), put the caller (the top most window)'s instanceID into the pool if the pool is empty.
* Once there's any new window created(popup, activity, app), check the parent window instanceID of the new window is in the pool or not.
** If it's in the pool, we think this window is part of the task of the current activity chaining.
** If it's not in the pool, we think the user is launching another app to do other stuff.
* Once there's a new activityrequesting got when the pool is not empty,
and the current active window is not in the pool, we will kill all the background inline activities.
* The instanceIDs in the pool will be removed by *terminated events to ensure clean.

Note:
* This does NOT fix the circular activity chaining issues.
** If sheet-app-1 calls sheet-app-2 calls sheet-app-1 we cannot do anything for now. Yes, we could kill all but it's somewhat out of range of this bug. I will open another bug to fix it.
* Sometimes system app is firing activity request, too. Maybe from gaia side or shell.js; we cannot tell this kind of request is related to the current task chain or not. For now we just guess they are all related to the top most window.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8449269 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21258

This is a very complex patch. It will take me a few more day to look at it.

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 8449269 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21258

Small comments on github, 1 question:

> Note:
> * This does NOT fix the circular activity chaining issues.

I think it's fine, but since we're not handling this could we just track the bottomMost/rootWindow and simplify the patch (ie. do we really need the chaining mechanism?).


Also I wonder how complex/appropriate bubbling an activity request through a mozbrowser event (instead of a mozChromeEvent) would be, maybe we should ask :)

Comment 6

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

(In reply to Etienne Segonzac (:etienne) from comment #5)
> Comment on attachment 8449269 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/21258
> 
> Small comments on github, 1 question:
> 
> > Note:
> > * This does NOT fix the circular activity chaining issues.
> 
> I think it's fine, but since we're not handling this could we just track the
> bottomMost/rootWindow and simplify the patch (ie. do we really need the
> chaining mechanism?).
> 
> 
> Also I wonder how complex/appropriate bubbling an activity request through a
> mozbrowser event (instead of a mozChromeEvent) would be, maybe we should ask
> :)

The idea was rejected by fabrice ago. Maybe we should ask again.

Comment 7

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #6)
> (In reply to Etienne Segonzac (:etienne) from comment #5)
> > Comment on attachment 8449269 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/21258
> > 
> > Small comments on github, 1 question:
> > 
> > > Note:
> > > * This does NOT fix the circular activity chaining issues.
> > 
> > I think it's fine, but since we're not handling this could we just track the
> > bottomMost/rootWindow and simplify the patch (ie. do we really need the
> > chaining mechanism?).
> > 
> > 
> > Also I wonder how complex/appropriate bubbling an activity request through a
> > mozbrowser event (instead of a mozChromeEvent) would be, maybe we should ask
> > :)
> 
> The idea was rejected by fabrice ago. Maybe we should ask again.

If my memory serves me right his concern is browser app and other app has mozbrowser API permission needs to deal with activity launch request on its own.

Comment 8

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

(In reply to Etienne Segonzac (:etienne) from comment #5)
> Comment on attachment 8449269 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/21258
> 
> Small comments on github, 1 question:
> 
> > Note:
> > * This does NOT fix the circular activity chaining issues.
> 
> I think it's fine, but since we're not handling this could we just track the
> bottomMost/rootWindow and simplify the patch (ie. do we really need the
> chaining mechanism?).
> 

We will need it at least for inline<->window.

The use case is email app couldn't be inline so if someone share something to email and it's using inline then we don't have a 'pure inline chaining'.

Comment 9

[Etienne Segonzac (:etienne)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #8)
> (In reply to Etienne Segonzac (:etienne) from comment #5)
> > Comment on attachment 8449269 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/21258
> > 
> > Small comments on github, 1 question:
> > 
> > > Note:
> > > * This does NOT fix the circular activity chaining issues.
> > 
> > I think it's fine, but since we're not handling this could we just track the
> > bottomMost/rootWindow and simplify the patch (ie. do we really need the
> > chaining mechanism?).
> > 
> 
> We will need it at least for inline<->window.
> 
> The use case is email app couldn't be inline so if someone share something
> to email and it's using inline then we don't have a 'pure inline chaining'.

Makes sense, thanks for the clarification.

Comment 10

[Etienne Segonzac (:etienne)]

(In reply to Etienne Segonzac (:etienne) from comment #5)
> Also I wonder how complex/appropriate bubbling an activity request through a
> mozbrowser event (instead of a mozChromeEvent) would be, maybe we should ask
> :)

Vivien tells me there's a simpler alternative already used for permission prompts.

Basically it's to keep the mozChromeEvent but dispatch it from the correct iframe, so we can inspect evt.target in the system app to know where it came from.

Comment 11

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

(In reply to Etienne Segonzac (:etienne) from comment #10)
> (In reply to Etienne Segonzac (:etienne) from comment #5)
> > Also I wonder how complex/appropriate bubbling an activity request through a
> > mozbrowser event (instead of a mozChromeEvent) would be, maybe we should ask
> > :)
> 
> Vivien tells me there's a simpler alternative already used for permission
> prompts.
> 
> Basically it's to keep the mozChromeEvent but dispatch it from the correct
> iframe, so we can inspect evt.target in the system app to know where it came
> from.

I knew this trick but wonder it does not give much help.
Because sometimes system app or b2g process behaves the activity caller for the active app
* when app uses window.open to call 'mailto:' or other protocol.
* when click on image there will be mozbrowsercontextmenu event to trigger activity call in BrowserContextMenu

I think gecko has difficult to distinguish because it just knows system app is the caller but in these cases the one triggers them is the top most app. System app still needs to do the dispatch (decide who is caller).

Comment 12

[[:fabrice] Fabrice Desré]

Correct me if I'm wrong, but is the core issue the fact that we route system message (including activities) to already opened apps? If so we should just let some system messages work with multiple instances properly. You should not have to workaround that in gaia.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8449269 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21258

r+, since we have test in place to verify the logic is working.

As an improvement, if there is no rush, I think the pool should be implemented with a Map ... the some() forEach() splice() usages in this patch is complex and can be confusing to future developers.

Comment 14

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

(In reply to Fabrice Desré [:fabrice] from comment #12)
> Correct me if I'm wrong, but is the core issue the fact that we route system
> message (including activities) to already opened apps? If so we should just
> let some system messages work with multiple instances properly. You should
> not have to workaround that in gaia.

I am happy to remove any workaround if bug 931339 is fixed. But it seems there's no ETA is given and no one is actively on it, we need to do something to prevent failures.

Comment 15

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

Comment on attachment 8449269 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21258

Using Map for pool, asking review again.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8449269 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21258

Not sure if you can use the instances directly as the key (instead of |instanceID|) but that works too.

Comment 17

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

The error seems unrelated so merged, reopen if disagree.
https://github.com/mozilla-b2g/gaia/commit/b19616b0534b7edcb7d17a402ccccbc9af70c59d

Comment 18

[Luke Chang [:lchang] (inactive)]

[Blocking Requested - why for this release]:
This bug blocks bug 1075353 which is a 2.0+ bug. Also, one of the issues in bug 1084247 can be fixed by this bug as well.

Comment 19

[Andrew Overholt [:overholt]]

Blocks a blocker = 2.0+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Please request Gaia v2.0 approval on this patch when you get a chance :)

Comment 21

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

I think Luke had a patch for v2.0 already? Could you append it and ask for review/approval?

Comment 22

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8449269 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21258

Request the approval directly since the original patch can be uplifted to v2.0 w/o conflicts and passes the local tests.


[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): it blocks bug 1075353 (2.0+)
[User impact] if declined: bug 1075353 still happens on v2.0
[Testing completed]: local tests are passed
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: N/A

Comment 23

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/98ef6bda06d850aef5963734dbdc63b1495c263d

Comment 24

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/98ef6bda06d850aef5963734dbdc63b1495c263d

Comment 25

[Shine(Leave from Mozilla)]

Please provide the bug reproduce steps and videos, which could help with our verification.

Comment 26

[Hubert Lu[:hlu] <[e-mail]>]

(In reply to Shine from comment #25)
> Please provide the bug reproduce steps and videos, which could help with our
> verification.

There are no specific user behavior for this issue, you could need to verify this one.