1176712 - Cannot have two activities with same name and different filters

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Fernando Campo (:fcampo)]

While running tests for NGA Contacts app tried to divide activities in the form:

"activities": {
    "open": {
      "filters": {
        "type": ["text/vcard", "text/x-vcard", "text/directory"]
      },
      "disposition": "inline",
      "href": "/contacts/vcard.html",
      "returnValue": true
    },
    "open": {
      "filters": {
        "type": "webcontacts/contact"
      },
      "disposition": "inline",
      "href": "/contacts/details.html",
      "returnValue": true
    }
}

...results in only the last one ["open": "webcontacts/contact"] being registered on the system.

This behaviour is consistent if the activities are treated as an object (same key properties are overwritten with the latest), but feels like an underuse of the filters. 

We should be able to redirect to different pages depending on the filter, even for same type of activities, and not use an opening page that redirects after checking the filter.

Comment 1

[Borja Salguero [:borjasalguero]]

Fernando, Fabrice, Could you help us on this? It's part of the NGA work that we are doing and it's critical to ensure that we are loading the right handler for the right activity. Thanks!

Comment 2

[[:fabrice] Fabrice Desré]

fwiw, we decided to allow this kind of use case by allowing arrays:

"activities": {
    "open": [
    {
      "filters": {
        "type": ["text/vcard", "text/x-vcard", "text/directory"]
      },
      "disposition": "inline",
      "href": "/contacts/vcard.html",
      "returnValue": true
    },
    {
      "filters": {
        "type": "webcontacts/contact"
      },
      "disposition": "inline",
      "href": "/contacts/details.html",
      "returnValue": true
    }
    ]
}

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Working on a test now

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

This test is passing perfectly when it's run alone, but it fails when it is run along with test_dev_mode_activity.html with this error:

JavaScript error: resource://gre/modules/ActivitiesService.jsm, line 412: NS_ERROR_XPC_CI_RETURNED_FAILURE: Component returned failure code: 0x80570015 (NS_ERROR_XPC_CI_RETURNED_FAILURE) [nsIJSCID.createInstance]

We are probably doing something wrong with the registration/unregistration of the mock components. Fabrice, any idea of why is this failing?

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 8627774 [details] [diff] [review]
v1

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

::: dom/activities/ActivitiesService.jsm
@@ +117,5 @@
> +        // indexes
> +        objectStore.createIndex("name", "name", { unique: false });
> +        objectStore.createIndex("manifest", "manifest", { unique: false });
> +
> +        self.add(activities, () => {

I think you can use |this| here since it's in an arrow function.

::: dom/apps/Webapps.jsm
@@ +1009,2 @@
>        }
> +      for (let i = 0; i < entry.length; i++) {

nit: entry.forEach() ? no big deal anyway...

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 8627779 [details] [diff] [review]
Tests

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

lgtm.
In https://mxr.mozilla.org/mozilla-central/source/dom/activities/tests/mochi/test_dev_mode_activity.html?force=1#114 we don't catch exceptions. Can you check if everything actually works properly?

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Unregistration seems to work ok. But it seems that for some reason we are using the unregistered classID in following calls:

19 INFO TEST-PASS | dom/activities/tests/mochi/test_dev_mode_activity.html | All apps are uninstalled.
20 INFO Unregistering {44d0e409-f181-0649-a4f5-b03e41ecc7a5}
21 INFO QQQ {00000000-0000-0000-c000-000000000046}
UnregisterFactory
Unregister OK
22 INFO Unregistering {c743ea35-407f-ea43-abcc-f1a276a6a3ca}
UnregisterFactory
Unregister OK
MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
MEMORY STAT | vsize 3300MB | residentFast 374MB | heapAllocated 118MB
23 INFO TEST-OK | dom/activities/tests/mochi/test_dev_mode_activity.html | took 1590ms
++DOMWINDOW == 18 (0x130c19400) [pid = 27326] [serial = 18] [outer = 0x1310f0c00]
24 INFO TEST-START | dom/activities/tests/mochi/test_same_name_multiple_filters.html
++DOMWINDOW == 19 (0x116d3f400) [pid = 27326] [serial = 19] [outer = 0x1310f0c00]
25 INFO Registering {647cc929-2bc6-554e-a120-74d15c1e0d99}
26 INFO QueryInterface {00000000-0000-0000-c000-000000000046}
27 INFO nsISupports nsISupports
28 INFO nsIFactory nsIFactory
29 INFO nsIActivityUIGlue nsIActivityUIGlue
30 INFO returning this
RegisterFactory cid {647cc929-2bc6-554e-a120-74d15c1e0d99} name Activity Glue contractID @mozilla.org/dom/activities/ui-glue;1
Registration OK
31 INFO Registering {8c69985a-8dd6-504a-aaa3-7d9ee10eb6cf}
32 INFO QI {00000000-0000-0000-c000-000000000046}
33 INFO Returning this
RegisterFactory cid {8c69985a-8dd6-504a-aaa3-7d9ee10eb6cf} name System Message Glue contractID @mozilla.org/dom/messages/system-message-glue;1
Registration OK
34 INFO Starting with 14 apps installed.
Early return line 1124
Creating instance failed.
         CID: {44d0e409-f181-0649-a4f5-b03e41ecc7a5}
         IID: {3caef69f-3569-4b19-bcea-1cfb0fee4466}
JavaScript error: resource://gre/modules/ActivitiesService.jsm, line 412: NS_ERROR_XPC_CI_RETURNED_FAILURE: Component returned failure code: 0x80570015 (NS_ERROR_XPC_CI_RETURNED_FAILURE) [nsIJSCID.createInstance]

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Nathan, could you give me any pointer here, please?

The problem that I am seeing is the following:

- We have one test that registers a mock component at [1]. In the log that I added in the previous comment this component is registered with cid {44d0e409-f181-0649-a4f5-b03e41ecc7a5}.
- The instance for this component is created at [2]
- The test passes with no problems.
- The unregistration of that component is done properly at [3]. You can see the log "Unregistering {44d0e409-f181-0649-a4f5-b03e41ecc7a5}" and the "UnregisterFactory" and "Unregister OK" messages that I added in nsComponentManager.

- I have this new test that I am adding in this bug that registers that very same mock component, but with a different cid. In the example above, the cid is {647cc929-2bc6-554e-a120-74d15c1e0d99}
- When I run the test, it fails cause when it tries to create the instance of that component at [2] it uses the old cid from the previous test ({44d0e409-f181-0649-a4f5-b03e41ecc7a5}) which is already unregistered.

If I run the new test independently, it works properly.

This is probably the same reason why bug 1162281 was backed out.

Thanks in advance for any help!

[1] https://mxr.mozilla.org/mozilla-central/source/dom/activities/tests/mochi/test_dev_mode_activity.html?force=1#165
[2] https://mxr.mozilla.org/mozilla-central/source/dom/activities/ActivitiesService.jsm#340
[3] https://mxr.mozilla.org/mozilla-central/source/dom/activities/tests/mochi/test_dev_mode_activity.html?force=1#173

Comment 11

[Nathan Froyd [:froydnj]]

What you describe sounds like it ought to work.  Two questions:

1) Where is the "Early return line 1124" in the log coming from?
2) What is the stack (C++ and JS) for the "Creating instance failed" bit?  i.e. who still has a reference to the old CID and is trying to instantiate it?

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

I needed to modify the way we unregister activities as well.

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

The issue with the tests is that we cannot register/unregister a component with different CID but same contract ID multiple times [1]. So I left the same CID for both tests.

Now everything is working fine.

[1] https://mxr.mozilla.org/mozilla-central/source/xpcom/components/nsComponentManager.cpp#1939

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34a3eeda10bc

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 8628424 [details] [diff] [review]
Tests

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

thanks!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6baeaddb32cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/52fd1c8b0fcb

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6baeaddb32cc
https://hg.mozilla.org/mozilla-central/rev/52fd1c8b0fcb

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for Gip(f5) permafail in https://hg.mozilla.org/integration/mozilla-inbound/rev/aa6a2ac2f2e6

https://treeherder.mozilla.org/logviewer.html#?job_id=11303236&repo=mozilla-inbound

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'll note that this failure did show up in the try push in comment 14.

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Yes, you are right. I missed that one. Sorry about that.

Comment 21

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/0b901209064c

Comment 22

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/e586cdf86004

Comment 23

[Pulsebot]

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

Comment 24

[Phil Ringnalda (:philor)]

The easy fix will be to just wait a few days for bug 1065501 to decide to hide and stop running Gip.

Comment 25

[Martijn Wargers (dead)]

Gip will still keep on running, but not on Treeherder. It still means those tests will fail once this is checked in.

Comment 26

[GitHub Autolander Bot]

Attachment: [gaia] ferjm:bug1180727.browser.ui.test > mozilla-b2g:master

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

The problem was in the test itself. I'll reland once bug 1180727 is fixed.

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

New try build with bug 1180727 fixed https://treeherder.mozilla.org/#/jobs?repo=try&revision=63ffabeb091d

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

It seems that the test still fails although bug 1180727 landed :(

I am quite unfamiliar with Gaia UI tests. Johan, do you know if the change for bug 1180727 is already available for Gaia try or do we need to wait for something before it is available there?

Comment 30

[Martijn Wargers (dead)]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #29)
> It seems that the test still fails although bug 1180727 landed :(
> 
> I am quite unfamiliar with Gaia UI tests. Johan, do you know if the change
> for bug 1180727 is already available for Gaia try or do we need to wait for
> something before it is available there?

I tried your fix with my local tree, which I then flashed my Flame device with. After that I ran the test_browser_share_link.py test and it passed, so I think you can safely land the patch. Otherwise, in the case it would fail, you can disable the test and me or somebody else can look at the test and try to fix it.

I don't see anything failing with the treeherder link in comment 28, btw.

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

Thank you Martijn.

I don't know why treeherder is not showing the test run and the failure, but they were there yesterday.

I just retried https://treeherder.mozilla.org/#/jobs?repo=try&revision=afaf0e8e9bdf

If it fails again, I'll land with the test disabled.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f848e6877ca4
https://hg.mozilla.org/integration/mozilla-inbound/rev/004e123383ae

Comment 33

[Martijn Wargers (dead)]

Gip is now hidden on Treeherder (see bug 1180903), so there's no need for a back-out as to regards to that test, anyway.

Comment 34

[Johan Lorenzo [:jlorenzo]]

Sorry, I'm not familiar with how the tests are updated for Gecko. My assumption would be to wait until the next merge between Gaia and Gecko to happen.

In any case, like Martijn mentioned, we hid Gip on treeherder.

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f848e6877ca4
https://hg.mozilla.org/mozilla-central/rev/004e123383ae