navigate() does not wait for DOM readystate == complete

RESOLVED FIXED in Firefox 24, Firefox OS v1.1hd

Status

Testing
Marionette
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: automatedtester, Assigned: mbu)

Tracking

unspecified
mozilla24
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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")
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.
(Assignee)

Comment 2

4 years ago
Thanks for the detail.

I have started this afternoon to debug, i think i'm on the good way :)
(Assignee)

Comment 3

4 years ago
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 ?
(Assignee)

Comment 4

4 years ago
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 ! :)
Attachment #754168 - Flags: review?(mdas)
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()).
Attachment #754168 - Flags: review?(mdas) → review-
(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.
(Assignee)

Comment 7

4 years ago
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!
(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
(Assignee)

Comment 9

4 years ago
Created attachment 756127 [details] [diff] [review]
Add timer and fix mistake according to Malini review
Attachment #754168 - Attachment is obsolete: true
Attachment #756127 - Flags: review?(mdas)
(Assignee)

Comment 10

4 years ago
hope german beers did not alter too much this patch !! :)
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.
(Assignee)

Comment 12

4 years ago
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.
(Reporter)

Comment 13

4 years ago
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
Attachment #756127 - Flags: feedback-
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"
Attachment #756127 - Flags: review?(mdas) → review-
(Assignee)

Comment 15

4 years ago
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.
Attachment #756127 - Attachment is obsolete: true
Attachment #758204 - Flags: review?(mdas)
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
Attachment #758204 - Flags: review?(mdas) → review-
(Assignee)

Comment 17

4 years ago
Created attachment 758433 [details] [diff] [review]
add fake protocole
Attachment #758204 - Attachment is obsolete: true
Attachment #758433 - Flags: review?(mdas)
Comment on attachment 758433 [details] [diff] [review]
add fake protocole

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

looks good, thanks!
Attachment #758433 - Flags: review?(mdas) → review+
landed in m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/033bd3fa9527
https://hg.mozilla.org/mozilla-central/rev/033bd3fa9527
Assignee: nobody → mat.bultel
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
http://hg.mozilla.org/releases/mozilla-b2g18/rev/b1b2a79a80c6
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox24: --- → fixed
status-firefox25: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/b1b2a79a80c6
status-b2g-v1.1hd: --- → fixed
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox25: fixed → ---
You need to log in before you can comment on or make changes to this bug.