Closed Bug 1291320 Opened 8 years ago Closed 7 years ago

Make refresh command synchronous

Categories

(Remote Protocol :: Marionette, defect, P1)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect

Tracking

(firefox-esr52 affected, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr52 --- affected
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: ato, Assigned: whimboo)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: pi-marionette-server, pi-marionette-spec, Whiteboard: [spec][17/03])

Attachments

(5 files)

Bug 1291320 - Refactor navigation unit tests by using non-remote pages only if necessary.
59 bytes, text/x-review-board-request
automatedtester
: review+
ato
: review+
Details
Bug 1291320 - Disallow slow loading page to be put into the cache.
59 bytes, text/x-review-board-request
automatedtester
: review+
ato
: review+
Details
Bug 1291320 - Refactor page load algorithm for listener framescript.
59 bytes, text/x-review-board-request
automatedtester
: review+
ato
: review+
Details
Bug 1291320 - Make refresh command synchronous.
59 bytes, text/x-review-board-request
automatedtester
: review+
ato
: review+
Details
Bug 1291320 - Skip test_focus_after_navigation for "Unable to locate element: :focus"
59 bytes, text/x-review-board-request
impossibus
: review+
Details 
Marionette does not listen for onDOMContentLoad events when navigating backwards, forwards, or refreshing.  This is causing tests that use these API calls to be flaky.
Blocks: webdriver
The underlying issue here is that the pages are getting loaded from bfcache. So as you noticed we do not see those page events.
This also happens for Element Click.
Summary: Marionette does not wait for load event when navigating back, forward, or refreshing → Marionette does not wait for load event when refreshing the document
Severity: normal → major
Priority: -- → P1
I feel this should wait for a solution on bug 1288336, so we know better what
could be done here. Andreas, David, what do you think?
Well, the bug number I wanted to use should have been bug 1333458.
As I commented on https://bugzilla.mozilla.org/show_bug.cgi?id=1335778, whichever is finished first can be rebased.  The scope of these bugs aren’t to change the behaviour of the current wait-for-page-to-load algorithm.
(In reply to Andreas Tolfsen ‹:ato› from comment #6)
> As I commented on https://bugzilla.mozilla.org/show_bug.cgi?id=1335778,
> whichever is finished first can be rebased.  The scope of these bugs aren’t
> to change the behaviour of the current wait-for-page-to-load algorithm.

The current page load algorithm is kinda kinda bound to the get() method in listener.js. Given that this code will completely change via bug 1333458, I don't think it make sense to work on that code now, which gets thrown away soon again. Just my opinion.
Depends on: webdriver-navigate
I checked which events we actually see when initiating a forced reload of the page. It will be the following:

* Event type: pagehide
** href: http://127.0.0.1:57348/test.html
** readyState: complete

* Event type: DOMContentLoaded
** href: http://127.0.0.1:57348/test.html
** readyState: interactive

* Event type: pageshow
** href: http://127.0.0.1:57348/test.html
** readyState: complete

So it's again that we see a readyState of `complete` when pagehide is fired. But checking MDN this seems to be right:

> The pagehide event is fired when a session history entry is being traversed from.

As such we cannot assume that the page has been unloaded. Directly calling `pollForReadyState` will fail, because it will return immediately due to readyState `complete`.

So I think what we need is the following which should also apply to goBack(), goForward(), and get():

1. Remoteness changes we should not take into account for normal navigation commands. Those are special.
2. We have to wait for the pagehide/unload/hashchange events to ensure a load has been triggered.
3. Once one of the above events is fired we have to setup the page load listeners
4. Depending on the page load strategy this will be pageshow for complete, and DOMContentLoaded for interactive and error pages.
5. The above should handle a normal navigation request easily
6. Only for remoteness changes we fire `pollForReadyState`, whereby even here we should wait for events, and not check the readyState directly.

Lets see if updating `pollForReadyState` only would allow us to do the above.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Btw. the current implementation is close to be synchronous. The only problem is that we actually register for DOMContentLoaded AFTER we trigger reload(true); Which means that we can miss such events.
Summary: Marionette does not wait for load event when refreshing the document → Make refresh command synchronous
Ok, I have a solution here which fixes all of our page load issues, except the file:// protocol. Which means before I continue on this bug I will provide a solution for bug 1333458.
(In reply to Henrik Skupin (:whimboo) from comment #10)
> Ok, I have a solution here which fixes all of our page load issues, except
> the file:// protocol. Which means before I continue on this bug I will
> provide a solution for bug 1333458.

We shouldnt be waiting for page load on file://. Reading the spec, we dont wait for page load on file://. I will fix this in the spec now.

David
Correct. I just wanted to address that I don't care about it for the time being. But good to see that it has been added to the spec. Thanks.
The attached patch is just a WIP and not finished yet. But it changes our polling approach for readyState to an event driven page load. All current tests are passing, and with the change I was able to kill a lot of code too. Even remoteness changes are not a problem anymore here.

Andreas, it would be great to get feedback from you now before I continue. Please keep in mind that this is not a refactoring as we still might have to do for bug 1333458. It's just the next step, and if it turns out bug 1333458 is not doable in chrome, we can leave it this way.

Thanks.
Flags: needinfo?(ato)
Comment on attachment 8847194 [details]
Bug 1291320 - Disallow slow loading page to be put into the cache.

https://reviewboard.mozilla.org/r/120198/#review122474

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:333
(Diff revision 1)
> +    def test_navigate_timeout_error_remoteness_change(self):
> +        # TODO: check how to verify remoteness change
> +        self.marionette.navigate("about:robots")

I guess you could somehow save a state in listener.js and check if it is still set after navigation?
Attachment #8847194 - Flags: review+
Comment on attachment 8847193 [details]
Bug 1291320 - Refactor navigation unit tests by using non-remote pages only if necessary.

https://reviewboard.mozilla.org/r/120196/#review122478
Attachment #8847193 - Flags: review+
Comment on attachment 8847195 [details]
Bug 1291320 - Refactor page load algorithm for listener framescript.

https://reviewboard.mozilla.org/r/120200/#review122482

::: testing/marionette/listener.js:899
(Diff revision 1)
> -
> -  if (typeof cleanupCallback == "undefined") {
> -    cleanupCallback = () => {};
> -  }
> +}
>  
> -  let endTime = startTime + pageTimeout;
> +let loadListener = {

I would perhaps suggest putting this in testing/marionette/navigate.js and construct it in listener.js, but if you think it is going away soon it is of course OK to have it here for the time being.

::: testing/marionette/listener.js:995
(Diff revision 1)
> +    removeEventListener("DOMContentLoaded", this);
> +    removeEventListener("pageshow", this);
> +    sendError(new TimeoutError("Error loading page, timed out (checkLoad)"), this.command_id);
> +  },
>  
> -      // We have to look at the originalURL because of about: pages,
> +  initWaitAfterRemotenessChange: function (command_id, pageTimeout, startTime) {

I think this abstraction makes sense.

::: testing/marionette/listener.js:1011
(Diff revision 1)
> -          }
> -        }});
> -      }
> -    },
> +  },
>  
> -    onLocationChange() {},
> +  triggerPageLoad: function (trigger, command_id, pageTimeout, url = undefined) {

I would rename this just “navigate” or something, but more importantly, it needs a docstring since trigger is a function.

::: testing/marionette/listener.js:1035
(Diff revision 1)
> -    sendOk(command_id);
> -  }
> -}
>  
> -/**
> - * Cancel the polling and remove the event listener associated with a
> +    if (!loadEventExpected) {
> +      setTimeout

Syntax error here.

::: testing/marionette/listener.js:1053
(Diff revision 1)
> -function getCurrentUrl() {
> -  return content.location.href;
> +function pollForReadyState(msg) {
> +  let {command_id, pageTimeout, startTime} = msg.json;
> -}
>  
> -/**
> +  loadListener.initWaitAfterRemotenessChange(command_id, pageTimeout, startTime);
> - * Get the title of the current browsing context.
> - */
> -function getTitle() {
> -  return curContainer.frame.top.document.title;
>  }

I think this is great.
Attachment #8847195 - Flags: review+
Comment on attachment 8847196 [details]
Bug 1291320 - Make refresh command synchronous.

https://reviewboard.mozilla.org/r/120202/#review122484
Attachment #8847196 - Flags: review+
(In reply to Henrik Skupin (:whimboo) from comment #18)
> Andreas, it would be great to get feedback from you now before I continue.
> Please keep in mind that this is not a refactoring as we still might have to
> do for bug 1333458. It's just the next step, and if it turns out bug 1333458
> is not doable in chrome, we can leave it this way.

Generally I like the direction this is heading in.  You’re on the right path,
I think.
Flags: needinfo?(ato)
(In reply to Andreas Tolfsen ‹:ato› from comment #23)
> Generally I like the direction this is heading in.  You’re on the right path,
> I think.

Thanks. Whereby I didn't expect a full review for the attached patches as stated above.

To ensure nothing obvious is missing/failing I will request review from David again in those cases changes are necessary for patches.
Comment on attachment 8847194 [details]
Bug 1291320 - Disallow slow loading page to be put into the cache.

https://reviewboard.mozilla.org/r/120198/#review122474

> I guess you could somehow save a state in listener.js and check if it is still set after navigation?

No, I might want to check the one and only important property `Browser.isRemoteBrowser` as run in chrome context. I would only have to mimic all the logic for different applications. But looks like there is no other way around that.
Comment on attachment 8847195 [details]
Bug 1291320 - Refactor page load algorithm for listener framescript.

https://reviewboard.mozilla.org/r/120200/#review122482

> I would perhaps suggest putting this in testing/marionette/navigate.js and construct it in listener.js, but if you think it is going away soon it is of course OK to have it here for the time being.

I tried to do that proposal but when actually moving this out a lot of other code would have to be duplicated, and passed in and out of any call which uses the listener. For example we have to also pass over the main content window, the current frame, callbacks to sendOk and sendError. This makes it all very complicated.

I would say that we should leave this code in listener.js as a singleton, and check what we can do for that later when I continue investigating navigation in chrome scope.
Comment on attachment 8847195 [details]
Bug 1291320 - Refactor page load algorithm for listener framescript.

https://reviewboard.mozilla.org/r/120200/#review123596

::: commit-message-3b659:3
(Diff revision 4)
> +Bug 1291320 - Refactor page load algorithm for listener framescript.
> +
> +MozReview-Commit-ID: IVtO6KgJFES

Details commit messages will follow with the next push of the patches.

::: testing/marionette/listener.js:951
(Diff revision 4)
> - * current browsing context, which means it handles the case where we
> - * navigate within an iframe.  All other navigation is handled by the
> - * driver (in chrome space).
> - */
> +   */
> -function get(msg) {
> -  let {pageTimeout, url, command_id} = msg.json;
> +  handleEvent: function (event) {
> +    dump("** Load event: " + event.type + "\n");

I left those calls to `dump()` in for now just in the case i have to do some follow-up changes based on the review. Those lines will be removed in the final patch before landing.
Comment on attachment 8847193 [details]
Bug 1291320 - Refactor navigation unit tests by using non-remote pages only if necessary.

https://reviewboard.mozilla.org/r/120196/#review124140

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:96
(Diff revision 3)
> +
> +              return tabBrowser.isRemoteBrowser;
> +            """)
> +
> +    @property
> +    def location_href(self):

why not use `get_url()` ?

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:267
(Diff revision 3)
> +    def test_focus_after_navigation(self):
> +        self.marionette.quit()
> +        self.marionette.start_session()
> +
> +        self.marionette.navigate(inline("<input autofocus>"))
> +        active_el = self.marionette.execute_script("return document.activeElement")

`marionette.get_active_element()`
Attachment #8847193 - Flags: review?(dburns) → review-
Comment on attachment 8847194 [details]
Bug 1291320 - Disallow slow loading page to be put into the cache.

https://reviewboard.mozilla.org/r/120198/#review124150
Attachment #8847194 - Flags: review?(dburns) → review+
Comment on attachment 8847196 [details]
Bug 1291320 - Make refresh command synchronous.

https://reviewboard.mozilla.org/r/120202/#review124152
Attachment #8847196 - Flags: review?(dburns) → review+
Comment on attachment 8847195 [details]
Bug 1291320 - Refactor page load algorithm for listener framescript.

https://reviewboard.mozilla.org/r/120200/#review124154
Attachment #8847195 - Flags: review?(dburns) → review+
Comment on attachment 8847193 [details]
Bug 1291320 - Refactor navigation unit tests by using non-remote pages only if necessary.

https://reviewboard.mozilla.org/r/120196/#review124140

> why not use `get_url()` ?

I took this code as written by Andreas. I don't see a reason why to keep it either, but lets see what Andreas had in mind with that.

> `marionette.get_active_element()`

Same here. Maybe this method didn't exist when the code for that test has been written.
There is a remaining failure for Windows 7 VM opt:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73a99d385b7a&selectedJob=84753773

Looking at the full logs I can see:

>1489793935638	Marionette	TRACE	conn60 -> [0,51,"switchToFrame",{"focus":true,"element":"e8c3be10-fee7-4e18-be61-19c02de80c03"}]
> JavaScript error: chrome://marionette/content/listener.js, line 1: SyntaxError: redeclaration of let loadListener
Blocks: 1335778
I have absolutely no idea what this failure is and why it gets redeclared. Also why line 1 is referenced here. I will temporarily add some more debug prints to the driver version of switchToFrame to figure out the underlying reason.
It looks like switching from using `let` to `var` fixes this problem. This also aligns with all other globally declared variables. So I think this will be fine. But I still don't understand why this is happening.

Here the try build which shows the successful jobs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=915cc3e798d27a549661263a716811f953b2afd2&selectedJob=85365708
I also tried to leave in the tracing output for which I used dump(), but as it looks like `logger.trace()` or any other level doesn't work when used from inside an event handler. So I'm going to remove the extra logging for now. We might want to check why that is the case at a later time.
Andreas, if you could have a look at all the patches again, I would appreciate! Thanks.
Flags: needinfo?(ato)
Blocks: webdriver-navigate
No longer depends on: webdriver-navigate
Whiteboard: [spec][17/03]
Comment on attachment 8847193 [details]
Bug 1291320 - Refactor navigation unit tests by using non-remote pages only if necessary.

https://reviewboard.mozilla.org/r/120196/#review124140

> I took this code as written by Andreas. I don't see a reason why to keep it either, but lets see what Andreas had in mind with that.

I think we had one issue in the past where get_url/getCurrentUrl was not reliable.  Possibly it had to do with command/response race conditions before we had sequences in protocol that we have today.

We appear to be using get_url/getCurrentUrl successfully elsewhere in this test, so cases of location_href can probably be replaced safely with get_url.  Feel free to remove it, but I don’t feel this is a blocker for landing this patch.

> Same here. Maybe this method didn't exist when the code for that test has been written.

I think that is a likely reason.  Same reply from me as above: Feel free to remove if you want, but don’t consider it a blocker for landing the patch.
Comment on attachment 8847193 [details]
Bug 1291320 - Refactor navigation unit tests by using non-remote pages only if necessary.

https://reviewboard.mozilla.org/r/120196/#review125320

::: commit-message-6d38a:1
(Diff revision 4)
> +Bug 1291320 - Refactor navigation unit tests and use non remote pages only if absolutely necessary.
> +
> +Using non-remote pages causes framescripts to be reloaded. We should try to avoid using those
> +pages as much as possible, and test remoteness switches in particular tests only. This is to
> +reduce possible hangs.

Try limiting the length of the first line and wrap subsequent commit message at ~72 characters.
Attachment #8847193 - Flags: review+
Comment on attachment 8847196 [details]
Bug 1291320 - Make refresh command synchronous.

https://reviewboard.mozilla.org/r/120202/#review125324

::: commit-message-621d9:3
(Diff revision 5)
> +This updates the refresh command to make use of the new page load
> +algorithm.

Please elaborate what this means.
Attachment #8847196 - Flags: review+
Comment on attachment 8847194 [details]
Bug 1291320 - Disallow slow loading page to be put into the cache.

https://reviewboard.mozilla.org/r/120198/#review122474

> No, I might want to check the one and only important property `Browser.isRemoteBrowser` as run in chrome context. I would only have to mimic all the logic for different applications. But looks like there is no other way around that.

I didn’t know about isRemoteBrowser.  That is much better.
Comment on attachment 8847194 [details]
Bug 1291320 - Disallow slow loading page to be put into the cache.

https://reviewboard.mozilla.org/r/120198/#review125328

::: testing/marionette/harness/marionette_harness/runner/httpd.py:42
(Diff revision 4)
> +    response.content = """<!doctype html>
> +<html>
> +<head>
> +  <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
> +</head>
> +<body>
> +  <title>Slow page loading</title>
> +  <p>Delay: <span id="delay">{}</span></p>
> +</body>
> +</html>

Drop all the boilerplate.  This is sufficient for making a valid HTML document:

> <!doctype html>
> <meta charset=utf-8>
> <title>Slow page loading</title>
> 
> <!-- content -->
Attachment #8847194 - Flags: review+
Comment on attachment 8847195 [details]
Bug 1291320 - Refactor page load algorithm for listener framescript.

https://reviewboard.mozilla.org/r/120200/#review125332

::: testing/marionette/driver.js:417
(Diff revisions 1 - 5)
> +
> +      Preferences.set("dom.file.createInChild", true);

Did this sneak in as part of a rebase?

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:34
(Diff revisions 1 - 5)
>  
> -        self.test_page = self.marionette.absolute_url('test.html')
> +        self.test_page_insecure = self.fixtures.where_is("test.html", on="https")
> +        self.test_page_not_remote = "about:robots"
> +        self.test_page_remote = self.marionette.absolute_url("test.html")
> +
> +        if self.marionette.session_capabilities['platformName'] == 'darwin':

Use double quotes for consistency.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:110
(Diff revisions 1 - 5)
> +        self.marionette.execute_script(
> +            "window.location.href = '%s'" % self.test_page_remote)

Add a sandbox=None argument.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:142
(Diff revisions 1 - 5)
> +        self.assertTrue(self.marionette.execute_script(
> +            """var elem = window.document.createElement('div'); elem.id = 'someDiv';
> +            window.document.body.appendChild(elem); return true;"""))

We can probably drop the return of a boolean and the assert here.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:142
(Diff revisions 1 - 5)
> +        self.assertTrue(self.marionette.execute_script(
> +            """var elem = window.document.createElement('div'); elem.id = 'someDiv';
> +            window.document.body.appendChild(elem); return true;"""))

Add a sandbox=None argument.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:145
(Diff revisions 1 - 5)
> +        self.assertFalse(self.marionette.execute_script(
> +            "return window.document.getElementById('someDiv') == undefined"))

I know this wasn’t you, but do you mind adding a typeof here?

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:148
(Diff revisions 1 - 5)
> +        # TODO(ato): Bug 1291320
> +        time.sleep(0.2)

Presumably this can be dropped now?

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:151
(Diff revisions 1 - 5)
> +        self.assertTrue(self.marionette.execute_script(
> +            "return window.document.getElementById('someDiv') == undefined"))

Same.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:173
(Diff revisions 1 - 5)
> +        state = self.marionette.execute_script(
> +            "return window.document.readyState")

Add a sandbox=None argument here.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:245
(Diff revisions 1 - 5)
> +            urlbar = self.marionette.find_element(By.ID, 'urlbar')
> +            urlbar.send_keys(self.mod_key + 'a')
> +            urlbar.send_keys(self.mod_key + 'x')
> +            urlbar.send_keys('about:support' + Keys.ENTER)

Double quotes.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:261
(Diff revisions 1 - 5)
> +            urlbar = self.marionette.find_element(By.ID, 'urlbar')
> +            urlbar.send_keys(self.mod_key + 'a')
> +            urlbar.send_keys(self.mod_key + 'x')

Quotes.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:278
(Diff revisions 1 - 5)
> +
> +        def open_with_shortcut():
> +            self.marionette.navigate(self.test_page_remote)
> +            with self.marionette.using_context("chrome"):
> +                main_win = self.marionette.find_element(By.ID, "main-window")
> +                main_win.send_keys(self.mod_key, Keys.SHIFT, 'a')

Quotes.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:305
(Diff revisions 1 - 5)
> +            if "is_remote" in page:
> +                self.assertEqual(page["is_remote"], self.is_remote_tab,
> +                                 "'{}' doesn't match expected remoteness state: {}".format(
> +                                     page["url"], page["is_remote"]))
> +
>          for page in test_pages[-2::-1]:

Add comment explaining the slice.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:318
(Diff revisions 1 - 5)
> +        if "is_remote" in page:
> +            self.assertEqual(page["is_remote"], self.is_remote_tab,
> +                             "'{}' doesn't match expected remoteness state: {}".format(
> +                                 page["url"], page["is_remote"]))
> +
>          for page in test_pages[1::]:

Explain slicing.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:409
(Diff revisions 1 - 5)
>              {"url": self.marionette.absolute_url('black.png')},
> -            {"url": self.test_page},
> +            {"url": self.test_page_remote},
>              {"url": self.marionette.absolute_url('white.png')},

Double quotes.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:471
(Diff revisions 1 - 5)
>          with self.assertRaises(errors.TimeoutException):
>              self.marionette.go_back()
> -        self.assertEqual(urls[0], self.marionette.get_url())
> -        self.marionette.timeout.page_load = 300000
> +        self.marionette.timeout.reset()
> +
> +        Wait(self.marionette, self.marionette.timeout.page_load).until(
> +            lambda _: urls[0] == self.marionette.get_url(),

s/_/mn/
s/self.marionette/mn/

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:485
(Diff revisions 1 - 5)
>          with self.assertRaises(errors.TimeoutException):
>              self.marionette.go_forward()
> -        self.assertEqual(urls[2], self.marionette.get_url())
> -        self.marionette.timeout.page_load = 300000
> +        self.marionette.timeout.reset()
> +
> +        Wait(self.marionette, self.marionette.timeout.page_load).until(
> +            lambda _: urls[2] == self.marionette.get_url(),

s/_/mn/
s/self.marionette/mn/

::: testing/marionette/listener.js:130
(Diff revisions 1 - 5)
> +   *
> +   * @param {number} command_id
> +   *     ID of the currently handled message between the driver and listener.
> +   * @param {number} timeout
> +   *     Timeout in seconds the method has to wait for the page being finished loading.
> +   * @param {boolean=} waitForUnloaded

These two are documented in reverse order.

::: testing/marionette/listener.js:135
(Diff revisions 1 - 5)
> +   * @param {boolean=} waitForUnloaded
> +   *     If `true` wait for page unload events, otherwise only for page load events.
> +   * @param {number} startTime
> +   *     Unix timestap when the navitation request got triggered.
> +   */
> +  startListening: function (command_id, timeout, startTime, waitForUnloaded = true) {

Since this object is called the loadListener, I think it would be sufficient to rename startListening to ‘start’ and stopListening to ‘stop’.

::: testing/marionette/listener.js:162
(Diff revisions 1 - 5)
> +  },
> +
> +  /**
> +   * Stop listening for page unload/load events.
> +   */
> +  stopListening: function () {

Rename to ‘stop’.
Attachment #8847195 - Flags: review-
(In reply to Henrik Skupin (:whimboo) from comment #51)
> Andreas, if you could have a look at all the patches again, I would
> appreciate! Thanks.

I had another pass at this, and I feel it’s almost ready.  Unfortunately there are a few nits and I spotted some things in the code you have moved around that it would be good to fix alongside these changes.

I don’t think it should take you too long to fix up the issues I raised.
Flags: needinfo?(ato)
Comment on attachment 8847195 [details]
Bug 1291320 - Refactor page load algorithm for listener framescript.

https://reviewboard.mozilla.org/r/120200/#review125338
Attachment #8847195 - Flags: review?(ato) → review-
Comment on attachment 8847193 [details]
Bug 1291320 - Refactor navigation unit tests by using non-remote pages only if necessary.

https://reviewboard.mozilla.org/r/120196/#review124140

> I think we had one issue in the past where get_url/getCurrentUrl was not reliable.  Possibly it had to do with command/response race conditions before we had sequences in protocol that we have today.
> 
> We appear to be using get_url/getCurrentUrl successfully elsewhere in this test, so cases of location_href can probably be replaced safely with get_url.  Feel free to remove it, but I don’t feel this is a blocker for landing this patch.

It's easy to remove, so I will do it now.

> I think that is a likely reason.  Same reply from me as above: Feel free to remove if you want, but don’t consider it a blocker for landing the patch.

Simple, so I do it now.
Comment on attachment 8847195 [details]
Bug 1291320 - Refactor page load algorithm for listener framescript.

https://reviewboard.mozilla.org/r/120200/#review125332

> Did this sneak in as part of a rebase?

Yes, that is what I noticed yesterday and got Andrea to move it to geckoinstance.py via bug 1349859.

> We can probably drop the return of a boolean and the assert here.

This is obselete given that the test is refactored in the next commit anyway. This is a simple copy and paste from what we had before.

> Add a sandbox=None argument.

Same as above. All this code goes away with the next commit.

> I know this wasn’t you, but do you mind adding a typeof here?

Same as above. This code goes away.

> Presumably this can be dropped now?

Not in this commit. Updating refresh() is the next commit in this series.

> Same.

Same as above.

> Since this object is called the loadListener, I think it would be sufficient to rename startListening to ‘start’ and stopListening to ‘stop’.

Makes sense. I updated it.
Comment on attachment 8847193 [details]
Bug 1291320 - Refactor navigation unit tests by using non-remote pages only if necessary.

https://reviewboard.mozilla.org/r/120196/#review125428
Attachment #8847193 - Flags: review?(dburns) → review+
Comment on attachment 8847195 [details]
Bug 1291320 - Refactor page load algorithm for listener framescript.

https://reviewboard.mozilla.org/r/120200/#review125332

> This is obselete given that the test is refactored in the next commit anyway. This is a simple copy and paste from what we had before.

Makes sense.

> Same as above. All this code goes away with the next commit.

OK.

> Not in this commit. Updating refresh() is the next commit in this series.

Oh, of course.
Comment on attachment 8847195 [details]
Bug 1291320 - Refactor page load algorithm for listener framescript.

https://reviewboard.mozilla.org/r/120200/#review125462
Attachment #8847195 - Flags: review?(ato) → review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db38ad37d57c
Refactor navigation unit tests by using non-remote pages only if necessary. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/7c06fe3e76fc
Disallow slow loading page to be put into the cache. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/7c314416a41c
Refactor page load algorithm for listener framescript. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/1c0d8a46504a
Make refresh command synchronous. r=ato,automatedtester
Backed out for failing xpcshell test testing/marionette/test_navigate.js:

https://hg.mozilla.org/integration/autoland/rev/4470faf1886e5fa14845e218eb73f6b89d3b3a6c
https://hg.mozilla.org/integration/autoland/rev/b1f8a4a80d1a75d7446262626b74149e0dbe65f6
https://hg.mozilla.org/integration/autoland/rev/0e16d2a2ce164e7c58577fd70e6053420effb5ef
https://hg.mozilla.org/integration/autoland/rev/c62bb4dbe6046ad1e58039447ae30ee8d7531aff

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1c0d8a46504a7e3fa3ea5524eee46553554efe6f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=86033854&repo=autoland

[task 2017-03-23T20:43:03.392648Z] 20:43:03     INFO -  TEST-START | testing/marionette/test_navigate.js
[task 2017-03-23T20:43:04.889034Z] 20:43:04  WARNING -  TEST-UNEXPECTED-FAIL | testing/marionette/test_navigate.js | xpcshell return code: 0
[task 2017-03-23T20:43:04.907982Z] 20:43:04     INFO -  TEST-INFO took 1499ms
[task 2017-03-23T20:43:04.908360Z] 20:43:04     INFO -  >>>>>>>
[task 2017-03-23T20:43:04.910596Z] 20:43:04     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2017-03-23T20:43:04.912401Z] 20:43:04     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2017-03-23T20:43:04.914202Z] 20:43:04     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2017-03-23T20:43:04.916015Z] 20:43:04     INFO -  running event loop
[task 2017-03-23T20:43:04.917901Z] 20:43:04     INFO -  testing/marionette/test_navigate.js | Starting test_isLoadEventExpected
[task 2017-03-23T20:43:04.919955Z] 20:43:04     INFO -  (xpcshell/head.js) | test test_isLoadEventExpected pending (2)
[task 2017-03-23T20:43:04.922153Z] 20:43:04     INFO -  TypeError: Expected destination URL at chrome://marionette/content/navigate.js:31
[task 2017-03-23T20:43:04.925937Z] 20:43:04     INFO -  navigate.isLoadEventExpected@chrome://marionette/content/navigate.js:31:11
[task 2017-03-23T20:43:04.927943Z] 20:43:04     INFO -  test_isLoadEventExpected/<@/home/worker/workspace/build/tests/xpcshell/tests/testing/marionette/test_navigate.js:12:23
[task 2017-03-23T20:43:04.929806Z] 20:43:04     INFO -  proto.throws@resource://testing-common/Assert.jsm:349:5
[task 2017-03-23T20:43:04.932322Z] 20:43:04     INFO -  test_isLoadEventExpected@/home/worker/workspace/build/tests/xpcshell/tests/testing/marionette/test_navigate.js:12:3
[task 2017-03-23T20:43:04.939474Z] 20:43:04     INFO -  _run_next_test@/home/worker/workspace/build/tests/xpcshell/head.js:1569:11
[task 2017-03-23T20:43:04.941720Z] 20:43:04     INFO -  run@/home/worker/workspace/build/tests/xpcshell/head.js:703:9
[task 2017-03-23T20:43:04.943576Z] 20:43:04     INFO -  _do_main@/home/worker/workspace/build/tests/xpcshell/head.js:211:5
[task 2017-03-23T20:43:04.946775Z] 20:43:04     INFO -  _execute_test@/home/worker/workspace/build/tests/xpcshell/head.js:546:5
[task 2017-03-23T20:43:04.952249Z] 20:43:04     INFO -  @-e:1:1
[task 2017-03-23T20:43:04.954499Z] 20:43:04     INFO -  exiting test
[task 2017-03-23T20:43:04.956331Z] 20:43:04     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2017-03-23T20:43:04.958198Z] 20:43:04     INFO -  "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2017-03-23T20:43:04.960175Z] 20:43:04     INFO -  "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2017-03-23T20:43:04.961917Z] 20:43:04     INFO -  <<<<<<<
Flags: needinfo?(hskupin)
I totally missed to remove the extra tests which exercise code that doesn't exist anymore or has been modified. I will update the patch accordingly.
Flags: needinfo?(hskupin)
Comment on attachment 8847195 [details]
Bug 1291320 - Refactor page load algorithm for listener framescript.

Updated patch with specific xpshell tests removed. Andreas, can you please check again? Thanks.
Attachment #8847195 - Flags: review?(ato)
Comment on attachment 8847195 [details]
Bug 1291320 - Refactor page load algorithm for listener framescript.

https://reviewboard.mozilla.org/r/120200/#review125846
Comment on attachment 8847195 [details]
Bug 1291320 - Refactor page load algorithm for listener framescript.

Looks good to me.
Attachment #8847195 - Flags: review?(ato)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5573ab27d696
Refactor navigation unit tests by using non-remote pages only if necessary. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/e99f058812bf
Disallow slow loading page to be put into the cache. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/3f619577e5d7
Refactor page load algorithm for listener framescript. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/8580c2a3629e
Make refresh command synchronous. r=ato,automatedtester
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c5d560a65f0
Backed out changeset 8580c2a3629e 
https://hg.mozilla.org/integration/autoland/rev/905cf165dd1b
Backed out changeset 3f619577e5d7 
https://hg.mozilla.org/integration/autoland/rev/04de653dce8a
Backed out changeset e99f058812bf 
https://hg.mozilla.org/integration/autoland/rev/88c9c2bcfaf9
Backed out changeset 5573ab27d696 for timeouts in navigation.py
The backout here is because of test failure for Wd wpt-tests for Linux64 opt/pgo:

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8580c2a3629ef717355ec74a175c7c89013b7e28&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-resultStatus=coalesced&filter-resultStatus=pending&filter-resultStatus=running&filter-searchStr=wd

With the second last push on March 23rd my changeset was excluded by SETA from running against those builds. It's unfortunate given that we might want to run tests for all builds if changes to those tests are part of the test. Joel, is that something SETA could do, or would that need a more complex configuration via Taskcluster?

I also rebased my patch against current mozilla-central and can reproduce this issue now. I was clearly not seeing it by last week. So another change which landed the last days should have caused this.
Flags: needinfo?(jmaher)
The problem here was that with my patch we correctly identify an invalid URL but didn't return it correctly to the driver module. It was a simple change, but also noticed that we have to hardening the call to ´trigger()´ inside the new ´navigate()´ method.

Updated patch is upcoming.
Andreas, I need an additional review for those changes:
https://reviewboard.mozilla.org/r/120194/diff/8-10/

Thanks.
Flags: needinfo?(ato)
good question :whimboo- there is probably room for 'when' clauses to ensure specific jobs are run.  Right now the 'when' clause in taskcluster is used to optimize out jobs when there isn't a match.  We schedule everything and optimize stuff out if it isn't match by the 'when' clause.

could we support something for 'seta'?  Possibly, but I wouldn't want to just hack it in.

:rwood, what do you think about adding logic to SETA to ensure we do not optimize out jobs when specific files change.
Flags: needinfo?(jmaher) → needinfo?(rwood)
Blocks: 1323755
> :rwood, what do you think about adding logic to SETA to ensure we do not
> optimize out jobs when specific files change.

I think that makes total sense i.e. don't optimize out a test job which contains a test whose source was changed in the actual push itself. I do think we should add this to SETA, however I think it makes sense to wait and include this logic when we are refactoring SETA for the manifest-based testing changes coming in the future. I think a big part of it will be deciding the best way to map test source to TC jobs.  This may also open up another option - the ability to only run test jobs based on the code that was touched in the push, excluding others. I filed Bug 1351029 for the first case.
Flags: needinfo?(rwood)
Comment on attachment 8847193 [details]
Bug 1291320 - Refactor navigation unit tests by using non-remote pages only if necessary.

https://reviewboard.mozilla.org/r/120194/#review126820

::: testing/marionette/listener.js:279
(Diff revisions 8 - 10)
> +      sendError(new UnknownCommandError(e.message), command_id);
> +    }

Missing return statement.
Flags: needinfo?(ato)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0faaff778221
Refactor navigation unit tests by using non-remote pages only if necessary. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/98267962a9a7
Disallow slow loading page to be put into the cache. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/cf7d24fe5cec
Refactor page load algorithm for listener framescript. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/dba85e8288af
Make refresh command synchronous. r=ato,automatedtester
Backed out for frequently failing test_navigation.py TestNavigate.test_focus_after_navigation:

https://hg.mozilla.org/integration/autoland/rev/7c1fe74aad56bc08877eefe8daddaae75f9734c8
https://hg.mozilla.org/integration/autoland/rev/517ccea63fe0283ac9ffa3976da60e088c4dd853
https://hg.mozilla.org/integration/autoland/rev/7697c16a882838db729af63efdd8790a976f2342
https://hg.mozilla.org/integration/autoland/rev/bb3aeeaea5f7630af625bdf7298832f135e6dc39

Failure logs (first errors from previous landing): https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1332064
Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=87080821&repo=autoland

[task 2017-03-28T21:48:34.418852Z] 21:48:34     INFO - TEST-START | test_navigation.py TestNavigate.test_focus_after_navigation
[task 2017-03-28T21:48:34.824078Z] 21:48:34     INFO - Application command: /home/worker/workspace/build/application/firefox/firefox -no-remote -marionette -profile /tmp/tmpdC2gUw.mozrunner
[task 2017-03-28T21:48:38.251616Z] 21:48:38     INFO - TEST-UNEXPECTED-ERROR | test_navigation.py TestNavigate.test_focus_after_navigation | NoSuchElementException: Unable to locate element: :focus
[task 2017-03-28T21:48:38.252493Z] 21:48:38     INFO - stacktrace:
[task 2017-03-28T21:48:38.252630Z] 21:48:38     INFO - 	WebDriverError@chrome://marionette/content/error.js:211:5
[task 2017-03-28T21:48:38.254125Z] 21:48:38     INFO - 	NoSuchElementError@chrome://marionette/content/error.js:399:5
[task 2017-03-28T21:48:38.254721Z] 21:48:38     INFO - 	element.find/</<@chrome://marionette/content/element.js:278:16
[task 2017-03-28T21:48:38.255379Z] 21:48:38     INFO - 	promise callback*element.find/<@chrome://marionette/content/element.js:264:5
[task 2017-03-28T21:48:38.255993Z] 21:48:38     INFO - 	element.find@chrome://marionette/content/element.js:254:10
[task 2017-03-28T21:48:38.257063Z] 21:48:38     INFO - 	findElementContent@chrome://marionette/content/listener.js:1199:18
[task 2017-03-28T21:48:38.257649Z] 21:48:38     INFO - 	TaskImpl_run@resource://gre/modules/Task.jsm:319:42
[task 2017-03-28T21:48:38.258712Z] 21:48:38     INFO - 	TaskImpl@resource://gre/modules/Task.jsm:277:3
[task 2017-03-28T21:48:38.259920Z] 21:48:38     INFO - 	asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-03-28T21:48:38.260587Z] 21:48:38     INFO - 	Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-03-28T21:48:38.261334Z] 21:48:38     INFO - 	TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:389:16
[task 2017-03-28T21:48:38.261912Z] 21:48:38     INFO - 	TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-03-28T21:48:38.263425Z] 21:48:38     INFO - 	TaskImpl@resource://gre/modules/Task.jsm:277:3
[task 2017-03-28T21:48:38.264500Z] 21:48:38     INFO - 	asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-03-28T21:48:38.265422Z] 21:48:38     INFO - 	Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-03-28T21:48:38.265644Z] 21:48:38     INFO - 	dispatch/<@chrome://marionette/content/listener.js:363:15
[task 2017-03-28T21:48:38.265781Z] 21:48:38     INFO - Traceback (most recent call last):
[task 2017-03-28T21:48:38.265916Z] 21:48:38     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/testcases.py", line 166, in run
[task 2017-03-28T21:48:38.265983Z] 21:48:38     INFO -     testMethod()
[task 2017-03-28T21:48:38.266087Z] 21:48:38     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/decorators.py", line 82, in skip_wrapper
[task 2017-03-28T21:48:38.266197Z] 21:48:38     INFO -     return test_item(self, *args, **kwargs)
[task 2017-03-28T21:48:38.266276Z] 21:48:38     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/decorators.py", line 150, in skip_wrapper
[task 2017-03-28T21:48:38.266344Z] 21:48:38     INFO -     return test_item(self, *args, **kwargs)
[task 2017-03-28T21:48:38.266562Z] 21:48:38     INFO -   File "/home/worker/workspace/build/tests/marionette/tests/testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py", line 199, in test_focus_after_navigation
[task 2017-03-28T21:48:38.266621Z] 21:48:38     INFO -     focus_el = self.marionette.find_element(By.CSS_SELECTOR, ":focus")
[task 2017-03-28T21:48:38.266739Z] 21:48:38     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 1938, in find_element
[task 2017-03-28T21:48:38.266802Z] 21:48:38     INFO -     return self._send_message("findElement", body, key="value")
[task 2017-03-28T21:48:38.266943Z] 21:48:38     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/decorators.py", line 23, in _
[task 2017-03-28T21:48:38.266996Z] 21:48:38     INFO -     return func(*args, **kwargs)
[task 2017-03-28T21:48:38.267121Z] 21:48:38     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 735, in _send_message
[task 2017-03-28T21:48:38.267173Z] 21:48:38     INFO -     self._handle_error(err)
[task 2017-03-28T21:48:38.267254Z] 21:48:38     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 768, in _handle_error
[task 2017-03-28T21:48:38.267383Z] 21:48:38     INFO -     raise errors.lookup(error)(message, stacktrace=stacktrace)
[task 2017-03-28T21:48:38.267475Z] 21:48:38     INFO - TEST-INFO took 3774ms
Flags: needinfo?(hskupin)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #100)
> Failure logs (first errors from previous landing):
> https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1332064
> Failure log example:
> https://treeherder.mozilla.org/logviewer.html#?job_id=87080821&repo=autoland

So I triggered a couple of jobs to see how often it is failing and is a bit rarely. So not sure if my patch is really causing it. Sebastian, what the limit of failures when Sheriffs say it has to be backed out?

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=722cb89d1cb2b220151045899c6d546c1c8b1a2e&filter-searchStr=mn

If it needs a fix it will have to wait until early next week.
Flags: needinfo?(hskupin) → needinfo?(aryx.bugmail)
So in the long the failure rate is ~10%. There is no fixed failure threshold for which a patch will get backed out, but in general if it's noticeable, it will get backed out. If you want to land the code because it e.g. bitrots too fast, disable the test and manage it in a follow-up.
Flags: needinfo?(aryx.bugmail)
[mass] Setting priority
Depends on: 1332064
Comment on attachment 8854147 [details]
Bug 1291320 - Skip test_focus_after_navigation for "Unable to locate element: :focus"

https://reviewboard.mozilla.org/r/126126/#review128690
Attachment #8854147 - Flags: review?(mjzffr) → review+
To be safe with landing the patch again, I triggered another try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aae94c718dffc6f8ab2a002d40db28dec4f5e9f
The recent rebase against central let a failure to slip through. I fixed that with the recent push, and triggered another try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07d22e78c789e4e7dc7f9f6a3e2883ad795cecd5
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/700a15f2800d
Refactor navigation unit tests by using non-remote pages only if necessary. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/11a425ae2d9a
Disallow slow loading page to be put into the cache. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/c7d68974f5a8
Refactor page load algorithm for listener framescript. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/c6eb25a8dc1c
Make refresh command synchronous. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/c55face61854
Skip test_focus_after_navigation for "Unable to locate element: :focus" r=maja_zf
Please uplift this test-only patch to aurora and beta. esr52 has to wait for other patches being uplifted first. Thanks.
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
Whiteboard: [spec][17/03] → [spec][17/03][checkin-needed-aurora][checkin-needed-beta]
Depends on: 1353599
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: