Closed Bug 1280300 Opened 8 years ago Closed 8 years ago

Marionette hangs when navigating to a fragment on the current page

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox49 wontfix, firefox50 fixed, firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: jmleyba, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1280300 - Support navigation by fragment;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details 
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.
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: webdriver
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: nobody → ato
Status: NEW → ASSIGNED
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 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 on attachment 8772487 [details]
Bug 1280300 - Lint navigation command;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65280/diff/2-3/
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 on attachment 8772487 [details]
Bug 1280300 - Lint navigation command;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65280/diff/3-4/
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.
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)
Attachment #8772487 - Attachment is obsolete: true
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 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?
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/
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.
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.
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 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 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 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 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 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+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39a2655e787d
Support navigation by fragment; r=automatedtester
https://hg.mozilla.org/mozilla-central/rev/39a2655e787d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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
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]
https://hg.mozilla.org/releases/mozilla-aurora/rev/10c5f2b25f7d
status-firefox50: --- → fixed
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-beta]
Needs rebasing for Beta.
status-firefox49: --- → affected
Flags: needinfo?(ato)
Whiteboard: [checkin-needed-beta]
I’m not sure we care enough about Beta to put in the effort to rebase it.
Flags: needinfo?(ato)
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.
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.
(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.
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: