While I ran some tests yesterday I noticed that the waitForPageLoad call times out when the appropriate page cannot be opened or requires a not signed certificate. See the following little Mozmill test: var setupModule = function(module) { module.controller = mozmill.getBrowserController(); } var testErrorPage = function () { // Open an invalid url and check waitForPageLoad controller.open('http://www.mozilla.or'); controller.waitForPageLoad(controller.tabs.activeTab); controller.window.alert("Page loaded"); } I'm not sure if this is a bug in Mozmill or if the right events aren't fired when we end up with an error page.

This occurs on safebrowsing webpages as well. Case in point: http://www.mozilla.com/firefox/its-an-attack.html

Boris, do you have an idea how that could be resolved? Is it correct that error pages don't set docuementLoaded to true? Does it also apply for the Safebrowsing pages?

Error page loads are really weird and somewhat buggy (imo) in terms of the events they fire. We have bugs on those. Safebrowsing pages are just error pages, last I checked. I have no idea what the documentLoaded thing you're talking about is, or how it gets set, so the above is about all I can tell you.

Thanks Boris. Just to give more clearance here: http://code.google.com/p/mozmill/source/browse/trunk/mozmill/extension/resource/modules/controller.js#814 We are using the value from document.defaultView.documentLoaded to check if the given page has been finished loading. We used the same way as Dietrich has implemented in one of his own patch for Places. So it would be nice to know if there is another way to check if an error page has been loaded successfully.

I understand what value you're using. I just have no idea who sets it and when. It's not a standard property on Window. > So it would be nice to know if there is another way to check if an error page > has been loaded successfully. "probably not; do see the bugs I said we have"

waitForPageLoad is intended to wait for web pages to load. Error pages are not web pages and don't trigger the same DOM events that we use to handle this. You should instead wait for one of the elements on the error page to exist rather than relying on waitForPageLoad.

Do we know that an error page has been loaded? If yes we could have an early return from the waitForPageLoad function? It's hard to find existing and open bugs.

With bug 579943 fixed this bug is always visible now. I have a patch in place I will attach in the next minute. IMO we should block Mozmill 1.4.2 on it.

Patch with tests included. Now waitForPageLoad works for all kind of pages.

Well, we also have to wait for the DOM. I accidentally removed that part in my last patch.

Comment on attachment 461868 [details] [diff] [review] Patch v1.1 >+++ b/mozmill/mozmill/extension/resource/modules/controller.js >+ try { >+ this.waitFor(function() { return self.loaded; }, timeout, interval); > >- //Once the object is available it's somewhere between 1 and 3 seconds before the DOM >- //Actually becomes available to us >- sleep(1); >- frame.events.pass({'function':'Controller.waitForPageLoad()'}); >+ //Once the object is available it's somewhere between 1 and 3 seconds before the DOM >+ //Actually becomes available to us >+ sleep(1000); >+ frame.events.pass({'function':'Controller.waitForPageLoad()'}); Still not that happy with the 1000ms sleep. Would be great to know when the DOM is available for us. Is there an event we could listen for?

Ok, so DOMContentLoaded should really be fired when the DOM is available for us. That's the difference to the former way we have used to detect the finish of a page load. This version doesn't include the mentioned 1000ms sleep call and all tests pass, except the ones for safe browsing. Those tests are broken either way and would need an upgrade. So I think this version of the patch should perfectly fit our needs for Mozmill 1.4.2.

Comment on attachment 461901 [details] [diff] [review] Patch v1.2 >diff --git a/mozmill/mozmill/extension/resource/modules/controller.js b/mozmill/mozmill/extension/resource/modules/controller.js >index b546393..f0f054c 100644 >--- a/mozmill/mozmill/extension/resource/modules/controller.js >+++ b/mozmill/mozmill/extension/resource/modules/controller.js >@@ -956,37 +956,46 @@ Tabs.prototype.selectTabIndex = function(i) { > this.controller.window.gBrowser.selectTabAtIndex(i); > } > >-function browserAdditions( controller ) { >+function browserAdditions (controller) { > controller.tabs = new Tabs(controller); >- controller.waitForPageLoad = function(_document, timeout, interval) { >- // if a user tries to do waitForPageLoad(2000), this will assign the >+ >+ controller.waitForPageLoad = function(document, timeout, interval) { >+ var tab = null; Why are you making this change? If you don't like _document, change it to aDocument and timeout ot aTimeout and interval to aInterval like we do elsewhere in the code, but do not include symantic changes in a patch along with functional changes. This make it incredibly difficult to follow. Furthermore, please don't use "document" then it becomes very difficult to tell if you're actually using the document global object or the passed-in argument of document and will likely lead to the next person who edits this code messing it up. > >- // Cache the default view. Otherwise we will fail for waitForPageLoad on >- // blank or error pages. >- var win = _document.defaultView; >+ var self = {loaded: false}; > >- waitFor(function() { >- return win.documentLoaded == true; >- }, timeout, interval); >+ // Add event listener for DOMContentLoaded which will fire once the >+ // content and all images have been loaded. >+ tab.addEventListener("DOMContentLoaded", function() { >+ tab.removeEventListener("DOMContentLoaded", arguments.callee, true); >+ self.loaded = true; >+ }, true); If we don't fire this event, we're going to leak the memory that the event listener takes up. > >- //Once the object is available it's somewhere between 1 and 3 seconds before the DOM >- //Actually becomes available to us >- sleep(1); >- frame.events.pass({'function':'Controller.waitForPageLoad()'}); >+ try { >+ this.waitFor(function() { return self.loaded; }, timeout, interval); >+ frame.events.pass({'function':'controller.waitForPageLoad()'}); >+ } catch (ex) { >+ throw new Error("controller.waitForPageLoad(): Timeout waiting for page loaded."); We should also remove the event listener here, I believe. That way if the event is not fired we don't leak anything. >diff --git a/mozmill/test/test_waitForPageLoad.js b/mozmill/test/test_waitForPageLoad.js >new file mode 100644 >index 0000000..e6eada3 >--- /dev/null >+++ b/mozmill/test/test_waitForPageLoad.js Nice test. Thanks. That makes me a lot more comfortable considering this for 1.4.2. I'm still considering this, but if we have high confidence in the fix, then it might be worth taking. I know that people have complained about waitForPageLoad for a long time and anything we can do to make that better is a good thing. It will still need changes I noted above though, so -'ing based on that.

Comment on attachment 461901 [details] [diff] [review] Patch v1.2 >-function browserAdditions( controller ) { >+function browserAdditions (controller) { > controller.tabs = new Tabs(controller); >- controller.waitForPageLoad = function(_document, timeout, interval) { >- // if a user tries to do waitForPageLoad(2000), this will assign the >+ >+ controller.waitForPageLoad = function(document, timeout, interval) { >+ var tab = null; >+ >+ // If a user tries to do waitForPageLoad(2000), this will assign the > // interval the first arg which is most likely what they were expecting >- if (typeof(_document) == "number"){ >- timeout = _document; >+ if (typeof(document) == "number"){ >+ timeout = document; > } Note sure why you changed _document -> document; is document in scope? > >- // in case they pass null >- if (_document == null){ >- _document = 0; >+ // Find the browser element for the given document >+ if (typeof(document) == "object") { >+ for each (var browser in this.window.gBrowser) { >+ if (browser && browser.contentDocument === document) { >+ tab = browser; >+ break; >+ } >+ } > } > >- // if _document isn't a document object >- if (typeof(_document) != "object") { >- _document = controller.tabs.activeTab; >- } >+ // Fallback to selected browser >+ tab = tab || this.window.gBrowser.selectedBrowser; > >- // Cache the default view. Otherwise we will fail for waitForPageLoad on >- // blank or error pages. >- var win = _document.defaultView; >+ var self = {loaded: false}; > >- waitFor(function() { >- return win.documentLoaded == true; >- }, timeout, interval); >+ // Add event listener for DOMContentLoaded which will fire once the >+ // content and all images have been loaded. >+ tab.addEventListener("DOMContentLoaded", function() { >+ tab.removeEventListener("DOMContentLoaded", arguments.callee, true); >+ self.loaded = true; >+ }, true); > >- //Once the object is available it's somewhere between 1 and 3 seconds before the DOM >- //Actually becomes available to us >- sleep(1); >- frame.events.pass({'function':'Controller.waitForPageLoad()'}); >+ try { >+ this.waitFor(function() { return self.loaded; }, timeout, interval); >+ frame.events.pass({'function':'controller.waitForPageLoad()'}); >+ } catch (ex) { >+ throw new Error("controller.waitForPageLoad(): Timeout waiting for page loaded."); >+ } > } > } > >diff --git a/mozmill/mozmill/extension/resource/modules/utils.js b/mozmill/mozmill/extension/resource/modules/utils.js >index 3e07949..62a6b3b 100644 >--- a/mozmill/mozmill/extension/resource/modules/utils.js >+++ b/mozmill/mozmill/extension/resource/modules/utils.js >@@ -396,8 +396,13 @@ function assert(callback) { > * Waits for the callback evaluates to true > */ > function waitFor(callback, timeout, interval) { >- timeout = timeout || 30000; >- interval = interval || 100; >+ if (typeof(timeout) != "number") { >+ timeout = 30000; >+ } >+ >+ if (typeof(interval) != "number") { >+ interval = 100; >+ } why these changes? interval or timeout should never be zero, and I can't think of another case that makes a difference here. > var self = {counter: 0, result: callback()}; > >diff --git a/mozmill/test/test_waitForPageLoad.js b/mozmill/test/test_waitForPageLoad.js glad to see you added a test!

>+ if (typeof(timeout) != "number") { >+ timeout = 30000; >+ } >+ >+ if (typeof(interval) != "number") { >+ interval = 100; >+ } I really disagree with adding this kind of thing--it papers over client-code bugs in a way that makes it hard to find problems later. A more defensive programming technique would be to throw an exception if either value isn't > 0 (i.e. a classic assertion). I don't think any of our code should ever attempt to "fix" values outside the valid input space.

(In reply to comment #13) > >- controller.waitForPageLoad = function(_document, timeout, interval) { > >- // if a user tries to do waitForPageLoad(2000), this will assign the > >+ > >+ controller.waitForPageLoad = function(document, timeout, interval) { > >+ var tab = null; > Why are you making this change? If you don't like _document, change it to > aDocument and timeout ot aTimeout and interval to aInterval like we do > elsewhere in the code, but do not include symantic changes in a patch along > with functional changes. This make it incredibly difficult to follow. Makes sense. Did that in the new version of the patch. > >+ tab.addEventListener("DOMContentLoaded", function() { > >+ tab.removeEventListener("DOMContentLoaded", arguments.callee, true); > >+ self.loaded = true; > >+ }, true); >[..] > We should also remove the event listener here, I believe. That way if the > event is not fired we don't leak anything. Good catch. I really forgot about that manual removal in a failure case. > >diff --git a/mozmill/test/test_waitForPageLoad.js b/mozmill> >+++ b/mozmill/test/test_waitForPageLoad.js > Nice test. Thanks. That makes me a lot more comfortable considering this for > 1.4.2. I'm still considering this, but if we have high confidence in the fix, > then it might be worth taking. I know that people have complained about > waitForPageLoad for a long time and anything we can do to make that better is a > good thing. Right. I will try to even add some more tests, especially for failing cases. (In reply to comment #14) > >- timeout = timeout || 30000; > >- interval = interval || 100; > >+ if (typeof(timeout) != "number") { > >+ timeout = 30000; > >+ } > >+ > >+ if (typeof(interval) != "number") { > >+ interval = 100; > >+ } > > why these changes? interval or timeout should never be zero, and I can't think > of another case that makes a difference here. That makes sense too. I have reverted that change. Thanks. (In reply to comment #15) > A more defensive programming technique would be to throw an exception if either > value isn't > 0 (i.e. a classic assertion). I don't think any of our code > should ever attempt to "fix" values outside the valid input space. If we wanna go such a way please file a bug. This change of behavior shouldn't be done in a minor release, because a lot of tests will break. Further to come back to the DOMContentLoaded vs. load events, I have to say that there is no load event fired for error pages. We only get a DOMContentLoaded event. At this stage the DOM the page has been loaded and the DOM is available. External data like stylesheets or images are still not ready. Given that the tests should use waitFor calls to wait for those element values or style data. That's something we always do in our Mozmill tests for Firefox. The only exception is for error pages. Tests which test elements in error pages are rare. I think we should update those tests instead of making them completely work in waitForPageLoad(). By using DOMContentLoaded we can at least make sure that we can detect, once an error page has been finished loading. See also comment 3 from Boris about the problems we have with error pages and load events.

All review comments have been addressed. I have also added two more tests. A test-run shows that none of our tests are broken because of that patch. Only the safe browsing pages which need an update in any way are affected.

Landed on master: http://github.com/mozautomation/mozmill/commit/f16360988f09d888733d461010b34e778e5916a5 Landed on 1.4.2: http://github.com/mozautomation/mozmill/commit/517bca06c9461c8b07a5b4e8ea0b696ed17a8ca0

Included tests and our Firefox tests all pass. So I call this verified fixed.