756577 - Navigating to a non-existent URL causes a Socket Timeout

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[David Burns :automatedtester]

When we navigate to a URL that doesnt exist we get a socket timeout from the browser because marionette blocks on this call

Comment 1

[David Burns :automatedtester]

test_should_error_if_invalid_url_type (test_navigation.TestNavigate) ... Traceback (most recent call last):
  File "/home/davidburns/mozilla-central/testing/marionette/client/marionette/tests/unit/test_navigation.py", line 97, in test_should_error_if_invalid_url_type
    self.marionette.navigate('http://localhost:8432')
  File "/home/davidburns/mozilla-central/testing/marionette/client/marionette/marionette.py", line 273, in navigate
    response = self._send_message('goUrl', 'ok', value=url)
  File "/home/davidburns/mozilla-central/testing/marionette/client/marionette/marionette.py", line 171, in _send_message
    raise TimeoutException(message='socket.timeout', status=21, stacktrace=None)
TimeoutException: socket.timeout

Comment 2

[David Burns :automatedtester]

Attachment: Failing test case

Comment 3

[David Burns :automatedtester]

Attachment: Cleaned up the failing test case

Comment 4

[David Burns :automatedtester]

Attachment: force a timeout if a page hasnt loaded. This allows the socket to return to the user;

Comment 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 653936 [details] [diff] [review]
force a timeout if a page hasnt loaded. This allows the socket to return to the user;

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

::: testing/marionette/client/marionette/tests/unit/test_navigation.py
@@ +72,5 @@
> +        except ValueError:
> +            import traceback
> +            print traceback.format_exc()
> +            self.fail("Should have thrown a MarionetteException")
> +

This could be written more succinctly using self.assertRaises.

::: testing/marionette/marionette-listener.js
@@ +514,5 @@
> +      return;
> +    }
> +
> +    timeoutCount++;
> +

Hardcoding this kind of timeout is very dangerous, since we don't know what pages we will eventually want to load, or under what network conditions.  I'm thinking that something along the lines of what Mozmill does for a similar situation would be better:  https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/mozmill.js#L373.  That is, if the readyState != "uninitialized" and the baseURI starts with about:error or about:blocked, return an error.  What do you think?

Comment 6

[David Burns :automatedtester]

The reason why I didnt go with DOMContentLoaded is there is outstanding TODO to replace the current code with DOMContentLoaded from :mdas. The bug is question is bug 720714 which is still open.

I agree that its not pretty but for the mean time this can do. We have added an item to the webdriver spec to make this particular timeout configurable. I can add that in with this patch as a stop gap till bug 720714

Thoughts?

Comment 7

[Jonathan Griffin (:jgriffin)]

We don't need to use DOMContentLoaded for this.  To detect an error page I think we could just do something like:

let errorRegex = /about:.+(error)|(blocked)\?/;
let doc = curWindow.document;
if (errorRegex.exec(doc.baseURI) && (doc.readyState != "uninitialized")) {
   // return error...
}

If this doesn't work, then I'm ok with making this timeout configurable, rather than hardcoded.

Comment 8

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

AFAICR this wasn't working in Mozmill so we had to use DOMContentLoaded. For our implementation see:

https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/mozmill.js#L373

Comment 9

[Jonathan Griffin (:jgriffin)]

Attachment: Detect pageload errors

So, the general approach was good, but it exposed some other problem in Marionette, namely that we weren't waiting for new tabs to be loaded after creating them.  This meant that we could get a readyState of "complete" for the "about:blank" page loaded by the new tab, occasionally, when we were navigating to an invalid page, instead of triggering the error code.

This patch fixes all that, and passes all tests, including some others I cooked up just for experimenting.

I should note that this patch uses DOMContentLoaded, which seems to work fine now.

I haven't tested this yet on B2G, so will do that before I flag it for review.

Comment 10

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

Comment on attachment 655243 [details] [diff] [review]
Detect pageload errors

Looks interesting. I think i will try the approach with document.readyState for Mozmill too. We have a couple of tests specifically to different error pages. This could proof the robustness of your code.

Comment 11

[David Burns :automatedtester]

Comment on attachment 655243 [details] [diff] [review]
Detect pageload errors

>diff -r 45204dc49ac2 testing/marionette/client/marionette/tests/unit/test_navigation.py

> 
>+    def test_shouldnt_error_if_nonexistent_url_used(self):
>+        self.assertRaises(MarionetteException,
>+                          self.marionette.navigate,
>+                          "http://localhost:12345")
>+
>+

Because TimeoutException is a subclass of MarionetteException we need to make sure that TimeoutException isnt thrown. The socket timeout will cause a timeout exception so if we can do anything after the navigate we should be good. My current update to my initial patch has that.

Comment 12

[Jonathan Griffin (:jgriffin)]

(In reply to David Burns :automatedtester from comment #11)
> Comment on attachment 655243 [details] [diff] [review]
> Detect pageload errors
> 
> >diff -r 45204dc49ac2 testing/marionette/client/marionette/tests/unit/test_navigation.py
> 
> > 
> >+    def test_shouldnt_error_if_nonexistent_url_used(self):
> >+        self.assertRaises(MarionetteException,
> >+                          self.marionette.navigate,
> >+                          "http://localhost:12345")
> >+
> >+
> 
> Because TimeoutException is a subclass of MarionetteException we need to
> make sure that TimeoutException isnt thrown. The socket timeout will cause a
> timeout exception so if we can do anything after the navigate we should be
> good. My current update to my initial patch has that.

Good point!  I'm restoring your original test logic here.

Comment 13

[Jonathan Griffin (:jgriffin)]

Attachment: Return an error if unable to load a page during navigate(); DONTBUILD because NPOTB

Tested on B2G and works fine there as well as desktop Firefox

Comment 14

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 656189 [details] [diff] [review]
Return an error if unable to load a page during navigate();  DONTBUILD because NPOTB

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

If the code works when calling goUrl in a frame, then ship it!

Comment 15

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

Comment on attachment 656189 [details] [diff] [review]
Return an error if unable to load a page during navigate();  DONTBUILD because NPOTB

>+      let errorRegex = /about:.+(error)|(blocked)\?/;
>+      if (curWindow.document.readyState == "interactive" && errorRegex.exec(curWindow.document.baseURI)) {
>+        sendError("Error loading page", 13, null);
>+        return;

This doesn't work in Mozmill for blocked sites like: http://www.mozilla.org/firefox/its-a-trap.html

In those cases the readyState is complete and not interactive. But even if you wait for complete it doesn't mean that the page and its elements have been fully loaded and the DOM is ready. We have a couple of testcases in Mozmill to prove our way in doing that and the above code fails for us.

Comment 16

[Jonathan Griffin (:jgriffin)]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> Comment on attachment 656189 [details] [diff] [review]
> Return an error if unable to load a page during navigate();  DONTBUILD
> because NPOTB
> 
> >+      let errorRegex = /about:.+(error)|(blocked)\?/;
> >+      if (curWindow.document.readyState == "interactive" && errorRegex.exec(curWindow.document.baseURI)) {
> >+        sendError("Error loading page", 13, null);
> >+        return;
> 
> This doesn't work in Mozmill for blocked sites like:
> http://www.mozilla.org/firefox/its-a-trap.html
> 
> In those cases the readyState is complete and not interactive. But even if
> you wait for complete it doesn't mean that the page and its elements have
> been fully loaded and the DOM is ready. We have a couple of testcases in
> Mozmill to prove our way in doing that and the above code fails for us.

So what's your solution? It looks like Mozmill adds a 1s wait after DOMContentLoaded?

Comment 17

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

(In reply to Jonathan Griffin (:jgriffin) from comment #16)
> So what's your solution? It looks like Mozmill adds a 1s wait after
> DOMContentLoaded?

It's a nasty one but that's the only way it worked. Not sure if something has been changed meanwhile. It could even be a bug in Firefox, but we never had time to deeply investigate that.

Comment 18

[Jonathan Griffin (:jgriffin)]

Attachment: Return an error if unable to load a page during navigate(); DONTBUILD because NPOTB

Updated patch which works with frame navigation, including new test.  I have never seen any errors with this method, so I'm hoping that the previous problem regarding DOMContentLoaded is fixed.

Comment 19

[Jonathan Griffin (:jgriffin)]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> (In reply to Jonathan Griffin (:jgriffin) from comment #16)
> > So what's your solution? It looks like Mozmill adds a 1s wait after
> > DOMContentLoaded?
> 
> It's a nasty one but that's the only way it worked. Not sure if something
> has been changed meanwhile. It could even be a bug in Firefox, but we never
> had time to deeply investigate that.

For Marionette, I don't think we need this. If you've set a search timeout in Marionette, and try to access an element, it will wait up unto that interval for the element to appear before returning an error.  So, most operations that involve elements have an implicit wait built in.

Comment 20

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 657488 [details] [diff] [review]
Return an error if unable to load a page during navigate();  DONTBUILD because NPOTB

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

lgtm

Comment 21

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/mozilla-central/rev/056a16f19be2