Closed Bug 756577 Opened 9 years ago Closed 9 years ago

Navigating to a non-existent URL causes a Socket Timeout

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla18

People

(Reporter: automatedtester, Assigned: jgriffin)

References

Details

Attachments

(1 file, 5 obsolete files)

When we navigate to a URL that doesnt exist we get a socket timeout from the browser because marionette blocks on this call
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
Attached patch Failing test case (obsolete) — Splinter Review
Attached patch Cleaned up the failing test case (obsolete) — Splinter Review
Attachment #625229 - Attachment is obsolete: true
Attachment #653936 - Flags: review?(jgriffin)
Attachment #625374 - Attachment is obsolete: true
Blocks: 757171
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?
Attachment #653936 - Flags: review?(jgriffin) → review-
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?
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.
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
Attached patch Detect pageload errors (obsolete) — Splinter Review
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.
Attachment #653936 - Attachment is obsolete: true
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 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.
(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.
Tested on B2G and works fine there as well as desktop Firefox
Attachment #656189 - Flags: review?(mdas)
Attachment #655243 - Attachment is obsolete: true
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!
Attachment #656189 - Flags: review?(mdas) → review+
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.
(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?
(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.
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.
Attachment #657488 - Flags: review?(mdas)
Attachment #656189 - Attachment is obsolete: true
(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 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
Attachment #657488 - Flags: review?(mdas) → review+
https://hg.mozilla.org/mozilla-central/rev/056a16f19be2
Assignee: nobody → jgriffin
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.