661408 - replace global 'documentLoaded' with namespaced 'mozmillDocumentLoaded'

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Heather Arthur [:harth]]

As per bug 652990, we need to namespace the global 'documentLoaded' that we attach to each document.

Comment 1

[Heather Arthur [:harth]]

Attachment: replace 'documentLoaded' occurances with 'mozmillDocumentLoaded'

This patch unfortunately seems to cause some tests to fail, and I can't figure out why because it's just replacing a variable with a new variable name. When running:

mozmill -t mozmill-tests/mozmill-tests/tests/functional/testTabbedBrowsing

I get 3 new test failures:
TEST-UNEXPECTED-FAIL | /Users/harth/repos/mozmill-tests/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js | testScrollBackgroundTabIntoView
TEST-UNEXPECTED-FAIL | /Users/harth/repos/mozmill-tests/tests/functional/testTabbedBrowsing/testOpenInBackground.js | testOpenInBackgroundTab
TEST-UNEXPECTED-FAIL | /Users/harth/repos/mozmill-tests/tests/functional/testTabbedBrowsing/testOpenInForeground.js | testOpenInForegroundTab

INFO Passed: 13
INFO Failed: 3
INFO Skipped: 0


I get this error when running mozmill-tests/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js:

ERROR | Test Failure: {"exception": {"stack": "waitFor([object Proxy],\"Window content has been loaded.\")@resource://mozmill/modules/utils.js:449\n@:0\n", "message": "Window content has been loaded.", "fileName": "resource://mozmill/modules/utils.js", "name": "Error", "lineNumber": 449}}

Comment 2

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

A change like this will break our Mozmill tests for the new release. Reason is the modal dialog and non-modal window handler which are using this window property.

Not sure if we can get-rid of those instances, but right now this upcoming change has massive impact to our test-runs. Especially that modal dialogs cannot be closed anymore and the whole test-run will be aborted.

We should figure out if we could implement a less-invasive way.

Comment 3

[Heather Arthur [:harth]]

(In reply to comment #2)
> A change like this will break our Mozmill tests for the new release. Reason
> is the modal dialog and non-modal window handler which are using this window
> property.
> 
> Not sure if we can get-rid of those instances, but right now this upcoming
> change has massive impact to our test-runs. Especially that modal dialogs
> cannot be closed anymore and the whole test-run will be aborted.
> 
> We should figure out if we could implement a less-invasive way.

Oh wow, it's used in the shared libs? There should be some method on the controller that wraps that, like a controller.isLoaded(), would that work?

Comment 4

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

I don't have time to check this anymore before my vacation, so one of you will have to check it. If it's doable we definitely need an interim beta we could use to update the tests.

Comment 5

[cmtalbert]

(In reply to comment #2)
> A change like this will break our Mozmill tests for the new release. Reason
> is the modal dialog and non-modal window handler which are using this window
> property.
> 
> Not sure if we can get-rid of those instances, but right now this upcoming
> change has massive impact to our test-runs. Especially that modal dialogs
> cannot be closed anymore and the whole test-run will be aborted.
> 
> We should figure out if we could implement a less-invasive way.

There is no less invasive way.  The shared libraries simply should not be depending on the private data of the implementation.  This is one of those cases where an API was needed to be created, and no one raised or realized it (myself included, as I've reviewed all the code in question).  Instead, code was written that depends on implementation specific, private behavior.  

Shared libraries should _absolutely_never_ depend on internal implementation-specific behaviors.  If they do need access to the data contained therein, an API should be created for that so we don't end up in this situation again.  We all need to think about this going forward as we finish 2.0 and as we update the shared libraries.

In this particular case, we'll make the missing API, ensure it works in the shared libraries and keep moving forward.  Please please please uplift bugs like this instead of working around them.  It will cause us all a lot less trouble in the future.

And yes, obviously, we'd need another beta once this change is in and a patch is made to the libraries.  We'll have to land both at once to ensure we maintain working compatibility.

Comment 6

[Heather Arthur [:harth]]

Attachment: export controller.isLoaded() and replace 'documentLoaded' with 'mozmillDocumentLoaded'

Alright, this exposes a isLoaded() on the controller that checks if the window is loaded, also namespaces all those documentLoadeds for the AMO review. I have a patch for mozmill-tests coming up.

Comment 7

[Heather Arthur [:harth]]

Attachment: replace documentLoaded with controller.isLoaded() in mozmill-tests

This gets mozmill-tests working with the mozmill patch.

Comment 8

[cmtalbert]

Landed the mozmill side:
master: https://github.com/mozautomation/mozmill/commit/d77446174992024764d92dd7eaed49dbb9d43648

hotfix-1.5: https://github.com/mozautomation/mozmill/commit/583342c357769e7644835cff88de42db10c5cd57

leaving bug open to track landing the mozmill-tests fixes.

Mozmill QA NOTE: This change will BREAK TESTS until the remaining patch is reviewed and checked in.  I can't remember when Henrik is coming back - does someone else from QA want to take this review and land it?

Comment 9

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

Frankly, I'm really concerned about this check-in here. In the current state it breaks all the tests which get executed by Mozmill Crowd or users who are running the latest Mozmill release. For situations like those you should really wait until we have updated our tests, so a smooth transition is possible.

Why there was a rush to get this in so quickly? Did I miss something?

Comment 10

[Jeff Hammel]

This was done to address AMO review comments: https://bugzilla.mozilla.org/show_bug.cgi?id=652990

Comment 11

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

Comment on attachment 537612 [details] [diff] [review]
export controller.isLoaded() and replace 'documentLoaded' with 'mozmillDocumentLoaded'

>diff --git a/mozmill/docs/_build/doctrees/environment.pickle b/mozmill/docs/_build/doctrees/environment.pickle
>index e86b375..06e69b1 100644
>Binary files a/mozmill/docs/_build/doctrees/environment.pickle and b/mozmill/docs/_build/doctrees/environment.pickle differ
>diff --git a/mozmill/docs/_build/doctrees/index.doctree b/mozmill/docs/_build/doctrees/index.doctree
>index 15c7914..e0487cb 100644
>Binary files a/mozmill/docs/_build/doctrees/index.doctree and b/mozmill/docs/_build/doctrees/index.doctree differ

What has changed for those entries? Why are those in this patch?

>+MozMillController.prototype.isLoaded = function() {
>+  return this.window.mozmillDocumentLoaded;
>+};

That's not enough for an API. It does not obey the new feature of handling documentLoaded events for iframes or embedded browsers in content-space. It definitely needs a window parameter, but can fallback to this.window if none has been specified.

Comment 12

[Heather Arthur [:harth]]

(In reply to comment #11)
> Comment on attachment 537612 [details] [diff] [review] [review]
> export controller.isLoaded() and replace 'documentLoaded' with
> 'mozmillDocumentLoaded'
> 
> >diff --git a/mozmill/docs/_build/doctrees/environment.pickle b/mozmill/docs/_build/doctrees/environment.pickle
> >index e86b375..06e69b1 100644
> >Binary files a/mozmill/docs/_build/doctrees/environment.pickle and b/mozmill/docs/_build/doctrees/environment.pickle differ
> >diff --git a/mozmill/docs/_build/doctrees/index.doctree b/mozmill/docs/_build/doctrees/index.doctree
> >index 15c7914..e0487cb 100644
> >Binary files a/mozmill/docs/_build/doctrees/index.doctree and b/mozmill/docs/_build/doctrees/index.doctree differ
> 
> What has changed for those entries? Why are those in this patch?

They're not in the raw patch as far as I can tell, where are you seeing these?

> 
> >+MozMillController.prototype.isLoaded = function() {
> >+  return this.window.mozmillDocumentLoaded;
> >+};
> 
> That's not enough for an API. It does not obey the new feature of handling
> documentLoaded events for iframes or embedded browsers in content-space. It
> definitely needs a window parameter, but can fallback to this.window if none
> has been specified.

You can make a controller for the iframe.

Comment 13

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

Comment on attachment 537615 [details] [diff] [review]
replace documentLoaded with controller.isLoaded() in mozmill-tests

>+++ b/lib/modal-dialog.js	Mon Jun 06 13:09:56 2011 -0700
>@@ -116,9 +116,11 @@
>   observe : function mdObserver_observe(aSubject, aTopic, aData) {
>     // Once the window has been found and loaded we can execute the callback
>     var window = this.findWindow();
>-    if (window && ("documentLoaded" in window)) {
>+    var controller = new mozmill.controller.MozMillController(window);
>+
>+    if (window && controller.isLoaded()) {
>       try {
>-        this._callback(new mozmill.controller.MozMillController(window));
>+        this._callback(controller);

That will not work if there is a delay in opening the modal dialog. With your change we will throw an exception and do not start another timer. 

As talked in the meeting I will come up with an alternative approach, which will hopefully work in all cases.

Comment 14

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

Can we place get this backed out or have a beta4 without that patch included? This change touched a very fragile area of code and we really have to make sure it works perfectly. It needs more time for me to come up with a working patch and ensure everything works.

Comment 15

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

Comment on attachment 537612 [details] [diff] [review]
export controller.isLoaded() and replace 'documentLoaded' with 'mozmillDocumentLoaded'

After additional tests I still stay at my position that this patch is not ready to land as it is. We really have to back it out and fix it correctly. Here some more comments in my additional review:

>@@ -309,8 +309,8 @@ var MozMillController = function (window) {
> 
>   utils.waitFor(function() {
>     return window != null &&
>-           ("documentLoaded" in window) &&
>-           window.documentLoaded;
>+           ("mozmillDocumentLoaded" in window) &&
>+           window.mozmillDocumentLoaded;

In the constructor you will have to use the isLoaded() method, which you have added to this class.

>+MozMillController.prototype.isLoaded = function() {
>+  return this.window.mozmillDocumentLoaded;
>+};

That's broken because "mozmillDocumentLoaded" will not always be present. See the checks from above how to correctly implement that.

> MozMillController.prototype.waitFor = function(callback, message, timeout,
>                                                interval, thisObject) {
>   utils.waitFor(callback, message, timeout, interval, thisObject);
>@@ -1285,7 +1289,7 @@ function browserAdditions (controller) {
> 
>     // Wait until the content in the tab has been loaded
>     this.waitFor(function() {
>-      return ("documentLoaded" in owner && owner.documentLoaded);
>+      return ("mozmillDocumentLoaded" in owner && owner.mozmillDocumentLoaded);

Same as above. Why don't you use the isLoaded() method?

Comment 16

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

I see why you didn't do that. We have different owners here. In the constructor it's the ChromeWindow, while in the waitForPageLoad method we operate on the content window. Please see my comment 11 again. I'm sure we have to add a parameter to isLoaded().

Comment 17

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

Attachment: Proposed follow-up

With this follow-up all my concerns should be addressed. I have also updated the patch for the shared modules over on bug 658231 and everything seems to work for me with a Nightly build.

Geo can you please double-check with a test-run?

Instead of backing out the original patch we could land a follow-up and build beta4 with it included. That's probably the easiest solution.

Comment 18

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

Comment on attachment 541047 [details] [diff] [review]
Proposed follow-up

Haven't done the test run (per irc, send me a tarball of the patched mozmill) but code looks ok to me. f+

Comment 19

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

Prebuilt 1.5.4b3 with the patch included can be found here:
http://people.mozilla.com/~hskupin/downloads/mozmill-1.5.4b3.tar.gz

I have also landed our patches for the shared modules, so we simply need this final patch landed.

Comment 20

[Heather Arthur [:harth]]

(In reply to comment #17)
> Created attachment 541047 [details] [diff] [review] [review]
Couldn't apply the patch to a fresh checkout of mozauto/hotfix-1.5, could you push to a remote branch?

Comment 21

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

Not sure why you have problems. It works pretty well for me. But here my newly created remote branch:

https://github.com/whimboo/mozmill/tree/documentLoaded

Comment 22

[Heather Arthur [:harth]]

Comment on attachment 541047 [details] [diff] [review]
Proposed follow-up

Thanks for the upgrade. I'm not sure why the patch didn't apply, but I've hand patched it and it works with the new mozmill-tests. I'll check it in.

Comment 23

[Heather Arthur [:harth]]

hotfix-1.5:
https://github.com/mozautomation/mozmill/commit/f1c852b67fbe0b834455d479ff2ba14d9b885e49

master:
https://github.com/mozautomation/mozmill/commit/eb13aa91b8d7e5f157f10253408a05821bc50b04

Comment 24

[Jeff Hammel]

So the fix to master doesn't in fact work and is at least partially responsible for the mutt test failure in https://bugzilla.mozilla.org/show_bug.cgi?id=644249#c10

Note that in mozmill 1.5, there is an owner:

https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L1269

But not in mozmill 2.0:

https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/controller.js#L897

Does anyone know the reason for the change?  I'll hit blame and try to do some detective work. 

Notably, this breaks the mutt tests which pretty much stops development.  A week ago, all mutt tests passed. Now there are two failures, this being (at least) one of them.  If you're developing 2.0, please run the tests before checking in.  We'll have automation someday (which is a safety not, not a first line of defense), but running `mutt` before pushing is pretty important.

Comment 25

[Jeff Hammel]

Attachment: use null instead of owner

This patch just changes owner to null as owner is not defined.  It fixes the failing mutt test. I have no idea if it is sufficient, but having owner undefined certainly breaks things.

Comment 26

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

Comment on attachment 542637 [details] [diff] [review]
use null instead of owner

That's wrong. If we want to have an intermediate fix it should be isLoaded(tab)

Comment 27

[Jeff Hammel]

Attachment: use tab for some reason

Comment 28

[Heather Arthur [:harth]]

master:
https://github.com/mozautomation/mozmill/commit/b4c2a8a6eb743a0203a8cf66c50a97fce79f2341

Comment 29

[Heather Arthur [:harth]]

Comment on attachment 542664 [details] [diff] [review]
use tab for some reason

fixed with the prev checkin, thanks!

Comment 30

[Jeff Hammel]

Yeah, all mutt tests currently pass \o/  Let's keep it that way.

won't push