Closed Bug 1612831 Opened 1 year ago Closed 6 months ago

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

Categories

(Testing :: Marionette, task, P1)

Version 3
task

Tracking

(Fission Milestone:M6c, firefox82 fixed)

RESOLVED FIXED
82 Branch
Fission Milestone M6c
Tracking Status
firefox82 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Depends on 1 open bug, Blocks 5 open bugs)

Details

(Whiteboard: [marionette-fission-mvp][complex])

Attachments

(8 files)

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.

See Also: → 1612538
Blocks: 1638365
Depends on: 1519335
Fission Milestone: --- → M6b
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P3 → P1
Blocks: 1450876

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

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

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.

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

Blocks: 1648444
Depends on: 1650132

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.

Blocks: 1625410
Depends on: 1651297
Blocks: 1611857
Attachment #9158459 - Attachment description: Bug 1612831 - [marionette] Add validation check for url argument. → Bug 1612831 - [marionette] Move validation check for url argument to parent process.
Keywords: leave-open
Attachment #9158459 - Attachment description: Bug 1612831 - [marionette] Move validation check for url argument to parent process. → Bug 1612831 - [marionette] Add validation check for url argument.
Pushed by hskupin@mozilla.com:
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

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

Flags: needinfo?(hskupin)
Pushed by hskupin@mozilla.com:
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

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

Flags: needinfo?(hskupin)

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

Flags: needinfo?(hskupin)
Pushed by hskupin@mozilla.com:
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

== 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

Depends on: 1658696
Whiteboard: [marionette-fission-mvp]
Blocks: 1580699
Blocks: 1660332

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.

Fission Milestone: M6b → M6c

(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.

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. :)

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

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.

Blocks: 1650132
No longer depends on: 1650132
Blocks: 1664165
No longer blocks: marionette-actor
Blocks: 1664285
Depends on: 1664426
Blocks: 1568099

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

Whiteboard: [marionette-fission-mvp] → [marionette-fission-mvp][complex]
No longer blocks: 1580699
Blocks: 1580699
Pushed by hskupin@mozilla.com:
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

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.

This is finally fixed.

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

== 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

Regressions: 1667377
Regressions: 1666755
Regressions: 1672758
Regressions: 1674158
You need to log in before you can comment on or make changes to this bug.