788466 - System Message API: shell.js would attempt to open apps twice which is redundant

Status: RESOLVED DUPLICATE of bug 796293

(Core :: DOM: Core & HTML, defect)

Description

[Gene Lian (I already quit Mozilla)]

I discovered this bug during powering-up. If we print out the system message in shell.js whenever catching the "system-messages-open-app" observer event. The following message for opening homescreen app would be fired twice:

{"uri":"app://homescreen.gaiamobile.org/index.html","manifest":"app://homescreen.gaiamobile.org/manifest.webapp","type":"activity","target":{"filters":{"type":"application/x-application-list"},"href":"app://homescreen.gaiamobile.org/index.html"}}

Other apps could probably have the same issue.

Comment 1

[Gene Lian (I already quit Mozilla)]

I'll be able to upload a patch for solving this. ;)

Comment 2

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Hi Fabrice,

I think we don't need to register the pages that have already been registered before. Sometimes, the activities would register different activity types but actually share the same page.

I could be wrong. Please let me if you don't agree on these changes.

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 658454 [details] [diff] [review]
Patch

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

This will break apps that register activities with different filters that are handled by the same page, since from the message handler point of view, these are just "activity" messages.

Comment 4

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Comment on attachment 658454 [details] [diff] [review]
> Patch
> 
> Review of attachment 658454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This will break apps that register activities with different filters that
> are handled by the same page, since from the message handler point of view,
> these are just "activity" messages.

To be honest, I don't quite understand this part. As you know, the page queue is only indexed by the following items:

  page.type
  page.manifest
  page.uri

And for each page within the queue, we've already had:

  page.pending[]

to collect the message that we're going to send to this page. That's why I think we need to avoid registering duplicated pages, which could happen in DOMApplicationRegistry._registerActivities(); otherwise, for each single activity message, it will attempt to consecutively open the same app more than twice if we had redundant pages registered in the queue. Please see SystemMessageInternal._sendMessage() for more details.

No matter how we consider it. I still think there must be something wrong to consecutively send "open-app" event twice from shell.js, where both of them carry exactly the same message info.

Does that sound reasonable to you?

Comment 5

[[:fabrice] Fabrice Desré]

I'm not saying that the current behavior is correct. I just disagree with the patch you proposed.

When an app registers these activities:

 "activities": {
    "pick": {
      "filters": {
        "type": "webcontacts/contact"
       },
      "disposition": "window",
      "href": "/contacts/index.html",
      "returnValue": true
    },
    "new": {
      "filters": {
        "type": "webcontacts/contact"
      },
      "disposition": "window",
      "href": "/contacts/index.html",
      "returnValue": true
    }
  }

we need different pending queues for each activity, even if from the System Message manager point of view, they both have:
page.type = "activity"
paget.url = "/contact/index.html"
page.manifest = "app://comm.gaiamobile.org/manifest.webapp"

Currently we workaround that in gaia by adding a parameter to the href and checking the activity name in the app. The homescreen should do that also before we find a proper solution.

Comment 6

[Gene Lian (I already quit Mozilla)]

Hmm... understand better! :)

How about adding another attribute in the page when it comes to page.type = "activity" to distinguish different purposes (like "pick" or "new")? Does that make sense from the System Message API point of view?

Comment 7

[[:fabrice] Fabrice Desré]

I'd like to not special-case the Web Activities usage in System Messages. What should work is that instead of registering a tuple (type, pageURI, manifestURI), we let callers pass (type, hash) and match based on this.

If we don't want to have all callers duplicate the same kind of hash function, we could do the hashing in SystemMessageInternal.js, and have the method signature be (type, [strings]).

We do such a hashing at https://mxr.mozilla.org/mozilla-central/source/dom/activities/src/ActivitiesService.jsm#72

Comment 8

[Gene Lian (I already quit Mozilla)]

Thanks Fabrice! Sounds interesting! I'll come back with the new patch applied with your suggested approach.

Comment 9

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> I'd like to not special-case the Web Activities usage in System Messages.
> What should work is that instead of registering a tuple (type, pageURI,
> manifestURI), we let callers pass (type, hash) and match based on this.
> 
> If we don't want to have all callers duplicate the same kind of hash
> function, we could do the hashing in SystemMessageInternal.js, and have the
> method signature be (type, [strings]).

I'm still kind of confused about how to distinquish different activity names here. Do you mean the [strings] here could be assigned by callers as below:

  [pageURI, manifestURI, activityName]

If yes, we would encounter another issue that the SystemMessageManager:GetPending request only needs to match the pageURI and manifestURI, which means we cannot compare hash keys between [pageURI, manifestURI, activityName] and [pageURI, manifestURI] since we're not aware of what attributes are in the hash key exactly .

Did I misunderstand what you're saying?

Comment 10

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2

Hi Fabrice,

This is a logically right patch. Due to comment 9, we still need more discussions. May I have your suggestions? Thank you!

Comment 11

[Gene Lian (I already quit Mozilla)]

Let's close this one if Bug 796293 has already been checked in, which seems adopting exactly the same solution in my first patch here. ;)