1142563 - Clean up global scope pollution from browser-loop.js

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Dão Gottwald [:dao]]

For no good reason, browser-loop.js (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-loop.js) defines these things in the browser window's global scope that it shares with many other scripts: injectLoopAPI, LoopRooms, MozLoopService, PanelFrame. Instead, they should be set on the LoopUI object.

e.g.:

XPCOMUtils.defineLazyModuleGetter(this, "injectLoopAPI", "resource:///modules/loop/MozLoopAPI.jsm");

needs to move to the end of browser-loop.js and be replaced with this:

XPCOMUtils.defineLazyModuleGetter(LoopUI, "injectLoopAPI", "resource:///modules/loop/MozLoopAPI.jsm");

and then, injectLoopAPI calls need to be replaced with calls to this.injectLoopAPI.

Comment 1

[zmiller12]

Hello Dão, I'm going to work on this.

Comment 2

[zmiller12]

Attachment: Bug1142563.diff Cleand up global scope pollution from browser-loop.js

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8577529 [details] [diff] [review]
Bug1142563.diff Cleand up global scope pollution from browser-loop.js

>+++ b/browser/base/content/browser-loop.js
>@@ -1,19 +1,20 @@
> // This Source Code Form is subject to the terms of the Mozilla Public
> // License, v. 2.0. If a copy of the MPL was not distributed with this
> // file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> // the "exported" symbols
> let LoopUI;
> 
>-XPCOMUtils.defineLazyModuleGetter(this, "injectLoopAPI", "resource:///modules/loop/MozLoopAPI.jsm");
>+
> XPCOMUtils.defineLazyModuleGetter(this, "LoopRooms", "resource:///modules/loop/LoopRooms.jsm");
> XPCOMUtils.defineLazyModuleGetter(this, "MozLoopService", "resource:///modules/loop/MozLoopService.jsm");
> XPCOMUtils.defineLazyModuleGetter(this, "PanelFrame", "resource:///modules/PanelFrame.jsm");
>+XPCOMUtils.defineLazyModuleGetter(LoopUI, "injectLoopAPI", "resource:///modules/loop/MozLoopAPI.jsm");

good start, but there's more left to do:

- LoopRooms, MozLoopService and PanelFrame need to be treated the same way you treated injectLoopAPI
- All the XPCOMUtils.defineLazyModuleGetter calls need to be moved to the very end of the file

Comment 4

[zmiller12]

Attachment: Bug1142563.diff Cleand up global scope pollution from browser-loop.js

Comment 5

[Dão Gottwald [:dao]]

I couldn't apply the patch... Is your source tree outdated?

patching file browser/base/content/browser-loop.js
Hunk #1 FAILED at 0
Hunk #6 FAILED at 185
Hunk #8 FAILED at 265

Comment 6

[zmiller12]

Attachment: Bug1142563.diff : Cleaned up global scope pollution from browser-loop.js

Comment 7

[zmiller12]

This should work.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8577699 [details] [diff] [review]
Bug1142563.diff : Cleaned up global scope pollution from browser-loop.js

>           iframe.addEventListener("DOMContentLoaded", function documentDOMLoaded() {
>             iframe.removeEventListener("DOMContentLoaded", documentDOMLoaded, true);
>-            injectLoopAPI(iframe.contentWindow);
>+            this.injectLoopAPI(iframe.contentWindow);
>             iframe.contentWindow.addEventListener("loopPanelInitialized", function loopPanelInitialized() {
>               iframe.contentWindow.removeEventListener("loopPanelInitialized",
>                                                        loopPanelInitialized);
>               showTab();
>             });
>           }, true);

This won't work, because 'this' is different within the anonymous function. You could refactor it like this:

let documentDOMLoaded = () => {
  iframe.removeEventListener("DOMContentLoaded", documentDOMLoaded, true);
  this.injectLoopAPI(iframe.contentWindow);
  ...
};
iframe.addEventListener("DOMContentLoaded", documentDOMLoaded, true);

Comment 9

[zmiller12]

Thanks Dão, you have been really helpful. Ill give it a shot.

Comment 10

[zmiller12]

Attachment: Bug 1142563 Clean up global scope pollution from browser-loop.js

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8581237 [details] [diff] [review]
Bug 1142563 Clean up global scope pollution from browser-loop.js

Looks good at a first glance! I've pushed this to the try server; I suspect some tests depend on the global variables and will need to be updated.

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

Comment 12

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #11)
> Looks good at a first glance! I've pushed this to the try server; I suspect
> some tests depend on the global variables and will need to be updated.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9301cc1a83c

So there are errors in browser/components/loop/test/mochitest/head.js (line 122) and browser_toolbarbutton.js (lines 26, 78, 86, and 165). They all use MozLoopService. browser/components/loop/test/mochitest/head.js already imports MozLoopService.jsm and could be updated easily to define MozLoopService at the top of the file where LOOP_SESSION_TYPE and MozLoopServiceInternal are defined. That should fix these errors.

Comment 13

[Dão Gottwald [:dao]]

Zach, was comment 12 clear enough or do you need more information / better instructions?

Comment 14

[zmiller12]

(In reply to Dão Gottwald [:dao] from comment #13)
> Zach, was comment 12 clear enough or do you need more information / better
> instructions?

 Dão, sorry I haven't been able to get back to you. Its the first week of spring term and I've been trying to calibrate myself. I'll take a look at it later today and get back to you ASAP.

Comment 15

[Dão Gottwald [:dao]]

Take your time, just wanted to make sure you didn't get stuck.

Comment 16

[zmiller12]

How does this look? 

(browser/components/loop/test/mochitest/head.js)

const HAWK_TOKEN_LENGTH = 64;
const {
  LOOP_SESSION_TYPE,
  MozLoopServiceInternal,
  MozLoopService, 
} 
const {LoopCalls} = Cu.import("resource:///modules/loop/LoopCalls.jsm", {});
const {LoopRooms} = Cu.import("resource:///modules/loop/LoopRooms.jsm", {});
.
.
.

Comment 17

[Dão Gottwald [:dao]]

(In reply to zmiller12 from comment #16)
> const {
>   LOOP_SESSION_TYPE,
>   MozLoopServiceInternal,
>   MozLoopService, 
> }

should be:

const {
  LOOP_SESSION_TYPE,
  MozLoopServiceInternal,
  MozLoopService,
} = Cu.import("resource:///modules/loop/MozLoopService.jsm", {});

Comment 18

[zmiller12]

I'm having a problem creating a diff from my patch file. hg qref dose not seem to be updating my patch file. Thus, hg export . > mypatch.diff just creates a empty diff file.

Comment 19

[Dão Gottwald [:dao]]

What does hg out tell you?

Comment 20

[zmiller12]

Attachment: head.js fix - updated head.js to define MozLoopService at the top of the file.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8594355 [details] [diff] [review]
head.js fix - updated head.js to define MozLoopService at the top of the file.

Looks good. Try server is closed at the moment, so I'll run the loop tests locally and let you know if there are more failures.

Comment 22

[zmiller12]

 (In reply to Dão Gottwald [:dao] from comment #21)
> Comment on attachment 8594355 [details] [diff] [review]
> head.js fix - updated head.js to define MozLoopService at the top of the
> file.
> 
> Looks good. Try server is closed at the moment, so I'll run the loop tests
> locally and let you know if there are more failures.

Sounds good.

Comment 23

[Dão Gottwald [:dao]]

One remaining failure:

TEST-UNEXPECTED-FAIL | browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js | leaked window property: injectLoopAPI

Looks like this line in browser_mozLoop_sharingListeners.js:

> const {injectLoopAPI} = Cu.import("resource:///modules/loop/MozLoopAPI.jsm");

needs to be replaced with:

> const {injectLoopAPI} = Cu.import("resource:///modules/loop/MozLoopAPI.jsm", {});

such that Cu.import doesn't declare injectLoopAPI in the global scope.

Comment 24

[zmiller12]

Attachment: Bug1142563-browser_mozloop_sharingListeners.js-fix..diff

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2a67d2f5929f

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2a67d2f5929f