Closed
Bug 756577
Opened 12 years ago
Closed 12 years ago
Navigating to a non-existent URL causes a Socket Timeout
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
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
Reporter | ||
Comment 1•12 years ago
|
||
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
Reporter | ||
Comment 2•12 years ago
|
||
Reporter | ||
Comment 3•12 years ago
|
||
Attachment #625229 -
Attachment is obsolete: true
Reporter | ||
Comment 4•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Attachment #653936 -
Flags: review?(jgriffin)
Reporter | ||
Updated•12 years ago
|
Attachment #625374 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 years ago
|
||
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-
Reporter | ||
Comment 6•12 years ago
|
||
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?
Assignee | ||
Comment 7•12 years ago
|
||
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•12 years ago
|
||
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
Assignee | ||
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
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.
Reporter | ||
Comment 11•12 years ago
|
||
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.
Assignee | ||
Comment 12•12 years ago
|
||
(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.
Assignee | ||
Comment 13•12 years ago
|
||
Tested on B2G and works fine there as well as desktop Firefox
Attachment #656189 -
Flags: review?(mdas)
Assignee | ||
Updated•12 years ago
|
Attachment #655243 -
Attachment is obsolete: true
Comment 14•12 years ago
|
||
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 15•12 years ago
|
||
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.
Assignee | ||
Comment 16•12 years ago
|
||
(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•12 years ago
|
||
(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.
Assignee | ||
Comment 18•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #656189 -
Attachment is obsolete: true
Assignee | ||
Comment 19•12 years ago
|
||
(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•12 years ago
|
||
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+
Assignee | ||
Comment 21•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/056a16f19be2
Assignee: nobody → jgriffin
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•