Kevin Grandon :kgrandon Assignee
	Description • 10 years ago

Spinning up a new bug as I'm not sure if this is the desired fix or not for bug 1013999.

The global leak checks are not really needed now that we have JSHint. Since they are causing problems, perhaps we should remove them?

	Kevin Grandon :kgrandon Assignee
	Comment 1 • 10 years ago

What do you guys think? Another quick fix could be to add it to globalIgnores.

I guess my thoughts are that I just don't think we really need this now that we have JSHint. It seems to do a really good job at detecting leakage. 

Also having to specify globals twice in tests can suck (once for mocha and once for JSHint).

	Julien Wajsberg [:julienw]
	Comment 2 • 10 years ago

jshint doesn't find everything (eg: window.XXX = 'YYY', or using a jshintignored or xfailed file).

I'd tend to agree with your proposal but I know we have some developers doing bad code and I'm afraid :(

	Andrew Sutherland [:asuth] (he/him)
	Comment 3 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #2)
> jshint doesn't find everything (eg: window.XXX = 'YYY', or using a
> jshintignored or xfailed file).
> 
> I'd tend to agree with your proposal but I know we have some developers
> doing bad code and I'm afraid :(

What situation are you envisioning?  In general bad code is a per-app type of problem and our apps operate in independent global namespaces from each other.  Intentional global contamination via window.XXX is more of a library problem where the library may run in a situation where its user may just do everything in the global scope itself.

I think our main concern would be about accidental memory leaks due to leaving a "var" off and retaining a reference on the global object.  But not only will jshint find that, "use strict" (which jshint yells about) makes that impossible.  If someone is doing window.XXX they likely mean it and we are now gaining memory metrics elsewhere on our automated tests.  (And with Tarako, leaks become somewhat more obvious :)

	James Lal [:lightsofapollo]
	Comment 4 • 10 years ago

Comment on attachment 8426478 [details]
Github pull request

I have no objections but julienw has been the lead on TA changes lately so I would wait for him to respond before landing unless this is an emergency.

	Kevin Grandon :kgrandon Assignee
	Comment 5 • 10 years ago

I kind of agree that assigning to window.XXX is a fairly deliberate action. I also don't think that this is going to help to prevent bad code, as the if this is a problem - they probably won't be writing unit tests in the first place :)

If we don't want to land this, I suppose I would be ok with adding all of the globals in bug 1013999 to our pre-defined globals. I just thought that this might be more future-proof and make our test files a bit cleaner.

I cry everytime I look at bootstrap_test.js, https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/bootstrap_test.js#L47

	Julien Wajsberg [:julienw]
	Comment 6 • 10 years ago

Comment on attachment 8426478 [details]
Github pull request

ok, let's do this, r=me

Andrew convinced me here: global leaks is really a library issue and here we're mainly working with applications.

	Kevin Grandon :kgrandon Assignee
	Comment 7 • 10 years ago

Thanks guys. If we run into this in the future, or have a few troublesome apps, perhaps we can look at some alternatives, set this on a per-app/test basis.

	Kevin Grandon :kgrandon Assignee
	Comment 9 • 10 years ago

Landed in js-test-agent: https://github.com/mozilla-b2g/js-test-agent/commit/59e03c9da4097e2151ed79e90d014ef5c221b271

Follow-up to update test-agent version: https://github.com/mozilla-b2g/js-test-agent/commit/591c94a27a7c6fdaa4c007cef4c141fac0955e73

Update to node_modules: https://github.com/mozilla-b2g/gaia-node-modules/commit/dbccfa4909974b3f4a48bdcd54f60e95991f78d9

And finally a pull request open to gaia: https://github.com/mozilla-b2g/gaia/pull/19525


Everything will probably be fine, but I'm concerned about the marionette-apps update to ~0.3.5. I need to follow-up there to understand why it's bumped on node_modules but not master yet. (The two package.json files should stay in sync)

	Ricky Chien [:rickychien] (inactive)
	Comment 11 • 10 years ago

Hi Kevin,

Could you please sync the test-agent.js from js-test-agent to gaia by using "make update-common" after you updated node_modules. Thanks!

	Kevin Grandon :kgrandon Assignee
	Comment 12 • 10 years ago

Thanks for the reminder! :)

	Kevin Grandon :kgrandon Assignee
	Comment 14 • 10 years ago

Landed: https://github.com/mozilla-b2g/gaia/commit/56162d96620aee365a463d56dd346984344f1d0c

	Peter Van der Beken [:peterv]
	Comment 15 • 10 years ago

This isn't totally fixed, while trying to land bug 789261 we're still running into this, see https://tbpl.mozilla.org/php/getParsedLog.php?id=40371072&tree=Mozilla-Inbound

gaia-unit-tests TEST-UNEXPECTED-FAIL | camera/test/unit/controllers/preview-gallery_test.js | controllers/preview-gallery PreviewGalleryController() Should add and remove a resize handler when opening and closing view | global leaks detected: addEventListener, removeEventListener
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/app_window_factory_test.js | system/AppWindowFactory handle event classic app launch | global leak detected: dispatchEvent
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/screenshot_test.js | system/Screenshot Receive home+sleep event with available device storage. | global leak detected: dispatchEvent
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/radio_test.js | Radio >  set enabled to true conn0 is enabling, conn1 is enabled "before each" hook | global leak detected: dispatchEvent
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/notifications_test.js | system/NotificationScreen > resending notifications > on restart > "before each" hook | global leak detected: dispatchEvent
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/fxa_client_test.js | system/FxAccountsClient > "before each" hook | global leaks detected: addEventListener, removeEventListener, dispatchEvent
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/hardware_buttons_test.js | system/HardwareButtons press and release home (screen enabled) | global leak detected: dispatchEvent
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/fxa_manager_test.js | system/FxAccountManager > "before each" hook | global leak detected: addEventListener
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/fxa_manager_test.js | system/FxAccountManager > On openFlow mozFxAccountsRPChromeEvent "before each" hook | global leak detected: dispatchEvent
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/system_nfc_connect_dialog_test.js | NfcConnectSystemDialog Dialog lifecycle "before each" hook | global leak detected: dispatchEvent

	Kevin Grandon :kgrandon Assignee
	Comment 16 • 10 years ago

This change has yet to be merged over to m-i yet though, right?

	Peter Van der Beken [:peterv]
	Comment 17 • 10 years ago

Ah, yes, I thought it'd already be merged.

	Ricky Chien [:rickychien] (inactive)
	Comment 18 • 10 years ago

Wow! Did you guys notice this patch reduce a lot of time on Travis?

I looked into Travis and noticed that our unit test duration reduce from 35~40min to 20~25min after this patch!!

Thanks Kevin accidentally improve our test-agent. LOL

	Kevin Grandon :kgrandon Assignee
	Comment 19 • 10 years ago

Awesome! Let's bring that time back with coverage checks maybe? :)