Closed Bug 887650 Opened 7 years ago Closed 7 years ago

[Activity][SystemMessage] Add new argument to system message to let gaia know the web activity caller

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

(blocking-b2g:koi+)

RESOLVED FIXED
blocking-b2g koi+

People

(Reporter: alive, Assigned: alive)

References

Details

(Whiteboard: [TD-57881])

Attachments

(2 files, 5 obsolete files)

Bug 880588 doesn't fix all the problems of inline activity's visibility.

1. Activity opener should be setVisible(false) when activity is opened(activityopen event).
However we may have OOM problem about this, see https://bugzilla.mozilla.org/show_bug.cgi?id=880588#c7
Briefly, if an app is having inline activity, we should lessen the OOM_ADJ of the app, to make it not easy to be killed by OOM killer even when it's in background.

2. If multiple inline activities are opened, the middleware activities visibility is not set to hidden.
This is because bug 846850 and gecko fix bug 853759.
Briefly, requested activity shouldn't close itself when visibility is hidden. Gecko should do that for them since app itself won't be notified when it's crashed/killed (I think).
This is a stupid question, but can you give me an example of how to trigger an inline activity?
example (1): Long press on homescreen -> wallpaper
example (2): Type any number in dialer -> click + -> open new contact activity -> click + on photo -> open another pick camera photo activity
Steps to repro an issue:

1. Start playing a video.
2. Receive image/audio by Bluetooth from other device.
3. Open the received file.
4. Observe the video is still playing, when the visibility has been moved away.
Severity: normal → critical
blocking-b2g: --- → leo+
Whiteboard: [TD-57881]
Target Milestone: --- → 1.1 QE4 (15jul)
Fix (1) would fix comment 3.
However I still concern the memory management malfunction.
(In reply to Alive Kuo [:alive] from comment #4)
> Fix (1) would fix comment 3.
> However I still concern the memory management malfunction.

I would r- a patch which did only (1) because of precisely the problem you described.
(In reply to Justin Lebar [:jlebar] from comment #5)
> (In reply to Alive Kuo [:alive] from comment #4)
> > Fix (1) would fix comment 3.
> > However I still concern the memory management malfunction.
> 
> I would r- a patch which did only (1) because of precisely the problem you
> described.

No idea how to move forward now.
I think we still need Fabrice or someone to feedback?

BTW before that, how do you think give gaia more API about memory management?
Is it OK for gaia system to touch (read and write) OOM-ADJ directly?

Until now I only know background frame has hight OOM ADJ than foreground, but I don't the accurate meaning of OOM ADJ value 0-6.
Flags: needinfo?(fabrice)
> Is it OK for gaia system to touch (read and write) OOM-ADJ directly?

No; we should handle this in Gecko, as we currently do.  Gaia just needs to give Gecko enough information so it can make the right decision.

We had a similar problem with the lock screen.  In this situation, the problem was that we were setVisible(false)'ing the topmost frame when you turned the phone off.  But that causes us to discard all images in that frame.

There's no reason to discard the images in the frame that's going to be shown when you unlock the screen, but the only information Gecko got was "this frame is not visible", and so it couldn't make a good decision.

I use the past tense above, but that bug may not be fixed; I forget what we ended up doing.

Anyway, this is similar.

I think the only question here is, do we have Gaia explicitly tell Gecko "this frame opened an inline activity", or do we rely on Gecko to keep track of this?  Does Gecko have enough information to know when a frame opened an inline activity and that activity is in the foreground?

Either way, once Gecko knows that a frame opened an inline activity, it can set the oom_adj appropriately.
(In reply to Justin Lebar [:jlebar] from comment #7)
> > Is it OK for gaia system to touch (read and write) OOM-ADJ directly?
> 
> I think the only question here is, do we have Gaia explicitly tell Gecko
> "this frame opened an inline activity", or do we rely on Gecko to keep track
> of this?  Does Gecko have enough information to know when a frame opened an
> inline activity and that activity is in the foreground?

NOPE. This is also my question. We even don't know who is the activity opener but just give a guess that the current app is the one who opens it because we know that inline activity is triggered by user action.

 543   openAppForSystemMessage: function shell_openAppForSystemMessage(msg) {
 544     let origin = Services.io.newURI(msg.manifest, null, null).prePath;
 545     this.sendChromeEvent({
 546       type: 'open-app',
 547       url: msg.uri,
 548       manifestURL: msg.manifest,
 549       isActivity: (msg.type == 'activity'),
 550       target: msg.target,
 551       expectingSystemMessage: true
 552     });
 553   },

The event detail is insufficient, indeed, in the case of comment because the real one to invoke activity is in the system app(bluetooth transfer), not the current active app.

I also desire and acquire the information if we want to manage activity correctly, not only on setting visibility.
Gecko gives a hint to gaia to let it know that the activity must be inlined. Then gaia does whatever it needs and don't send anything back to gecko explicitly, but we have a hook point where gecko get a handle to the window that has been opened for the activity. Here we should be able to know if it's actually an inline activity or not.
So, while true my last comment was not really relevant to what we need here. Discussing with Alive on irc, we want to:
- change the oom_adj value for the caller of an activity.
- restore its previous value when the activity is done.

And gaia could use the manifest url and page url of the caller.
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #10)
> So, while true my last comment was not really relevant to what we need here.
> Discussing with Alive on irc, we want to:
> - change the oom_adj value for the caller of an activity.
> - restore its previous value when the activity is done.
> 
> And gaia could use the manifest url and page url of the caller.

YES.

Gaia part work would be: correctly set the visibility of activity caller app, per information given by gecko, when activity is started/terminated.

Gecko part: besides the stuff :fabrice mentioned, we need to consider an activity "callee" may also open another activity so its OOM score also need to be adjusted. I think you guys know more detail than me.
Depends on: 892371
Let's track the gecko part fix in bug 892371.
Assignee: nobody → alive
WIP without gecko tell me who is the inline activity caller.

https://github.com/alivedise/gaia/commit/1679ed57c7ebae7b295635b58a784f21f99a3e42
:fabrice, please help to give some advise..
I think not every system message would have a caller,
but I don't want to add 'type=activity' check in system-message-internal..don't know if there's better way without affecting other system message format.
Attachment #775593 - Flags: review?(fabrice)
Comment on attachment 775593 [details] [diff] [review]
Proposed gecko fix about appending activity caller info in system message

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

I don't like very much the idea of adding activity-specific information there in system messages, but I don't have yet a really better idea.
One minor enhancement would be instead of adding "manifestURL" and "pageURL" to sendMessage() to just pass an opaque "extra" object. This would carry less activity-specfic semantics.

If you can make this change, I would not object until I come with a something nicer....
Attachment #775593 - Flags: review?(fabrice) → review-
I know it's dirty ;) Pollute the middleware |SystemMessageInternal| does make me unhappy, too.
But I think we need to carry additional info without changing SystemMessageInternal, and this kind of requirement would getting more in the future.

  _openAppPage: function _openAppPage(aPage, aMessage) {
    // We don't need to send the full object to observers.
    let page = { uri: aPage.uri,
                 manifest: aPage.manifest,
                 type: aPage.type,
                 target: aMessage.target };
    debug("Asking to open " + JSON.stringify(page));
    Services.obs.notifyObservers(this, "system-messages-open-app", JSON.stringify(page));
  },

Why do we reorganize the message into page object here instead of sending page + message directly?
Does your 'extra' live in the page object as well?
(In reply to Alive Kuo [:alive] from comment #16)

> Why do we reorganize the message into page object here instead of sending
> page + message directly?

As the comment says, we don't want to send the full page and message objects, so we build a new one.

> Does your 'extra' live in the page object as well?

yes.

But it may be even cleaner to add the "extra" data as an optional parameter to https://mxr.mozilla.org/mozilla-central/source/dom/messages/interfaces/nsISystemMessagesInternal.idl#22 and broadcastMessage(). At least that would make this clear for all callers.
(In reply to Fabrice Desré [:fabrice] from comment #17)
> 
> But it may be even cleaner to add the "extra" data as an optional parameter
> to
> https://mxr.mozilla.org/mozilla-central/source/dom/messages/interfaces/
> nsISystemMessagesInternal.idl#22 and broadcastMessage(). At least that would
> make this clear for all callers.

This looks clear to go.
Though I do think this change is not very trivial..
The optional parameter is only meaningful to js caller but not c++ caller.
There c++ caller needs to assign an undefined value to the function.

Note: bluetooth has two functions in different files using broadcastMessage, they seem to be able to be merged into one.
Attachment #776982 - Flags: review?(fabrice)
Attachment #776982 - Flags: feedback?(gyeh)
Comment on attachment 776982 [details] [diff] [review]
Patch v2: Add optional parameter for sendMessage/broadcastMessage

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

Looks great for Bluetooth part. I think we should also use BroadcastSystemMessage() in BluetoothService.cpp (as we did in other files). Let me open a follow-up for this. Thanks for your effort.
Attachment #776982 - Flags: feedback?(gyeh) → feedback+
See last review request +
* Change UUID
Attachment #776982 - Attachment is obsolete: true
Attachment #776982 - Flags: review?(fabrice)
Attachment #777033 - Flags: review?(fabrice)
Comment on attachment 777033 [details] [diff] [review]
Patch v2: Add optional parameter for sendMessage/broadcastMessage

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

r=me with nits addressed and pending Gene review.

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +17,5 @@
>     * @param type        The type of the message to be sent.
>     * @param message     The message payload.
>     * @param pageURI     The URI of the page that will be opened.
>     * @param manifestURI The webapp's manifest URI.
> +   * @param extra       Extra information

nit: "Extra opaque information that will be passed around in the observer notification to open the page".

@@ +26,5 @@
>     * Allow any internal user to broadcast a message of a given type.
>     * The application that registers the message will be launched.
>     * @param type        The type of the message to be sent.
>     * @param message     The message payload.
> +   * @param extra       Extra information

Nit: same here.
Attachment #777033 - Flags: review?(gene.lian)
Comment on attachment 777033 [details] [diff] [review]
Patch v2: Add optional parameter for sendMessage/broadcastMessage

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

Giving review- because one line is obviously wrong...

::: dom/messages/SystemMessageInternal.js
@@ +172,1 @@
>                                           aManifestURI.spec);

This is wrong! |aExtra| should be the last argument...
Attachment #777033 - Flags: review?(gene.lian) → review-
Attached patch Patch v3: fix the argument issue (obsolete) — Splinter Review
Thank you guys for review.

Gene, please help to check this again. I'd test manually nothing bad happens.
Attachment #777033 - Attachment is obsolete: true
Attachment #777033 - Flags: review?(fabrice)
Attachment #777660 - Flags: review?(gene.lian)
Attachment #775593 - Attachment is obsolete: true
In system app, extra info got when it's activity request:

E/GeckoConsole(  108): Content JS LOG at app://system.gaiamobile.org/js/window_manager.js:1325 in WindowManager</<: [alive] {"type":"open-app","url":"app://wallpaper.gaiamobile.org/pick.html","manifestURL":"app://wallpaper.gaiamobile.org/manifest.webapp","isActivity":true,"target":{"filters":{"type":["image/*","image/jpeg"],"width":320,"height":480},"disposition":"inline","returnValue":true,"href":"app://wallpaper.gaiamobile.org/pick.html"},"expectingSystemMessage":true,"extra":{"manifestURL":"app://homescreen.gaiamobile.org/manifest.webapp","pageURL":"app://homescreen.gaiamobile.org/index.html#root"}}

No extra info if it's not activity:

E/GeckoConsole(  108): Content JS LOG at app://system.gaiamobile.org/js/window_manager.js:1325 in WindowManager</<: [alive] {"type":"open-app","url":"app://clock.gaiamobile.org/index.html","manifestURL":"app://clock.gaiamobile.org/manifest.webapp","isActivity":false,"expectingSystemMessage":true}
Comment on attachment 777660 [details] [diff] [review]
Patch v3: fix the argument issue

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

Looks good to me. Btw, you need to make b2g18-specific patch since we had some differences in the SystemMessageInternal.js and shell.js.

::: dom/messages/SystemMessageInternal.js
@@ +161,5 @@
>      // clean it up from the pending message queue when apps receive it.
>      let messageID = gUUIDGenerator.generateUUID().toString();
>  
>      debug("Sending " + aType + " " + JSON.stringify(aMessage) +
> +      " for " + aPageURI.spec + " @ " + aManifestURI.spec + '; extra=' + JSON.stringify(aExtra));

nit: s/=/:/.

We prefer that within this file although I don't have strong opinion.

@@ +182,5 @@
>      if (page) {
>        // Queue this message in the corresponding pages.
>        this._queueMessage(page, aMessage, messageID);
>  
>          if (result === MSG_SENT_FAILURE_APP_NOT_RUNNING) {

Since you're here, could you please align this if-block? Thanks!

@@ +195,5 @@
>      // so that we can know the correct pages registered to be broadcasted.
>      if (!this._webappsRegistryReady) {
>        this._bufferedSysMsgs.push({ how: "broadcast",
>                                     type: aType,
> +                                   extra: aExtra,

Put this property after |msg| to align the order of arguments.

@@ +204,5 @@
>      // Give this message an ID so that we can identify the message and
>      // clean it up from the pending message queue when apps receive it.
>      let messageID = gUUIDGenerator.generateUUID().toString();
>  
>      debug("Broadcasting " + aType + " " + JSON.stringify(aMessage));

Add debug message for |aExtra| just like what you did for sendMessage(...).

@@ +580,5 @@
>                                   { type: aType,
>                                     msg: aMessage,
>                                     manifest: aManifestURI,
>                                     uri: aPageURI,
> +                                   msgID: aMessageID});

Don't need this change. Please recover it.

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +17,5 @@
>     * @param type        The type of the message to be sent.
>     * @param message     The message payload.
>     * @param pageURI     The URI of the page that will be opened.
>     * @param manifestURI The webapp's manifest URI.
> +   * @param extra       Extra opaque information that will be passed around in the observer

nit: add an period at the end of this sentence.

@@ +27,5 @@
>     * Allow any internal user to broadcast a message of a given type.
>     * The application that registers the message will be launched.
>     * @param type        The type of the message to be sent.
>     * @param message     The message payload.
> +   * @param extra       Extra opaque information that will be passed around in the observer

Ditto.
Attachment #777660 - Flags: review?(gene.lian) → review+
Attached file Patch to checkin for mozilla-central (obsolete) —
I am told by wayne they may want to leo? this one so let's wait..
I will make another b2g18 patch if needed.

The gaia part fix depends on 892371, but this gecko patch is safe to go without other change.
(Update bug commit info)
Attachment #777702 - Attachment is obsolete: true
Uh..I think we also need to know 'which' activity is done in 'activity-done' event.
Nominate leo? since the dependent 892371 isn't an easy one to go.
blocking-b2g: leo+ → leo?
Removing leo+ and milestone as no longer blocker by leo.
blocking-b2g: leo? → koi?
Target Milestone: 1.1 QE4 (15jul) → ---
checkin needed for mc
Keywords: checkin-needed
Attachment #777660 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4ee3d4c78ae0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Summary: [System] Manage the inline activity's visibility correctly → [Activity][SystemMessage] Add new argument to system message to let gaia know the web activity caller
blocking-b2g: koi? → koi+
Depends on: 1144132
You need to log in before you can comment on or make changes to this bug.