696834 - Imported securable modules are not singletons

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Attachment: Testbed demonstrating modules aren't singletons

Some investigation into Mozmill shows that when you require a module more than once in a chain, the module is re-evaluated and re-created on every require.

So:

Test requires B
B requires C
Test requires C

Module C will get created twice: once when B requires it, and once when the Test directly requires it.

Unfortunately, this has some nasty implications:

1) There's no way to implement a singleton object that's in scope or easily importable for the entire project. 

For example, you might want harness runtime options or a singleton that tracks AUT state in some manner. As it is now, if you tried to put it in a module and import it you'd get one instance of the singleton on each require and they won't share state. So, you end up having to pass it around or inject it. 

That also means there's no real opportunity to make such a singleton usable from module setup code, since setup code will execute before any injection could happen.

2) Circular references behave strangely. If A requires B, B requires C, and C requires A, the require("a.js") in C will silently return back an empty module object that hasn't been initialized. At the very least, I'd expect a hard error.

3) This one's the most subtle: object/class identity is broken because there are multiple "class instances". If, above, C defined some sort of class (i.e. the constructor), A and B will get different instances of that constructor. That means an object created in B will not instanceof true to the class visible in A.

Also, the .js file the module lives in is re-evaluated on each require, which is possibly an expensive operation.

My expected behavior would be:

1) First require initializes the .js file and returns the module object.

2) Subsequent requires return the already-initialized module object, state intact, without re-evaluating the .js file.

That would fix 1) and 3), along with making 2) predictable. You'd still want to stay away from module-level initialization code that relies on circular reference--it's demonstrable that it's quite difficult to get everything initialized properly in time for each module to see it--but you'd be safe for circular references inside functions as long as they're not called before the module chain has been evaluated.

I've attached a quick testbed (I have a couple more if this isn't sufficient). Unzip notSingleton.zip and mozmill -t testImport.js. 

All it does is have a module count how many times it's been imported. It'll output 1-1-1. It would have been 1-2-3 if modules were singletons.

Comment 1

[Jeff Hammel]

Attachment: make required modules singletons

So this isn't the most elegant solution.  However, securableModule.loader is not a great API for the way that we use it.  That should be improved.  This might also not work for the e10s world (of course, neither does mozmill at this point, so that doesn't really matter).

Comment 2

[Jeff Hammel]

Atul, can you point us to someone that can meaningfully evaluated our choices with respect to securableModule.loader()? I feel an API change is an order, like being able to specify rootpaths per require, though what is needed depends on the future of securableModule and its maintainers (though certainly more documentation would be nice).

For reference, our front-end of require (without much context):
https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/frame.js#L117

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 569206 [details] [diff] [review]
make required modules singletons

Until we haven't gotten a reply from Atul I wouldn't like to give a review on that patch. Also forwarding a feedback request to Geo because he is more involved into that issue and already did a lot of investigation and has a couple of test cases ready for testing the patch.

Comment 4

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 569206 [details] [diff] [review]
make required modules singletons

I won't have time for a bit to apply patches, build custom Mozmills, etc, and don't have enough visibility into the code to understand from a desk read. 

I can get to this next week if you want me to exercise it, though I was comfortable with Jeff's exercises against the testbeds I already gave him.

Comment 5

[Jeff Hammel]

So do we want to land this? On what branches?

Comment 6

[cmtalbert]

I think we should land it on all three branches for consistency: 1.5.x, 2.0.x and master.

I think that Geo was working against 1.5.x when he hit the bug, and that's the only reason I'm advocating taking this on that branch.  If that wasn't the case, then I wouldn't take this on 1.5.  So unless Geo estimates he'll be switching to 2.0 for the rest of the work, I say we land it on all three and keep our finger on the "backout" button in case things go wonky.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I agree. For now we don't know exactly when we will upgrade to Mozmill 2. Backing out is an easy task, and once it has been landed we can deeply test it.

Comment 8

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 569206 [details] [diff] [review]
make required modules singletons

I'm fine landing this with the understanding that we need to roll it back if problems appear. I'm comfortable with the degree of testing to date.

Comment 9

[Jeff Hammel]

pushed to master: https://github.com/mozautomation/mozmill/commit/859f2501b19831525e1227a5f5397d945b90b1dd

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so before it's getting lost in the nirvana lets ask for approval for Mozmill 1.5.9 then. It's too hot for me to let it chime into Mozmill 1.5.8 which we will probably release today.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Jeff and/or Geo, how safe is this code? Has it been thoughtfully tested? What would have to be done to get this landed? We will release Mozmill 1.5.9 later today and I wonder what we can do here.

Comment 12

[Jeff Hammel]

AFAIK it is safe, though i'm not sure if i've tested in the 1.5 series in forever.

If we take it on 1.5 we should probably take it on 2. as well

Comment 13

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Good question. I haven't looked at it recently either. What sort of testing should we do to validate it?

FWIW, this doesn't hit us hard in the current codebase, so I'm OK delaying if need be.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With the patch in place we should run all of our different testruns and none should fail. Once this is done, and I think a single platform is enough, we should be ok in landing it.

Ok, so lets push it out to 1.5.10 so we can get 1.5.9 landed ASAP.

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Th patch does not apply cleanly to the hotfix-1.5 branch. I have locally tweaked the patch simply replacing mozelement with elementslib and will continue to test. If this is all that needs to be done to make the patch suitable for 1.5 then I should soon have some results.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, yes that's all. Nothing more should be necessary (fingers crossed)

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Patch for hotfix-1.5 v1.0 [checked-in]

Tested this patch against all testruns on Mac OSX 10.7.3 with no failures.

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Also, can we get this merged to hotfix-2.0? What's the process for that, do you want me to submit a pull request?

Comment 19

[Jeff Hammel]

Comment on attachment 612258 [details] [diff] [review]
Patch for hotfix-1.5 v1.0 [checked-in]

Yep, I think its that simple :)

Comment 20

[Jeff Hammel]

(In reply to Dave Hunt (:davehunt) from comment #18)
> Also, can we get this merged to hotfix-2.0? What's the process for that, do
> you want me to submit a pull request?

I'd just put up another patch for review.  If you have push access, this one is so similar between the two i'd just say to push it.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 612258 [details] [diff] [review]
Patch for hotfix-1.5 v1.0 [checked-in]

Landed on hotfix-1.5:
https://github.com/mozautomation/mozmill/commit/4471549dbef831478eff581395396297b79ae399

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Also I have cherry-picked the patch which has already been landed on master 5 months ago and landed it on hotfix-2.0:

https://github.com/mozautomation/mozmill/commit/d829758771cd0e15c1f77bec9f3ae1d6c9f53917

Testing has been shown that we are ready to continue the API refactoring project now.