1612831 - Move all page navigation related code into parent process (driver.js)

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, task, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Before the work on the JSWindowActor API can be started, all the page navigation related code should be moved out of the content processes. That was a clear statement by the Fission team when I was talking with them about Fission during the office hours.

So this affects pure navigation, reload, and back and forward navigation.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1612831 - [marionette] Rename WebDriver.get() to WebDriver.navigateTo().

get() is used a lot within Marionette for getters,
and as such makes it very hard to find the actual
"WebDriver:NavigateTo" implementation.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1612831 - [marionette] Add validation check for url argument.

Depends on D80617

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1612831 - [marionette] Switch to top-level frame before checking current URL.

The navigation command is always executed on the top-level
browsing context. As such the check for a possible load
event should not be done for the currently selected frame.

At the same time the switch to frame call can be moved
to the parent process.

Depends on D80618

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1612831 - [marionette] Do check for load event expected in parent process.

Depends on D80619

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1612831 - [marionette] Replace usages of then with await for navigation commands.

Depends on D80620

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1612831 - [marionette] Move navigation commands to parent process.

Depends on D80621

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Today I got the very first all-passing results from try (including the web-platform reftests):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d67b2f363922a21f1c16b9f5869494fbc480a76

There is still a bit of clean-up to do and I will also move more tests from the Marionette unit test suite to Wdspec. But maybe by the end of the week I will have something to review.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I noticed a kinda problematic leak with the above version of the patches. When using Promise.race() possibly registered event handlers in one or more of the combined promises, will not be removed because all promises except the first resolved/rejected one might never resolve/reject. That means they will linger around forever.

As such I had to refactor my patch so we now have a callback again, which is somewhat similar to what we already had for the listener.

Here a complete new try build for all the tests depending on Marionette:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4777b6d7f03ed1770b3d1d84bc2690bad1e79adf

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I talked with James, and he is fine to disable the usage of about:newtab for web platform tests. This should help to get the remaining tests green. I will do that early on Monday.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ffd32eb9077
[marionette] Rename WebDriver.get() to WebDriver.navigateTo(). r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/3abf197a31a4
[marionette] Add validation check for url argument. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/cc9da09e1c78
[marionette] Switch to top-level frame before checking current URL. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/5996678b6d08
[marionette] Do check for load event expected in parent process. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/5ffd34876862
[marionette] Replace usages of then with await for navigation commands. r=marionette-reviewers,maja_zf

Comment 11

[Cristina Coroiu [:ccoroiu]]

Backed out 5 changesets (bug 1612831) for xpcshell failures at testing/marionette/test/unit/test_navigate.js

Backout: https://hg.mozilla.org/integration/autoland/rev/5b830a35c421dbf950e866a2176d2e9e262f4003

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5ffd34876862a62bab9736ea9d7713e48a3f2568

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310286629&repo=autoland&lineNumber=3059

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I missed to update the xpcshell tests regarding the changes for isLoadEventExpected. I will push again with updated tests.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33e3f1393d8b
[marionette] Rename WebDriver.get() to WebDriver.navigateTo(). r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/2f63e799f5e7
[marionette] Add validation check for url argument. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/b066b09fe903
[marionette] Switch to top-level frame before checking current URL. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/5add96b1bf93
[marionette] Do check for load event expected in parent process. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/9df3d6a420fd
[marionette] Replace usages of then with await for navigation commands. r=marionette-reviewers,maja_zf

Comment 14

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on test_navigate.js

backout: https://hg.mozilla.org/integration/autoland/rev/732d50e7eca887fb66d0b27089fd107c3ed4d8db

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9df3d6a420fddd03d71d31cfa9803a1b8e04dfd3&group_state=expanded&selectedTaskRun=divjWPItRim5AiqkBrYKDg.0

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310320689&repo=autoland&lineNumber=2744

[task 2020-07-19T22:39:42.246Z] 22:39:42 INFO - TEST-START | testing/marionette/test/unit/test_navigate.js
[task 2020-07-19T22:39:42.316Z] 22:39:42 WARNING - TEST-UNEXPECTED-FAIL | testing/marionette/test/unit/test_navigate.js | xpcshell return code: 0
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - TEST-INFO took 69ms
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - >>>>>>>
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - running event loop
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - testing/marionette/test/unit/test_navigate.js | Starting test_isLoadEventExpected
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - (xpcshell/head.js) | test test_isLoadEventExpected pending (2)
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - TEST-PASS | testing/marionette/test/unit/test_navigate.js | test_isLoadEventExpected - [test_isLoadEventExpected : 15] ..
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - TypeError: URL constructor: undefined is not a valid URL. at /builds/worker/workspace/build/tests/xpcshell/tests/testing/marionette/test/unit/test_navigate.js:32
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - test_isLoadEventExpected@/builds/worker/workspace/build/tests/xpcshell/tests/testing/marionette/test/unit/test_navigate.js:32:56
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - _run_next_test@/builds/worker/workspace/build/tests/xpcshell/head.js:1650:11
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - run@/builds/worker/workspace/build/tests/xpcshell/head.js:777:9
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - _do_main@/builds/worker/workspace/build/tests/xpcshell/head.js:248:6
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - _execute_test@/builds/worker/workspace/build/tests/xpcshell/head.js:577:5
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - @-e:1:1
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - exiting test
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2020-07-19T22:39:42.316Z] 22:39:42 INFO - <<<<<<<

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Looks like the I missed to push the very last local change.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8434113fb0c8
[marionette] Rename WebDriver.get() to WebDriver.navigateTo(). r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/4947c770eee4
[marionette] Add validation check for url argument. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/ed0fba194392
[marionette] Switch to top-level frame before checking current URL. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/4010698fcf4f
[marionette] Do check for load event expected in parent process. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/d2685a47990b
[marionette] Replace usages of then with await for navigation commands. r=marionette-reviewers,maja_zf

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8434113fb0c8
https://hg.mozilla.org/mozilla-central/rev/4947c770eee4
https://hg.mozilla.org/mozilla-central/rev/ed0fba194392
https://hg.mozilla.org/mozilla-central/rev/4010698fcf4f
https://hg.mozilla.org/mozilla-central/rev/d2685a47990b

Comment 18

[Florin Strugariu [:Bebe]]

== Change summary for alert #26551 (as of Mon, 20 Jul 2020 19:36:25 GMT) ==

Improvements:

1% Base Content JS windows10-64-shippable-qr opt 3,563,213.33 -> 3,515,024.00
1% Base Content JS windows7-32-shippable opt 2,751,884.33 -> 2,725,702.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26551

Comment 19

[Neha Kochar [:neha]]

The main work has been done already to move to parent process. The remaining should not block the Nightly experiment launch so moving this meta to M6c.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Neha Kochar [:neha] from comment #19)

The main work has been done already to move to parent process. The remaining should not block the Nightly experiment launch so moving this meta to M6c.

I wouldn't say that the main work has been done, but lots of the code got moved to the parent process. Still the different kinds of navigation get triggered from within the framescript, and listening for page load events is also still part of the framescript. Let me see what I can do over the next days to maybe get this finished up.

As discussed on a different bug it might be better to make use of the web progress listener instead of the events. I will have to investigate that.

Comment 21

[Neha Kochar [:neha]]

Thank you, Henrik for the follow-up here with more info. And I appreciate your efforts to try to finish this as soon as possible. :)

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1612831 - [marionette] Remove no longer used handling of pending commands.

Now that all the navigation related code runs in the parent process
there is no need anymore for handling pending commands. This was
only necessary for navigation commands which could have caused
remoteness changes and as such new instances of the framescript.
In these cases the reply cannot be sent to the client unless the
command has been finished.

Depends on D80622

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Rewriting the tests via bug 1650132 will still take a little bit more time. As such I would like to get these changes landed first.

Waiting for the navigation to finish will still be handled via events for now, but I will file a follow-up patch to get it actually moved to the webprogress listener.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1612831 - [wpt] Update expected data for formerly erroring tests.

Formerly some tests were not running at all because the wptrunner
triggered an error when loading the test page. With the changes
to the navigation in Marionette this error is no longer present,
and as such allows the tests to actually run even when they still
fail or timeout.

Depends on D89717

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90d5c57d1e5c
[marionette] Move navigation commands to parent process. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/d2792a2b8d2e
[marionette] Remove no longer used handling of pending commands. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/4951412ca28e
[wpt] Update expected data for formerly erroring tests. r=jgraham,maja_zf,annevk

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I triggered some awsy jobs on autoland and as it looks like we will get another 4% base memory improvement in content for these latest changes.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Here the missing perfherder results:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=autoland&newRevision=4951412ca28ee33b50f4f8af722d01703feb7099&framework=4&selectedTimeRange=172800

Comment 28

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/90d5c57d1e5c
https://hg.mozilla.org/mozilla-central/rev/d2792a2b8d2e
https://hg.mozilla.org/mozilla-central/rev/4951412ca28e

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is finally fixed.

Comment 30

[Florin Strugariu [:Bebe]]

== Change summary for alert #26947 (as of Wed, 16 Sep 2020 10:22:24 GMT) ==

Improvements:

5% Base Content JS windows10-64-shippable-qr opt 3,240,643.33 -> 3,078,412.67
4% Base Content JS windows7-32-shippable opt 2,504,500.00 -> 2,392,080.00
4% Base Content JS windows10-64-shippable opt 3,248,993.07 -> 3,115,563.33
4% Base Content JS macosx1014-64-shippable opt 3,187,174.00 -> 3,058,380.00
4% Base Content JS linux1804-64-shippable-qr opt 3,181,584.00 -> 3,054,956.67
4% Base Content JS linux1804-64-shippable opt 3,181,578.00 -> 3,055,442.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26947

Comment 31

[Alexandru Ionescu (needinfo me) [:alexandrui]]

deleted