920616 - SpecialPowers.swapFactoryRegistration incorrectly uses the 'real' CID when swapping out a factory

Status: RESOLVED WONTFIX

(Testing :: Mochitest, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

<jaws> anybody know why MockObjects.js is going unused? i think it's leaving the replaced service in a broken state
<jaws> bsmedberg: do you see anything possibly wrong here? http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#1456
<jaws> in my case, i'm replace the prompt service. if i show a prompt in test1 and then test2 replaces the prompt service with a mock object, the mock object's code never runs
<jaws> a similar issue happens if i use a mock object in test1 and then test2 expects the prompt to be displayed
<jaws> in the third case, if i use a mock object in test1 and then try to use a mock object in test2, the test2 mock object code is never run
<jaws> so i think the juggling act within http://hg.mozilla.org/mozilla-central/annotate/681a8e611ede/testing/specialpowers/content/specialpowersAPI.js#l1456 is not swapping them correctly if the service has already been instantiated or it's been swapped out before
<jaws> test1 and test2 are separate test files
<jaws> in the second case that i described above, the prompt is never displayed
<bsmedberg> jaws: there are two mappings within the component manager
<bsmedberg> CID -> factory
<bsmedberg> contractid -> CID
<bsmedberg> once a CID is mapped to a factory, you can never replace it
<bsmedberg> it's basically permanent
<bsmedberg> contractID -> CID you can replace
<jaws> ok, so should this mock object functionality just be removed since by design it's basically broken?
<bsmedberg> so all of these mock things should be careful to never re-use a CID... the mock object should always use a different CID than the "real" service
<bsmedberg> and also you need to make sure that all of the clients get services by contract ID and not by CID
<bsmedberg> so you basically can mock a contractid, but not a CID
<jaws> ok, i think it's using the real CID
<jaws> this line in particular, http://hg.mozilla.org/mozilla-central/annotate/681a8e611ede/testing/specialpowers/content/specialpowersAPI.js#l1464
<bsmedberg> yeah, that's not good
<bsmedberg> jaws: that should perhaps be making a completely random CID

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch

Benjamin, can you take a look at this patch? I think I made the correct changes, but it didn't fix the issue I was running in to.

For example:
Test1 shows prompts during the test run and does not swap the prompt service out.
Test2 shows propmts during the test run but swaps out the prompt service.

Even though there was an attempt to swap out the prompt service during Test2, the original prompt service is used and a modal dialog is shown.

Comment 2

[:Paolo Amadini]

<bsmedberg> once a CID is mapped to a factory, you can never replace it
<bsmedberg> it's basically permanent

For the record, this was introduced as part of bug 568691 in the original mockObjects.js module, then it was somehow lost in the history of the module.

Actually, I find the current swapFactoryRegistration logic quite contrieved - can we just implement that with simpler "register" and "unregister" methods?

It would be a nice improvement to share this functionality across all frameworks as a test-only JSM (but I've no idea how that could interact with SpecialPowers).

Comment 3

[Benjamin Smedberg]

What's test1 and test2? Can you get a stack at the point the prompt is shown, so that we can see who the promptservice client is and how they are getting the prompt service?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

test1 is browser_bug896291.js (will be introduced by bug 896291)
test2 is a refactored browser_homeDrop.js (modified by bug 918069)

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

This bug is in the process of being abandoned. After bug 896291 is fixed, there will be no consumers of this file and we can just remove it from the tree.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Filed bug 928425 to remove MockObjects.js from the tree.