Closed Bug 846850 Opened 12 years ago Closed 9 years ago

[Unagi][Camera][Activity][Contacts] App doesn't get 'mozvisibilitychange' event when running as an activity

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(b2g18+)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
b2g18 + ---

People

(Reporter: mikeh, Assigned: alive)

References

Details

Observed on Unagi with:
- gecko: inbound-src:33a198f5593d
- gaia: 3ad2a0445acb2057db75eb107ee64288f36c0ac6

I noticed this while trying to reproduce bug 846262.

Following the STR in that bug and watching the logcat, I noticed that the camera preview doesn't stop while the screen is off, and the camera hardware isn't released.  It behaves as if the camera isn't getting the 'mozvisibilitychange' event.

If the camera app is launched directly from the home screen, pressing the power button causes the preview to stop and the hardware to be released, so this seems to only happen when the camera is used in the image picker activity.

logcat is attached to bug 846262:
https://bugzilla.mozilla.org/attachment.cgi?id=720020
Summary: [Unagi][Camera][Activity][Contacts] App doesn't get 'mozvisibilitychange' event when called as an activity → [Unagi][Camera][Activity][Contacts] App doesn't get 'mozvisibilitychange' event when running as an activity
blocking-b2g: --- → tef?
tracking-b2g18: --- → ?
(clearing tef? as bug report was not from the v1.0.1 branch.)
blocking-b2g: tef? → ---
Can this affect power usage?
tracking-b2g18: ? → +
(In reply to Andrew Overholt [:overholt] from comment #2)
>
> Can this affect power usage?

It most certainly will: the camera continues to run after the power button is pressed to turn off the screen.
This is a general issue in system app.
Why it isn't fixed is because this may introduce some regression by the app incorrectly uses mozvisiblitychange event. So we use some workaround to avoid changing to logic now.
Bug 846262 is an example.
Taken but wait for activity review from gaia side to do fix if possible(hope we would).
Assignee: nobody → alive
I think we have a dead lock in this issue.

If we want the activity caller is informed the callee is removed with postError(),
we must do it in the activity callee.
Currently it's achieved by adding this code to "every" activity callee:

```
-    // When the app is being closed or killed, we will cancel the pending
-    // request.
-    document.addEventListener('mozvisibilitychange', function visibility(e) {
-      if (!document.mozHidden || !activityRequest)
-        return;
-
-      activityRequest.postError('canceled');
-      activityRequest = null;
-    });
```

I think this is a dirty workaround, because with this code we couldn't achieve the multiple activities correctly. Now system doesn't set the first activity to visible=false because the callee(contact app) thinks the only way to detect it's remove/closed is by mozvisibilitychange event.

To solve this, maybe we need new API to know the frame is closing instead of watching visible state.
Fabrice, Justin, any idea?

I just try window.onunload and it works, but IMO make every activity callee to postError by itself still sounds a bad idea, how if they forget to do? Could we do this natively in gecko?
Flags: needinfo?(justin.lebar+bug)
Flags: needinfo?(fabrice)
Why can't we just use the visibilitychange event?  You said

> Why it isn't fixed is because this may introduce some regression by the app incorrectly 
> uses mozvisiblitychange event.

but could you be specific here?  Can we just fix these regressions, whatever they are?
Flags: needinfo?(justin.lebar+bug)
(In reply to Justin Lebar [:jlebar] from comment #7)
> Why can't we just use the visibilitychange event?  You said
> 
> > Why it isn't fixed is because this may introduce some regression by the app incorrectly 
> > uses mozvisiblitychange event.
> 
> but could you be specific here?  Can we just fix these regressions, whatever
> they are?

Use case:
Dialer=>Open a 'new Contact' activity to Contact, Contact=>open a 'pick photo' activity to Camera.

The visible state should be:
(A)

      ----TOP----
      Camera(activity)  visible=true
      Contact(activity) visible=false
      Dialer(app)       visible=false
      ----BOTTOM----

But now it's

(B)
      ----TOP----
      Camera(activity)  visible=true
      Contact(activity) visible=true
      Dialer(app)       visible=false
      ----BOTTOM----


Because contact needs to do postError() to let the Dialer knows it's removed by some reason(by system app in case another app is opened now).
And the only way to know it's close is mozvisibilitychange event or window.onunload

If I correct the visible state to (A) without doing anything to Contact app,
when camera is triggered by activity request from contact:

(C)
      ----TOP----
      Contact(activity) visible=true           => click add a photo to trigger another activity
      Dialer(app)       visible=false
      ----BOTTOM----

(D)

      ----TOP----
      Camera(activity)  visible=true
      Contact(activity) visible=true------>false by system app------>knows visibilitychange---->close itself by postError----> implictly remove itself------> the activity chain is broken.
      Dialer(app)       visible=false
      ----BOTTOM----
Okay, but this is clearly a bug in the contact app, right?  It should not be assuming that a visibilitychange to false is the same thing as an unload.

Indeed, if you lock the phone, it will get a visibilitychange to false.  Should locking the phone cause the activity to close itself?
(In reply to Justin Lebar [:jlebar] from comment #9)
> Okay, but this is clearly a bug in the contact app, right?  It should not be
> assuming that a visibilitychange to false is the same thing as an unload.
> 
> Indeed, if you lock the phone, it will get a visibilitychange to false. 
> Should locking the phone cause the activity to close itself?

Yeah, it shouldn't do this.

What I propose is, in dom/activities, when the iframe knows it's removed(I guess gecko would know this right?), postError by itself if it never does postSuccess or postError before. Then we could get rid of all workaround in gaia.
Sure, we probably should do that in Gecko.  Maybe mounir can help with that.  But I still don't get why we can't simply change the current visibilitychange listeners to unload listeners.
(In reply to Justin Lebar [:jlebar] from comment #11)
> Sure, we probably should do that in Gecko.  Maybe mounir can help with that.
> But I still don't get why we can't simply change the current
> visibilitychange listeners to unload listeners.

It's still a workaround and should/would be removed in the future right?

If this is not a dead project and many dev would come to the world of FxOS to create/develop their app,
web activity would be a very strong weapon. They surely would implement one if they need.
I really don't want to see we need to tell devs "YOU MUST ADD THESE LINES OF CODES TO YOUR APP EVEN IT WOULD BECOME REDUNDANT IN SOME DAY", if we have a correct way to do that.

This is just a proposal and if no platform dev would love to spend his time with me,
I, for sure, would do that s/document.addEventListener('mozvisibilitychange', function()/window.onunload/gi stuff though that's sad.
> It's still a workaround and should/would be removed in the future right?

Yes, I totally agree that we should ensure at the Gecko level that activities should always either succeed or fail.  But I don't actually know much about how activities work, so maybe Mounir has a different idea.
(In reply to Justin Lebar [:jlebar] from comment #13)
> > It's still a workaround and should/would be removed in the future right?
> 
> Yes, I totally agree that we should ensure at the Gecko level that
> activities should always either succeed or fail.  But I don't actually know
> much about how activities work, so maybe Mounir has a different idea.

NVM, thanks for the comments.
Flags: needinfo?(mounir)
I am completely unfamiliar with the activity/visibility relationship. Fabrice might help regarding this.

About the Web Activity design, if the called activity isn't expected to return something, the postError/postSuccess should fire immediately when an app has been selected (or if the cancel button has been used or if there is no app).
If the activity is expected to return a result, it is up to the activity to send that result back. I guess the contact activity is returning something? What is the activity exactly?
Flags: needinfo?(mounir)
> If the activity is expected to return a result, it is up to the activity to send that 
> result back.

I think the suggestion is that we should ensure that postError/postSucces always fires.  If when the activity page unloads the activity still hasn't fired postError/postSuccess, we should fire postError for it.

Even if you think that we should be relying on pages to be correct -- and I don't think we should rely on that -- we need to do that anyway because a correct page might be killed due to low-memory before it has a chance to postError/postSuccess.
I agree with Justin - Let's fire postError when unloading the app before we have fired anything.

Alive, if you don't feel comfortable doing the gecko patch, reassign to me.
Flags: needinfo?(fabrice)
(In reply to Justin Lebar [:jlebar] from comment #16)
> > If the activity is expected to return a result, it is up to the activity to send that 
> > result back.
> 
> I think the suggestion is that we should ensure that postError/postSucces
> always fires.  If when the activity page unloads the activity still hasn't
> fired postError/postSuccess, we should fire postError for it.
> 
> Even if you think that we should be relying on pages to be correct -- and I
> don't think we should rely on that -- we need to do that anyway because a
> correct page might be killed due to low-memory before it has a chance to
> postError/postSuccess.

I didn't went into details but our design was that applications that lose focus should immediately send an error event. I thought Gaia's window manager was doing that...
> I didn't went into details but our design was that applications that lose focus should 
> immediately send an error event.

Indeed, that's the current design.

But what happens if you lock the screen while an activity is focused?  The activity will become invisible, and lose focus.  Should we close the activity?  It seems like we postError but don't close it, which is pretty wrong...
(In reply to Justin Lebar [:jlebar] from comment #19)
> > I didn't went into details but our design was that applications that lose focus should 
> > immediately send an error event.
> 
> Indeed, that's the current design.
> 
> But what happens if you lock the screen while an activity is focused?  The
> activity will become invisible, and lose focus.  Should we close the
> activity?  It seems like we postError but don't close it, which is pretty
> wrong...

IMO, we shouldn't close the activity handler nor send an error event in that case. If we go OOM, we should indeed send an error event though.
> IMO, we shouldn't close the activity handler nor send an error event in that case. If we 
> go OOM, we should indeed send an error event though.

I agree.
(In reply to Mounir Lamouri (:mounir) from comment #18)
> (In reply to Justin Lebar [:jlebar] from comment #16)
> > > If the activity is expected to return a result, it is up to the activity to send that 
> > > result back.
> > 
> > I think the suggestion is that we should ensure that postError/postSucces
> > always fires.  If when the activity page unloads the activity still hasn't
> > fired postError/postSuccess, we should fire postError for it.
> > 
> > Even if you think that we should be relying on pages to be correct -- and I
> > don't think we should rely on that -- we need to do that anyway because a
> > correct page might be killed due to low-memory before it has a chance to
> > postError/postSuccess.
> 
> I didn't went into details but our design was that applications that lose
> focus should immediately send an error event. I thought Gaia's window
> manager was doing that...

WM cannot do postError "for the app".
The only thing WM could do is to remove the iframe in some cases. (Like another app is opened by someone when an app is having inline activities.)
If I misunderstand, please correct me.
(In reply to Justin Lebar [:jlebar] from comment #21)
> > IMO, we shouldn't close the activity handler nor send an error event in that case. If we 
> > go OOM, we should indeed send an error event though.
> 
> I agree.

Agree, too. But what's the event here? Is it 'mozbrowsererror, type=fatal'?
If so, only System app is notified and due to #22 WM couldn't postError for the app.

I am fine if you think system app should do that but I don't see how WM could do it. The same, if I misunderstand the API, please correct me.
(In reply to Fabrice Desré [:fabrice] from comment #17)
> I agree with Justin - Let's fire postError when unloading the app before we
> have fired anything.
> 
> Alive, if you don't feel comfortable doing the gecko patch, reassign to me.

Glad to learn to patch gecko but it may take long time ;)
If we agree to do what I proposed, I'd like to create another platform bug instead of on this one.
Alive, why do you need another bug? We should really not do a gaia workaroud if the right fix is in gecko.
(In reply to Fabrice Desré [:fabrice] from comment #25)
> Alive, why do you need another bug? We should really not do a gaia workaroud
> if the right fix is in gecko.

I am suggesting we have gecko and gaia fix in different bug and mark the dependency.
The gaia side should be to remove the original workaround and some fix in WM so I will take it. Any concern?
Sounds good to me! I did realize you needed to remove the existing workaround.
Blocks: 853759
Blocks: 880588
No longer blocks: 853759
Depends on: 853759
Blocks: 887650
Blocks: 898307
Alive, the dependent bug is fixed. What's the next step here?
Flags: needinfo?(alive)
We're still blocked by real gecko fix bug 892371 to avoid OOM killer to kill activity caller.
Depends on: 892371
Flags: needinfo?(alive)
Depends on: 1144132
This was fixed after bug 1144132.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.