The default bug view has changed. See this FAQ.

Make refresh command synchronous

ASSIGNED
Assigned to

Status

Testing
Marionette
P1
major
ASSIGNED
8 months ago
13 hours ago

People

(Reporter: ato, Assigned: whimboo)

Tracking

(Blocks: 3 bugs, {ateam-marionette-server, ateam-marionette-spec})

Version 3
ateam-marionette-server, ateam-marionette-spec
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [spec][17/03], URL)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

8 months ago
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.
(Reporter)

Updated

8 months ago
Blocks: 721859
(Assignee)

Comment 1

8 months ago
The underlying issue here is that the pages are getting loaded from bfcache. So as you noticed we do not see those page events.
(Reporter)

Comment 2

5 months ago
This also happens for Element Click.
(Reporter)

Comment 3

5 months ago
geckodriver tracking issue: https://github.com/mozilla/geckodriver/issues/308
(Reporter)

Updated

5 months ago
(Reporter)

Updated

2 months ago
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
(Assignee)

Comment 4

2 months ago
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?
(Assignee)

Comment 5

2 months ago
Well, the bug number I wanted to use should have been bug 1333458.
(Reporter)

Comment 6

2 months ago
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.
Keywords: ateam-marionette-server, ateam-marionette-spec
(Assignee)

Comment 7

a month ago
(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: 1333458
(Assignee)

Comment 8

14 days ago
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
(Assignee)

Comment 9

14 days ago
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
(Assignee)

Comment 10

14 days ago
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
(Assignee)

Comment 12

11 days ago
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.
(Reporter)

Updated

11 days ago
Duplicate of this bug: 1297983
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

10 days ago
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)
(Reporter)

Comment 19

9 days ago
mozreview-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

::: 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+
(Reporter)

Comment 20

9 days ago
mozreview-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+
(Reporter)

Comment 21

9 days ago
mozreview-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+
(Reporter)

Comment 22

9 days ago
mozreview-review
Comment on attachment 8847196 [details]
Bug 1291320 - Make refresh command synchronous.

https://reviewboard.mozilla.org/r/120202/#review122484
Attachment #8847196 - Flags: review+
(Reporter)

Comment 23

9 days ago
(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)
(Assignee)

Comment 24

8 days ago
(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.
(Assignee)

Comment 25

8 days ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

7 days ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

7 days ago
mozreview-review
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 38

4 days ago
mozreview-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

::: 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 39

4 days ago
mozreview-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 40

4 days ago
mozreview-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 41

4 days ago
mozreview-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+
(Assignee)

Comment 42

3 days ago
mozreview-review-reply
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.
(Assignee)

Comment 43

3 days ago
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
(Assignee)

Updated

3 days ago
Blocks: 1335778
(Assignee)

Comment 44

3 days ago
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.
(Assignee)

Comment 45

3 days ago
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
(Assignee)

Comment 46

2 days ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 51

2 days ago
Andreas, if you could have a look at all the patches again, I would appreciate! Thanks.
Flags: needinfo?(ato)
(Assignee)

Updated

2 days ago
Blocks: 1333458
No longer depends on: 1333458
(Assignee)

Updated

2 days ago
Whiteboard: [spec][17/03]
(Reporter)

Comment 52

a day ago
mozreview-review-reply
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.
(Reporter)

Comment 53

a day ago
mozreview-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/#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+
(Reporter)

Comment 54

a day ago
mozreview-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+
(Reporter)

Comment 55

a day ago
mozreview-review-reply
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.
(Reporter)

Comment 56

a day ago
mozreview-review
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+
(Reporter)

Comment 57

a day ago
mozreview-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-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 62

a day ago
(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)
(Reporter)

Comment 63

a day ago
mozreview-review
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-
(Assignee)

Comment 64

a day ago
mozreview-review-reply
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.
(Assignee)

Comment 65

21 hours ago
mozreview-review-reply
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 66

21 hours ago
mozreview-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/#review125428
Attachment #8847193 - Flags: review?(dburns) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 71

20 hours ago
mozreview-review-reply
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.
(Reporter)

Comment 72

20 hours ago
mozreview-review
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+

Comment 73

14 hours ago
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)
(Assignee)

Comment 75

13 hours ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 78

13 hours ago
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)
You need to log in before you can comment on or make changes to this bug.