Last Comment Bug 763483 - navigate() does not wait for DOM readystate == complete
: navigate() does not wait for DOM readystate == complete
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: Marionette (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla24
Assigned To: mathieu bultel:mbu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-11 07:09 PDT by David Burns :automatedtester
Modified: 2013-07-18 18:32 PDT (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
wontfix
fixed
fixed
wontfix
wontfix
fixed


Attachments
first version of goUrl refactoring (5.33 KB, patch)
2013-05-25 15:42 PDT, mathieu bultel:mbu
malini: review-
Details | Diff | Review
Add timer and fix mistake according to Malini review (5.33 KB, patch)
2013-05-30 12:20 PDT, mathieu bultel:mbu
malini: review-
dburns: feedback-
Details | Diff | Review
uploading patch according Malini and David feedback and review (5.36 KB, patch)
2013-06-04 14:31 PDT, mathieu bultel:mbu
malini: review-
Details | Diff | Review
add fake protocole (5.65 KB, patch)
2013-06-05 00:34 PDT, mathieu bultel:mbu
malini: review+
Details | Diff | Review

Description David Burns :automatedtester 2012-06-11 07:09:10 PDT
With the following little bit of code we get element not found. If we wait for document.readyState === 'complete' then it would find the element

driver = marionette.Marionette('localhost', 3000)
driver.start_session()
 
driver.navigate("https://marketplace.mozilla.org/en-US/app/lordofultima/")
driver.find_element("link text", "Log in / Register")
Comment 1 Malini Das [:mdas] - Away, not checking bugmail 2013-05-23 08:07:21 PDT
To do this, we'll need to update the content's way of navigating: https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#1225 which will check if the readyState is complete.

We currently only do DOMContentLoaded which is needed to detect when the new page is reached and its DOM is loaded. After DOMContentLoaded is called, we should check the readyState of the document for the duration of the page timeout period, if one is set. The page timeout should be fired if the whole process of DOMContentLoaded/readyState check takes longer than the timeout period.

We have a similar check for the chrome side, http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-actors.js#1027 if you want some ideas.
Comment 2 mathieu bultel:mbu 2013-05-24 08:42:25 PDT
Thanks for the detail.

I have started this afternoon to debug, i think i'm on the good way :)
Comment 3 mathieu bultel:mbu 2013-05-25 03:53:25 PDT
Ok, I think i fixed it.
I add unit tests on : test_navigation.py .. any objection ? I need to use "test_shouldnt_error_if_nonexistent_url_used" unit test to validate a part of the function goUrl. But if you prefer, i can write those unit tests in a new file.

I have just one question about the error message: The second parameter is the "status" of error messages, where can i find the referential of error status codes ?
Comment 4 mathieu bultel:mbu 2013-05-25 15:42:20 PDT
Created attachment 754168 [details] [diff] [review]
first version of goUrl refactoring

Submit the first version of this fix.

I have refactored goUrl's function to add a "checkLoad" function, which check the state of the load page during the timeout or raise an exception.

Adding unit tests on test_navigation.py and update one of them.
Test the 3 states of function : complete, timeout exceeded and error on loading page.

Hope it will be good ! :)
Comment 5 Malini Das [:mdas] - Away, not checking bugmail 2013-05-27 13:28:22 PDT
Comment on attachment 754168 [details] [diff] [review]
first version of goUrl refactoring

Review of attachment 754168 [details] [diff] [review]:
-----------------------------------------------------------------

Great start, this is the restructuring I was looking for. There is just one problem with it, and that is the need for a timer to deal with the case where DOMContentLoaded isn't fired.

::: testing/marionette/marionette-listener.js
@@ +1234,5 @@
> +    let elapse = end - start;
> +    if (msg.json.pageTimeout == null || elapse <= msg.json.pageTimeout){
> +      if (curWindow.document.readyState == "complete"){
> +        sendOk(command_id);
> +        return;

you don't need a return statement here

@@ +1241,5 @@
> +        sendError("Error loading page", 13, null, command_id);
> +      }
> +      else{
> +        checkTimer.initWithCallback(checkLoad, 100, Ci.nsITimer.TYPE_ONE_SHOT);
> +        return;

nor here

@@ +1246,5 @@
> +      }
> +    }
> +    else{
> +      sendError("Error Timeout limit exceeded", 28, null, command_id);
> +      return;

or here! returns are implicit:)

@@ +1257,3 @@
>      if (!event.originalTarget.defaultView.frameElement ||
>        event.originalTarget.defaultView.frameElement == curWindow.frameElement) {
> +      checkTimer.initWithCallback(checkLoad, 100, Ci.nsITimer.TYPE_ONE_SHOT);

instead of calling checkLoad as a callback after 100ms, you're safe to just call it like a normal function here, like:
checkLoad();

@@ -1247,5 @@
> -    removeEventListener("DOMContentLoaded", onDOMContentLoaded, false);
> -  }
> -  if (msg.json.pageTimeout != null){
> -    checkTimer.initWithCallback(timerFunc, msg.json.pageTimeout, Ci.nsITimer.TYPE_ONE_SHOT);
> -  }

We still want to keep this timer.

This timer ensures that if a page we navigate to doesn't trigger DOMContentLoaded within the timeout period, we stop execution and return to the client.

What you can do is have two timers, one like this, and the other one that calls checkLoad. If checkLoad gets called successfully, then you know that DOMContentLoaded was actually fired, and you can cancel the other timer (https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsITimer#cancel()).
Comment 6 Malini Das [:mdas] - Away, not checking bugmail 2013-05-27 13:39:11 PDT
(In reply to Malini Das [:mdas] from comment #5)
> @@ +1246,5 @@
> > +      }
> > +    }
> > +    else{
> > +      sendError("Error Timeout limit exceeded", 28, null, command_id);
> > +      return;
> 
> or here! returns are implicit:)
> 

oh, my statement might be confusing. I mean that since this function is essentially just an if/else statement, and there's no code out of the if/else block, then once you complete the if or else block, there's nothing else to do, so the return is implicit. You might see some places where we explicitly called 'return;' and that's so we skip over any code that would execute after the current block.
Comment 7 mathieu bultel:mbu 2013-05-28 01:53:28 PDT
Ok, 

I will take that in count. I didn't understand the interest of the second timer function.
Sorry for the mistake with the "return" statement.. :) I thought i needed explicit return in js... shame on me!! :)

I'm not sure that i could give you the new version today or on Wednesday because i'll travel to munich tomorrow ... but, i will on Thursday!
Comment 8 Malini Das [:mdas] - Away, not checking bugmail 2013-05-28 10:49:30 PDT
(In reply to mathieu bultel from comment #7)
> Ok, 
> 
> I will take that in count. I didn't understand the interest of the second
> timer function.
> Sorry for the mistake with the "return" statement.. :) I thought i needed
> explicit return in js... shame on me!! :)
> 
> I'm not sure that i could give you the new version today or on Wednesday
> because i'll travel to munich tomorrow ... but, i will on Thursday!

No problem, take your time, and enjoy Munich for me!
/me is jealous
Comment 9 mathieu bultel:mbu 2013-05-30 12:20:06 PDT
Created attachment 756127 [details] [diff] [review]
Add timer and fix mistake according to Malini review
Comment 10 mathieu bultel:mbu 2013-05-30 12:22:29 PDT
hope german beers did not alter too much this patch !! :)
Comment 11 Malini Das [:mdas] - Away, not checking bugmail 2013-05-30 13:45:52 PDT
Comment on attachment 756127 [details] [diff] [review]
Add timer and fix mistake according to Malini review

Review of attachment 756127 [details] [diff] [review]:
-----------------------------------------------------------------

I have some suggestions:

::: testing/marionette/client/marionette/tests/unit/test_navigation.py
@@ +80,1 @@
>              pass

Nice!

you can remove 'pass' now that you've added some code to execute in the except block.

@@ +92,4 @@
>  
> +    def test_find_element_error(self):
> +        try:
> +            self.marionette.timeouts("page load", 100)

Can you set it to 0 to be sure it will never load?

@@ +98,5 @@
> +            self.assertTrue(self.marionette.find_element("id", "mozLink"))
> +            self.fail("Should have thrown a MarionetteException")
> +        except MarionetteException as e:
> +            self.assertEqual(str(e), "Error Timeout limit exceeded")
> +            pass

No pass needed here either

@@ +108,5 @@
> +    def test_navigate_iframe(self):
> +        test_iframe = self.marionette.absolute_url("test_iframe.html")
> +        self.marionette.navigate(test_iframe)
> +        self.assertTrue('test_iframe.html' in self.marionette.get_url())
> +        self.assertTrue(self.marionette.find_element("id", "test_iframe"))

what does test_navigate_iframe do that other tests don't do?

::: testing/marionette/marionette-listener.js
@@ +1240,5 @@
> +      else if (curWindow.document.readyState == "interactive" && errorRegex.exec(curWindow.document.baseURI)){
> +        sendError("Error loading page", 13, null, command_id);
> +      }
> +      else{
> +        checkTimer.initWithCallback(checkLoad, 100, Ci.nsITimer.TYPE_ONE_SHOT);

I'm not sure we can change the callback of the timer once we already called initWithCallback. This is something I'll look into tomorrow.
Comment 12 mathieu bultel:mbu 2013-05-30 14:28:36 PDT
Yes, i have forgot to remove the pass statement on test functions.

For the timeout, I set 100ms because if it's zero, this is the first timeout that is call and raise the other exception.

I could add an other with 0ms of timeout to raise the exception of the first timer.

"test_navigate_iframe" -> i don't remember why i have added this test. Maybe to validate the normal execution.
Comment 13 David Burns :automatedtester 2013-05-31 03:00:58 PDT
Comment on attachment 756127 [details] [diff] [review]
Add timer and fix mistake according to Malini review


> 
>+    def test_find_element_error(self):
>+        try:
>+            self.marionette.timeouts("page load", 100)
>+            test_html = self.marionette.absolute_url("test.html")
>+            self.marionette.navigate(test_html)
>+            self.assertTrue(self.marionette.find_element("id", "mozLink"))
>+            self.fail("Should have thrown a MarionetteException")
>+        except MarionetteException as e:

Can we be more specific with the exception. since all exceptions have MarionetteException as a super then it could thrown a JavaScriptException and be caught. Going by the name of the test, which I would prefer to be more verbose, it should throw a NoSuchElementException
Comment 14 Malini Das [:mdas] - Away, not checking bugmail 2013-06-04 13:06:30 PDT
Comment on attachment 756127 [details] [diff] [review]
Add timer and fix mistake according to Malini review

Review of attachment 756127 [details] [diff] [review]:
-----------------------------------------------------------------

I have a few changes I recommend, and please rerun the tests after making changes. test_navigation.py failed since the error string didn't match.

::: testing/marionette/client/marionette/tests/unit/test_navigation.py
@@ +97,5 @@
> +            self.marionette.navigate(test_html)
> +            self.assertTrue(self.marionette.find_element("id", "mozLink"))
> +            self.fail("Should have thrown a MarionetteException")
> +        except MarionetteException as e:
> +            self.assertEqual(str(e), "Error Timeout limit exceeded")

This string should match "Error loading page, timed out"

::: testing/marionette/marionette-listener.js
@@ +1240,5 @@
> +      else if (curWindow.document.readyState == "interactive" && errorRegex.exec(curWindow.document.baseURI)){
> +        sendError("Error loading page", 13, null, command_id);
> +      }
> +      else{
> +        checkTimer.initWithCallback(checkLoad, 100, Ci.nsITimer.TYPE_ONE_SHOT);

To be sure that we clear the previous timer's state, how about we do this:

checkTimer.cancel();
checkTimer.initWithCallback(checkLoad, 100, Ci.nsITimer.TYPE_ONE_SHOT);


then we'll safely be reusing the timer.

@@ +1261,2 @@
>    function timerFunc(){
> +    sendError("Error loading page, timeout limit reach", 13, null, command_id);

the message should be "Error loading page, timed out"
Comment 15 mathieu bultel:mbu 2013-06-04 14:31:32 PDT
Created attachment 758204 [details] [diff] [review]
uploading patch according Malini and David feedback and review

I have fixed all your comment. I set a timeout exception as we discussed about it on irc with David.
Comment 16 Malini Das [:mdas] - Away, not checking bugmail 2013-06-04 16:48:31 PDT
Comment on attachment 758204 [details] [diff] [review]
uploading patch according Malini and David feedback and review

Review of attachment 758204 [details] [diff] [review]:
-----------------------------------------------------------------

cool, just some small nits:

::: testing/marionette/client/marionette/tests/unit/test_navigation.py
@@ +70,5 @@
>          self.assertTrue('test_iframe.html' in self.marionette.get_url())
>  
>      def test_shouldnt_error_if_nonexistent_url_used(self):
>          try:
>              self.marionette.navigate("http://localhost:12345")

ah I just noticed this. In the off-chance that 12345 is being used, we should avoid trying to connect. We can do something like self.marionette.navigate("thisprotocoldoesnotexist://")

::: testing/marionette/marionette-listener.js
@@ +1089,3 @@
>      if (!event.originalTarget.defaultView.frameElement ||
>        event.originalTarget.defaultView.frameElement == curWindow.frameElement) {
> +      checkLoad()

please add a ; at the end
Comment 17 mathieu bultel:mbu 2013-06-05 00:34:46 PDT
Created attachment 758433 [details] [diff] [review]
add fake protocole
Comment 18 Malini Das [:mdas] - Away, not checking bugmail 2013-06-05 10:20:07 PDT
Comment on attachment 758433 [details] [diff] [review]
add fake protocole

Review of attachment 758433 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, thanks!
Comment 19 Malini Das [:mdas] - Away, not checking bugmail 2013-06-05 10:26:20 PDT
landed in m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/033bd3fa9527
Comment 20 Ryan VanderMeulen [:RyanVM] 2013-06-05 13:35:17 PDT
https://hg.mozilla.org/mozilla-central/rev/033bd3fa9527
Comment 21 Jonathan Griffin (:jgriffin) 2013-07-18 16:49:11 PDT
http://hg.mozilla.org/releases/mozilla-b2g18/rev/b1b2a79a80c6

Note You need to log in before you can comment on or make changes to this bug.