1145668 - Marionette navigate() method should not manipulate location.href directly but using some variant of loadURI()

Status: RESOLVED WORKSFORME

(Remote Protocol :: Marionette, defect)

Description

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

Here the testcase to execute:

from marionette_driver.errors import MarionetteException
from marionette import MarionetteTestCase


class TestAboutPages(MarionetteTestCase):

    def test_failure_loading_about_pages(self):
        with self.marionette.using_context('content'):
            self.assertRaises(MarionetteException, self.marionette.navigate,
                              'about:error')

        with self.marionette.using_context('content'):
            self.marionette.navigate('about:blank')
            return self.marionette.get_url()

Traceback for the failure:

Traceback (most recent call last):
  File "/mozilla/code/firefox/nightly/testing/marionette/client/marionette/marionette_test.py", line 296, in run
    testMethod()
  File "/mozilla/code/firefox-ui-tests/firefox_ui_tests/remote/security/test_untrusted_connection_error_page.py", line 13, in test_failure_loading_about_pages
    self.marionette.navigate('about:blank')
  File "/mozilla/code/firefox/nightly/testing/marionette/driver/marionette_driver/marionette.py", line 1261, in navigate
    response = self._send_message("get", "ok", url=url)
  File "/mozilla/code/firefox/nightly/testing/marionette/driver/marionette_driver/decorators.py", line 36, in _
    return func(*args, **kwargs)
  File "/mozilla/code/firefox/nightly/testing/marionette/driver/marionette_driver/marionette.py", line 716, in _send_message
    self._handle_error(response)
  File "/mozilla/code/firefox/nightly/testing/marionette/driver/marionette_driver/marionette.py", line 767, in _handle_error
    raise errors.MarionetteException(message=message, status=status, stacktrace=stacktrace)
MarionetteException: MarionetteException: Error loading page

Comment 1

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

For our former Mozmill tests we are usually using about:blank to clean-up any loaded webpage from the remaining opened tab. That's what we should also do for the Firefox UI Tests. Given that this doesn't work we would have to load another artificial page in setUp for each test. We can have a soft-workaround for that but would be good that we can get this fixed, given that it can also fail in the middle of a test if we get unexpected error pages.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Yeah that looks bad.

Comment 3

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

It looks like that not all about: pages are affected. At least for about:blank and about:home this failure can be seen.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

The problem is we check content.document.baseURI in the listener to see if it looks like an error page and return an error based on that, but for some reason when marionette navigates to about:blank (or about:home), the baseURI doesn't change. I thought this was a recent regression, but it seems inherent to marionette's navigate or a browser bug and I'm not really sure there's an easy fix.

Comment 5

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

So I have taken a look here to see if it is a Marionette or Firefox issue. Given that we never had this problem with Mozmill, I was just curious. So as it looks like this is clearly a problem with navigate, or better Marionette server's get() [1] method:

   this.getCurrentWindow().location.href = aRequest.parameters.url;

I assume it happens because it directly sets the location.href property of the document. This is not a good practice. Instead gBrowser.loadURI() [2] should be used. A testcase I have put together works perfectly with that change. Beside that it may be even better to use openUILinkIn() [3] to ensure the page is loaded in the correct tab.
 
For our Firefox UI tests we have at least an ugly workaround for now, to bypass that problem. Also given that is affects the server component it doesn't block the next release of driver or client packages.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#1322
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/loadURI
[3] https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Tabbed_browser#Opening_a_URL_in_the_correct_window.2Ftab

Comment 6

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

Attachment: working testcase

Comment 7

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

This failure is still present. Andreas, what do you think about my proposed solution as shown in the attached testcase? If you agree I can get this implemented. I would really like to see this problem fixed given all the troubles we currently have with navigate() and get() as seen by intermittent failures on Treeherder.

Comment 8

[Andreas Tolfsen ❲:ato❳]

If calling loadURI fixes the issue then that seems like a good solution.

Generally, just be aware that the page loading algorithm has changed somewhat since this bug was filed: https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/listener.js#L903-#L1030

Comment 9

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

So I had another look at my testcase and how we currently trigger loading of URLs. So we defer loading of any URL for content scope from the driver.js to listener.js, which runs as a frame script. It means we won't be able to use gBrowser.loadURI() or openUILinkIn().

I'm even not sure how things have been changed with e10s and how content scripts should open URLs. Mike, mind telling us if `content.location = requestedURL;` is the right way to do?

If we keep this the attached testcase would throw an exception in listener.js@get():

> TEST-UNEXPECTED-ERROR | test_window_handles.py TestAboutPages.test_failure_loading_about_pages | UnknownException: Reached error page: about:neterror?e=dnsNotFound&u=http%3A//examlpe.org/&c=UTF-8&f=regular&d=Firefox%20can%E2%80%99t%20find%20the%20server%20at%20examlpe.org.

Whereby with my proposed change we correctly get:

INFO ******* Location: http://www.examlpe.org/ Thu Oct 27 2016 15:55:16 GMT+0200 (CEST)
INFO ******* Base URI: about:neterror?e=dnsNotFound&u=http%3A//www.examlpe.org/&c=UTF-8&f=regular&d=Firefox%20can%E2%80%99t%20find%20the%20server%20at%20www.examlpe.org. Thu Oct 27 2016 15:55:16 GMT+0200 (CEST)
INFO ******* Location: about:blank Thu Oct 27 2016 15:55:17 GMT+0200 (CEST)
INFO ******* Base URI: about:blank Thu Oct 27 2016 15:55:17 GMT+0200 (CEST)

Comment 10

[Mike Conley (:mconley) (:⚙️)]

I believe content.location is a perfectly valid way of navigating from within a framescript.

Comment 11

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

Looks like we may have to change exactly how we load pages via bug 1333458.

Comment 12

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

So far nothing has been turned out to be wrong with using `content.location`. I will close this bug as WFM for now with the option to re-enable if it needs to.