Marionette hangs when navigating to a fragment on the current page

RESOLVED FIXED in Firefox 50

Status

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jmleyba, Assigned: ato)

Tracking

(Blocks 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 wontfix, firefox50 fixed, firefox51 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

58 bytes, text/x-review-board-request
automatedtester
: review+
Details
(Reporter)

Description

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

Comment 2

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

Updated

3 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

3 years ago
Blocks: webdriver
Duplicate of this bug: 1287766
(Assignee)

Comment 6

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

Updated

3 years ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
(Assignee)

Comment 7

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

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

Comment 8

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

Comment 9

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

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

Comment 10

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

Comment 11

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

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

Comment 12

3 years ago
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 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.
Attachment #8772488 - Flags: review?(dburns) → review-
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.
(Assignee)

Comment 16

3 years ago
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/
Attachment #8772488 - Flags: review- → review?(dburns)
(Assignee)

Updated

3 years ago
Attachment #8772487 - Attachment is obsolete: true
(Assignee)

Comment 17

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

Comment 18

3 years ago
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/
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?
(Assignee)

Comment 20

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

Comment 21

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

Comment 22

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

Comment 23

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

Comment 24

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

Comment 25

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

Comment 26

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

Comment 27

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

Comment 28

3 years ago
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 on attachment 8772488 [details]
Bug 1280300 - Support navigation by fragment;

https://reviewboard.mozilla.org/r/65282/#review66354
Attachment #8772488 - Flags: review?(dburns) → review+

Comment 30

3 years ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39a2655e787d
Support navigation by fragment; r=automatedtester

Comment 31

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/39a2655e787d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 32

3 years ago
David, can you uplift this along with the relevant patches from mconley?
Flags: needinfo?(dburns)
(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.
No longer depends on: 1293107
(Assignee)

Comment 34

3 years ago
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?
Flags: needinfo?(dburns)
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]

Comment 35

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/10c5f2b25f7d
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-beta]
Needs rebasing for Beta.
Flags: needinfo?(ato)
Whiteboard: [checkin-needed-beta]
(Assignee)

Comment 37

3 years ago
I’m not sure we care enough about Beta to put in the effort to rebase it.
Flags: needinfo?(ato)

Comment 38

2 years ago
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

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

Comment 40

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