1280300 - Marionette hangs when navigating to a fragment on the current page

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[jmleyba]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.84 Safari/537.36

Steps to reproduce:

Using:  geckodriver-0.8.0-linux64

Test case:
https://github.com/SeleniumHQ/selenium/blob/master/javascript/node/selenium-
webdriver/test/page_loading_test.js#L57

Steps:
driver.get(url);  // Marionette returns
driver.get(url + '#someFragment');  // Marionette never returns


Actual results:

Firefox navigates to the URL, but Marionette never returns a response to the client.

Marionette output:
1466015529314	Marionette	DEBUG	Marionette enabled via build flag and pref
1466015529644	Marionette	INFO	Listening on port 2828
1466015530289	Marionette	DEBUG	Marionette enabled via command-line flag
1466015530349	Marionette	DEBUG	Accepted connection conn0 from 127.0.0.1:57886
1466015530399	Marionette	TRACE	conn0 -> [0,1,"newSession",{"capabilities":{"desiredCapabilities":{"browserName":"firefox"},"requiredCapabilities":{}},"sessionId":null}]
1466015530400	Marionette	CONFIG	Changing capabilities: {"browserName":"Firefox","browserVersion":"47.0","platformName":"Linux","platformVersion":"3.13.0-83-generic","specificationLevel":0,"raisesAccessibilityExceptions":false,"rotatable":false,"acceptSslCerts":false,"takesElementScreenshot":true,"takesScreenshot":true,"proxy":{},"platform":"LINUX","XULappId":"{ec8030f7-c20a-464f-9b0e-13a3a9e97384}","appBuildId":"20160606114208","device":"desktop","version":"47.0"}
1466015531369	Marionette	DEBUG	loaded listener.js
1466015531373	Marionette	DEBUG	loaded listener.js
1466015531593	Marionette	TRACE	conn0 <- [1,1,null,{"sessionId":"c76e9f9a-9c8f-4c69-9b8d-97b1ef60130d","capabilities":{"browserName":"Firefox","browserVersion":"47.0","platformName":"Linux","platformVersion":"3.13.0-83-generic","specificationLevel":0,"raisesAccessibilityExceptions":false,"rotatable":false,"acceptSslCerts":false,"takesElementScreenshot":true,"takesScreenshot":true,"proxy":{},"platform":"LINUX","XULappId":"{ec8030f7-c20a-464f-9b0e-13a3a9e97384}","appBuildId":"20160606114208","device":"desktop","version":"47.0","command_id":1}}]
1466015531638	Marionette	TRACE	conn0 -> [0,2,"get",{"url":"http://127.0.0.1:41308/common/xhtmlTest.html"}]
1466015533144	Marionette	TRACE	conn0 <- [1,2,null,{}]
1466015533149	Marionette	TRACE	conn0 -> [0,3,"get",{"url":"http://127.0.0.1:41308/common/xhtmlTest.html#text"}]
JavaScript warning: https://normandy.cdn.mozilla.net/static/js/bundles/selfrepair-7575f6d27445b45bcffb.js, line 9825: mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create




Expected results:

Marionette should not hang.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

I assume this is happening because we are waiting for an unload and load event to be happening, which in such a case will not occur.

Comment 2

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> I assume this is happening because we are waiting for an unload and load
> event to be happening, which in such a case will not occur.

Excellent point.  Navigating in this case should not wait for those events.

We could convert the input string URL into a URL object and compare it with the current URL.  If it is the same and a fragment is set, skip waiting for unload- and load events.

Comment 4

[Andreas Tolfsen ❲:ato❳]

https://github.com/mozilla/geckodriver/issues/150

Comment 5

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1280300 - Lint navigation command;

Review commit: https://reviewboard.mozilla.org/r/65280/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65280/

Comment 6

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1280300 - Support navigation by fragment;

Adds support for navigating to a fragment on the currenty visible document
without waiting for a DOM event that the document has been fully loaded.

This addresses https://github.com/mozilla/geckodriver/issues/150.

Review commit: https://reviewboard.mozilla.org/r/65282/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65282/

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772487 [details]
Bug 1280300 - Lint navigation command;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65280/diff/1-2/

Comment 8

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65282/diff/1-2/

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772487 [details]
Bug 1280300 - Lint navigation command;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65280/diff/2-3/

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65282/diff/2-3/

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772487 [details]
Bug 1280300 - Lint navigation command;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65280/diff/3-4/

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65282/diff/3-4/

Comment 13

[David Burns :automatedtester]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

https://reviewboard.mozilla.org/r/65282/#review62556

::: testing/marionette/navigate.js:53
(Diff revision 4)
> +    return false;
> +  }
> +
> +  // navigating to same url, but with any hash
> +  if (cur.origin == fut.origin &&
> +      cur.pathname == fut.pathname.substring(0, fut.pathname.length - fut.hash.length) &&

The hash shouldn't be in the path name so we shouldnt need to substring

::: testing/marionette/test_navigate.js:24
(Diff revision 4)
> +  equal(true, navigate.isLoadEventExpected("http://a", "about:blank"));
> +  equal(true, navigate.isLoadEventExpected("http://a", "https://a"));
> +  equal(false, navigate.isLoadEventExpected("http://a", "javascript:whatever"));
> +  equal(false, navigate.isLoadEventExpected("http://a", "http://a#b"));
> +  equal(false, navigate.isLoadEventExpected("http://a#b", "http://a#b"));
> +  equal(false, navigate.isLoadEventExpected("http://a#b", "http://a#c"));

Can you add a test for `http://a#`. It should act like `http://a` but it would be good to make sure that it is.

Comment 14

[David Burns :automatedtester]

Comment on attachment 8772487 [details]
Bug 1280300 - Lint navigation command;

https://reviewboard.mozilla.org/r/65280/#review62570

Comment 15

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

Note that I've got a similar solution cooking in bug 1261842. ato - I'm not sure if you want to take my patch instead (because I think it solves at least part of the same problem) or if I should base my solution on top of yours. Let me know how you want to roll.

Comment 16

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65282/diff/4-5/

Comment 17

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65282/diff/5-6/

Comment 18

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65282/diff/6-7/

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

https://reviewboard.mozilla.org/r/65282/#review65916

Because I'm interested in this fix I had a quick looks and added some comments.

::: testing/marionette/harness/marionette/tests/unit/test_navigation.py:61
(Diff revision 7)
>          self.assertEqual("Marionette Test", self.marionette.title)
>          self.marionette.navigate("about:blank")
>          self.assertEqual("about:blank", self.location_href)
>          self.marionette.go_back()
> +        # TODO(ato): Bug 1291320
> +        time.sleep(0.2)

Keep in mind that in the case of back/forward/reload the content comes from the bfcache. So as you said we do not have the unload/load events.

::: testing/marionette/listener.js:899
(Diff revision 7)
> -    end = new Date().getTime();
> +
> -    let aboutErrorRegex = /about:.+(error)\?/;
> -    let elapse = end - start;
>      let doc = curContainer.frame.document;
> -    if (pageTimeout == null || elapse <= pageTimeout) {
> +    let now = new Date().getTime();
> +    if (pageTimeout == null || (now - start) <= pageTimeout) {

Performance wise it would be better to calculate the timeout given `start + timeout` instead of doing it over and over again when this method gets executed.

::: testing/marionette/listener.js:941
(Diff revision 7)
>  function get(msg) {
>    let start = new Date().getTime();
> -  let requestedURL = new URL(msg.json.url).toString();
> +  let command_id = msg.json.command_id;
> +
>    let docShell = curContainer.frame
> -                             .document
> +      .document

nit: 4 vs 2 space indentation. I feel we should keep 2 for JS as it is used in the Mozilla code base.

::: testing/marionette/listener.js:950
(Diff revision 7)
> -                             .QueryInterface(Ci.nsIDocShell);
> +      .QueryInterface(Ci.nsIDocShell);
>    let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> -                            .getInterface(Ci.nsIWebProgress);
> +      .getInterface(Ci.nsIWebProgress);
>    let sawLoad = false;
>  
> +  dump("checking url\n");

Looks like left-over debug code?

::: testing/marionette/navigate.js:32
(Diff revision 7)
> + * @throws TypeError
> + *     If |current| is not defined, or any of |current| or |future|
> + *     are invalid URLs.
> + */
> +navigate.isLoadEventExpected = function(current, future = undefined) {
> +  if (typeof current == "undefined") {

Shouldn't we better check for a string, given that this is the expected input? I don't see that an integer with the value `42` is a valid URL.

::: testing/marionette/test_navigate.js:7
(Diff revision 7)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const {utils: Cu} = Components;
> +
> +Cu.importGlobalProperties(["URL"]);

Is that import necessary?

Comment 20

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65282/diff/7-8/

Comment 21

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/65282/#review65916

> Performance wise it would be better to calculate the timeout given `start + timeout` instead of doing it over and over again when this method gets executed.

This has negligible performance affects.

> nit: 4 vs 2 space indentation. I feel we should keep 2 for JS as it is used in the Mozilla code base.

In Marionette we always use two spaces for indentation and four for continuation of a statement that spans multiple lines.  This is fairly well established.

> Shouldn't we better check for a string, given that this is the expected input? I don't see that an integer with the value `42` is a valid URL.

The type checks are performed by the `URL` constructor.  It is totally unnecessary, and is just a library-level convenience for pointing out that the function needs at least one argument.

> Is that import necessary?

Yes.

Comment 22

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/65282/#review65916

> Keep in mind that in the case of back/forward/reload the content comes from the bfcache. So as you said we do not have the unload/load events.

Ah, good catch.

Comment 23

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/65282/#review62556

> The hash shouldn't be in the path name so we shouldnt need to substring

We do need this check because the fragment is part of pathname for data URLs.

Also we need to check explicitly for zero-length fragments (http://a#) as these produce empty strings: `new URL("http://a#").hash == ""`.

> Can you add a test for `http://a#`. It should act like `http://a` but it would be good to make sure that it is.

Good catch.

Comment 24

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65282/diff/8-9/

Comment 25

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65282/diff/9-10/

Comment 26

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65282/diff/10-11/

Comment 27

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65282/diff/11-12/

Comment 28

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65282/diff/12-13/

Comment 29

[David Burns :automatedtester]

Comment on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

https://reviewboard.mozilla.org/r/65282/#review66354

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39a2655e787d
Support navigation by fragment; r=automatedtester

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39a2655e787d

Comment 32

[Andreas Tolfsen ❲:ato❳]

David, can you uplift this along with the relevant patches from mconley?

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

(In reply to Andreas Tolfsen ‹:ato› from comment #32)
> David, can you uplift this along with the relevant patches from mconley?

Maybe we should wait and first investigate bug 1293107 which seem to be a regression.

Comment 34

[Andreas Tolfsen ❲:ato❳]

Test related uplift.

Sheriffs: Is there an easy way to tell what other patches depend on this?  I know some of mconley’s patches do, but perhaps there’s an easy way in hg/git to find out?

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/10c5f2b25f7d

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Beta.

Comment 37

[Andreas Tolfsen ❲:ato❳]

I’m not sure we care enough about Beta to put in the effort to rebase it.

Comment 38

[tb]

Hi.
I'm not sure this is the correct place to add this, but here goes.
I experience stalled tests when requesting the same URL twice which seems similar to the problem described in this ticket.

I use:
selenium-server-standalone-3.4.0.jar

Gecko driver: v0.16.1

Firefox (tried various versions): 53.0.3 (32bit), 54.0b11 (32bit), 55.0a1 (2017-05-29) (32-bit)

OS: Windows 7, 64bit

---------------

Snippet from the selenium log illustrating visit of same page:

13:13:22.088 INFO - Executing: [get: https://sitetesting.panorama9.com:443/portal#issues/snoozed])
13:13:22.316 INFO - Done: [get: https://sitetesting.panorama9.com:443/portal#issues/snoozed]
13:13:22.435 INFO - Executing: [find element: By.id: loading])
13:13:22.459 INFO - Done: [find element: By.id: loading]
13:13:22.511 INFO - Executing: [is displayed: 4 [[FirefoxDriver: firefox on ANY(34e77c0a-ea1c-4e1d-ad53-6b7492389516)] -> id: loading]])
13:13:22.622 INFO - Done: [is displayed: 4 [[FirefoxDriver: firefox on ANY (34e77c0a-ea1c-4e1d-ad53-6b7492389516)] -> id: loading]]
13:13:22.638 INFO - Executing: [get: https://sitetesting.panorama9.com:443/portal#issues/snoozed])

At the last line the test stalls until timeout.

This bug 1337464 (https://bugzilla.mozilla.org/show_bug.cgi?id=1337464) had me worried that this bug may have resurfaced?

Best regards, Thomas.

Comment 39

[tb]

Hi.
I'm not sure this is the correct place to add this, but here goes.
I experience stalled tests when requesting the same URL twice which seems similar to the problem described in this ticket.

I use:
selenium-server-standalone-3.4.0.jar

Gecko driver: v0.16.1

Firefox (tried various versions): 53.0.3 (32bit), 54.0b11 (32bit), 55.0a1 (2017-05-29) (32-bit)

OS: Windows 7, 64bit

---------------

Snippet from the selenium log illustrating visit of same page:

13:13:22.088 INFO - Executing: [get: https://sitetesting.panorama9.com:443/portal#issues/snoozed])
13:13:22.316 INFO - Done: [get: https://sitetesting.panorama9.com:443/portal#issues/snoozed]
13:13:22.435 INFO - Executing: [find element: By.id: loading])
13:13:22.459 INFO - Done: [find element: By.id: loading]
13:13:22.511 INFO - Executing: [is displayed: 4 [[FirefoxDriver: firefox on ANY(34e77c0a-ea1c-4e1d-ad53-6b7492389516)] -> id: loading]])
13:13:22.622 INFO - Done: [is displayed: 4 [[FirefoxDriver: firefox on ANY (34e77c0a-ea1c-4e1d-ad53-6b7492389516)] -> id: loading]]
13:13:22.638 INFO - Executing: [get: https://sitetesting.panorama9.com:443/portal#issues/snoozed])

At the last line the test stalls until timeout.

This bug 1337464 had me worried that this bug may have resurfaced?

Best regards, Thomas.

Comment 40

[Andreas Tolfsen ❲:ato❳]

(In reply to tb from comment #39)
> I'm not sure this is the correct place to add this, but here goes.
> I experience stalled tests when requesting the same URL twice which seems
> similar to the problem described in this ticket.

Please file a new bug and make sure you test on Firefox Nightly.  It would also be great if you included a trace-level log from geckodriver.