Cannot have two activities with same name and different filters

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: fcampo, Assigned: ferjm)

Tracking

unspecified
FxOS-S2 (10Jul)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [NG Gaia Contacts][patch])

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Description

4 years ago
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.
Whiteboard: [NG Gaia Contacts]
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!
Flags: needinfo?(ferjmoreno)
Flags: needinfo?(fabrice)
Assignee: nobody → ferjmoreno
Flags: needinfo?(ferjmoreno)
Flags: needinfo?(fabrice)
Status: NEW → ASSIGNED
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
    }
    ]
}
Posted patch v1 (obsolete) — Splinter Review
Working on a test now
Attachment #8627094 - Flags: review?(fabrice)
Posted patch v1 (obsolete) — Splinter Review
Attachment #8627094 - Attachment is obsolete: true
Attachment #8627094 - Flags: review?(fabrice)
Attachment #8627095 - Flags: review?(fabrice)
Target Milestone: --- → FxOS-S2 (10Jul)
Component: Gaia::Contacts → DOM: Apps
Product: Firefox OS → Core
Attachment #8627095 - Flags: review?(fabrice)
Posted patch v1 (obsolete) — Splinter Review
Attachment #8627095 - Attachment is obsolete: true
Attachment #8627774 - Flags: review?(fabrice)
Posted patch Tests (obsolete) — Splinter Review
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?
Attachment #8627779 - Flags: feedback?
Attachment #8627779 - Flags: feedback? → feedback?(fabrice)
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...
Attachment #8627774 - Flags: review?(fabrice) → review+
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?
Attachment #8627779 - Flags: feedback?(fabrice) → feedback+
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]
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
Flags: needinfo?(nfroyd)
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?
Flags: needinfo?(nfroyd)
Posted patch v2Splinter Review
I needed to modify the way we unregister activities as well.
Attachment #8627774 - Attachment is obsolete: true
Attachment #8628422 - Flags: review?(fabrice)
Posted patch TestsSplinter Review
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
Attachment #8627779 - Attachment is obsolete: true
Attachment #8628424 - Flags: review?(fabrice)
Attachment #8628422 - Flags: review?(fabrice) → review+
Comment on attachment 8628424 [details] [diff] [review]
Tests

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

thanks!
Attachment #8628424 - Flags: review?(fabrice) → review+
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
Status: RESOLVED → REOPENED
Flags: needinfo?(ferjmoreno)
Resolution: FIXED → ---
I'll note that this failure did show up in the try push in comment 14.
Yes, you are right. I missed that one. Sorry about that.
Flags: needinfo?(ferjmoreno)
Whiteboard: [NG Gaia Contacts] → [NG Gaia Contacts][patch]
Depends on: 1065501
The easy fix will be to just wait a few days for bug 1065501 to decide to hide and stop running Gip.
Gip will still keep on running, but not on Treeherder. It still means those tests will fail once this is checked in.
No longer depends on: 1065501
Attachment #8629979 - Attachment is obsolete: true
Depends on: 1180727
The problem was in the test itself. I'll reland once bug 1180727 is fixed.
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?
Flags: needinfo?(jlorenzo)
(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.
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.
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.
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.
Flags: needinfo?(jlorenzo)
https://hg.mozilla.org/mozilla-central/rev/f848e6877ca4
https://hg.mozilla.org/mozilla-central/rev/004e123383ae
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

2 years ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.