Marionette support for navigating between remote and non-remote pages with e10s enabled

RESOLVED FIXED in Firefox 39

Status

Testing
Marionette
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

(Blocks: 1 bug, {ateam-marionette-server})

unspecified
mozilla39
ateam-marionette-server
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox38 wontfix, firefox39 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
We were talking about this on IRC this morning. Navigating to an about: page with marionette or synthesizing key events that end us on an about: page do not work as expected with e10s turned on.
Relevant irc discussion:
<ahal> chmanchester: un-relatedly, doing marionette.navigate() to an about: page seems to fail with e10s enabled
<ahal> the navigate works, but then there's a timeout waiting for the page to load
<AutomatedTester> ahal: that navigate is because about: pages are shit
<AutomatedTester> ahal: port the readyState == interactive stuff from http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#1287 to http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#1166
<AutomatedTester> ahal: about: pages never fire the complete event
* AutomatedTester raises a shaking fist at About: pages
<AutomatedTester> ahal: we had this issue on B2G
<ahal> AutomatedTester: thanks, I'll look into it
<ahal> AutomatedTester: so if readyState == "complete" || readyState == "interactive" then sendOK ?
<AutomatedTester> ahal: readyState == Interactive only on about pages
<AutomatedTester> ahal: see http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#1288
<chmanchester> ahal: if you run into something like "<foo> is undefined" along the way, "<foo>AsCPOW" is effective sometimes
<ahal> AutomatedTester: yeah, I see, but I'm confused what you're telling me to do.. I don't understand how sending back an error will solve my problem
<AutomatedTester> ahal: if it's an about page and it hits interactive we should return an ok
<AutomatedTester> if it hits interactive and its not an about page we should wait for it to hit complete
<AutomatedTester> or timeout
<AutomatedTester> ahal: that make sense?
<ahal> AutomatedTester: ok, makes sense. Is url.startswith('about:') a reasonable test?
<AutomatedTester> I think so
I'll note that the marionette-server.js file AutomatedTester linked to is for processing navigate() in chrome scope. Since we navigate to about pages in content, I believe we need to update this function:
http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#1283
(Assignee)

Comment 3

3 years ago
A test (test_browser_window.py) that ends us on an about page without calling "navigate" leaves marionette in an apparently unusable state, which may be a different underlying issue.
Blocks: 984139
tracking-e10s: --- → +
No longer depends on: 1094246
Depends on: 1096571
(Assignee)

Comment 4

3 years ago
Created attachment 8520708 [details] [diff] [review]
Detect and handle switching from remote to non-remote pages in marionette

We're getting in trouble when navigating results in changing the remoteness of a tab because the old window is destroyed and restored. While the listener script is loaded into the new window, the registration of the new window with the marionette server isn't handled correclty.

I'm going to work on cleaning this up, but it works ok for the case of marionette calling "navigate" and passes tests locally. This doesn't address the case where synthesized keystrokes results in a context switching page load. I think that will be harder to deal with.
(Assignee)

Updated

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

Comment 5

3 years ago
> I'm going to work on cleaning this up, but it works ok for the case of
> marionette calling "navigate" and passes tests locally. This doesn't address
> the case where synthesized keystrokes results in a context switching page
> load. I think that will be harder to deal with.

I'm thinking a non-invasive approach here would be to add a check to switch_to_window for an active listener, and if we don't have one, load it.
(Assignee)

Comment 6

3 years ago
Created attachment 8522368 [details] [diff] [review]
Detect and handle switching from remote to non-remote pages and back in marionette

This is still a little magical, but passes the included test case with and without e10s.
(Assignee)

Updated

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

Updated

3 years ago
No longer depends on: 1096571
Summary: Marionette support for interacting with about: pages with e10s enabled → Marionette support for navigating between about: pages with e10s enabled
(Assignee)

Comment 7

3 years ago
Try looks ok (e10s off): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1c50a021ce52

This applies on top of the commits in bug 1095260 and will need to be modified if there's significant changes required before that lands.
Keywords: ateam-marionette-server
(Assignee)

Updated

3 years ago
Summary: Marionette support for navigating between about: pages with e10s enabled → Marionette support for navigating between remote and non-remote pages with e10s enabled
(Assignee)

Updated

3 years ago
Blocks: 1131878
(Assignee)

Comment 8

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a994a986ba45
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c725cabfe5da
(Assignee)

Comment 9

3 years ago
Cleaned up a bit more, this is about ready for review:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df3fbe830bdd
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c48ea7c8bf5
(Assignee)

Updated

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

Comment 10

3 years ago
(In reply to Chris Manchester [:chmanchester] from comment #9)
> Cleaned up a bit more, this is about ready for review:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=df3fbe830bdd
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c48ea7c8bf5

Not with that syntax error it's not. New try runs:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=36fe38461bc5
https://treeherder.mozilla.org/#/jobs?repo=try&revision=493548a1cbe7
(Assignee)

Comment 11

3 years ago
(In reply to Chris Manchester [:chmanchester] from comment #10)
> (In reply to Chris Manchester [:chmanchester] from comment #9)
> > Cleaned up a bit more, this is about ready for review:
> > 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=df3fbe830bdd
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c48ea7c8bf5
> 
> Not with that syntax error it's not. New try runs:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=36fe38461bc5
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=493548a1cbe7

New run with a fix for that win 8 failure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f56c837f7486
(Assignee)

Comment 12

3 years ago
Created attachment 8570761 [details]
MozReview Request: bz://1096488/chmanchester

/r/4459 - Bug 1096488 - Unskip marionette test navigating to non-remote pages.
/r/4461 - Bug 1096488 - Test that switching browser remoteness leaves marionette in a usable state.
/r/4463 - Bug 1096488 - Detect and handle switching from remote to non-remote pages and back in marionette.

Pull down these commits:

hg pull review -r 629d43b0410d321aa20397da4cc5a9276217fc59
(Assignee)

Comment 13

3 years ago
https://reviewboard.mozilla.org/r/4457/#review3645

::: testing/marionette/client/marionette/tests/unit/test_about_pages.py
(Diff revision 1)
> +    def is_remote(self):
> +        with self.marionette.using_context("chrome"):
> +            is_remote = self.marionette.execute_script("""
> +              return gBrowser.selectedBrowser.isRemoteBrowser;
> +            """)
> +        return is_remote

Not planning on commiting this part as it's likely to break as the browser is updated, this is here to make sure I'm testing what I think I am.

::: testing/marionette/client/marionette/tests/unit/test_window_handles.py
(Diff revision 1)
> +        # Window handles don't persist in cases of remoteness change.
> +        start_tab = self.marionette.current_window_handle

This is a follow up bug if window handles persisting across a session is a requirement.
(Assignee)

Comment 14

3 years ago
Comment on attachment 8570761 [details]
MozReview Request: bz://1096488/chmanchester

/r/4459 - Bug 1096488 - Unskip marionette test navigating to non-remote pages.
/r/4461 - Bug 1096488 - Test that switching browser remoteness leaves marionette in a usable state.
/r/4463 - Bug 1096488 - Detect and handle switching from remote to non-remote pages and back in marionette.

Pull down these commits:

hg pull review -r 629d43b0410d321aa20397da4cc5a9276217fc59
Attachment #8570761 - Flags: review?(jgriffin)
Attachment #8570761 - Flags: review?(jgriffin)
Comment on attachment 8570761 [details]
MozReview Request: bz://1096488/chmanchester

https://reviewboard.mozilla.org/r/4457/#review3731

::: testing/marionette/marionette-frame-manager.js
(Diff revision 1)
> +    messageManager.addWeakMessageListener("Marionette:cancelRequest", this.server);

This seems unnecessary as there is no cancelRequest handler in the server.

::: testing/marionette/marionette-server.js
(Diff revision 1)
> +    if (this._hasRemotenessChange === null) {

The significance of this._hasRemotenessChange === null (as opposed to false) is a little confusing; is the 'null' case for non-Firefox runtimes?

::: testing/marionette/marionette-listener.js
(Diff revision 1)
> +function cancelRequest(msg) {

param 'msg' never used

::: testing/marionette/marionette-server.js
(Diff revision 1)
> +      case "Marionette:listening":

A more descriptive name would be useful here; "waitingFor...", perhaps?

::: testing/marionette/marionette-server.js
(Diff revision 1)
> +  this.frameManager = new FrameManager(server); //We should have one FM per BO so that we can handle modals in each Browser

This line is duplicated from a few lines above.

Whew, this is a big, complicated patch!  I'm going to flag David also, to make sure I didn't miss anything.

I think it would be wise to get a try run against marionette-webapi on the emulator as well.  Even though they're perma-orange, I want to make sure we don't miss something that will break it worse than it already is.
Comment on attachment 8570761 [details]
MozReview Request: bz://1096488/chmanchester

This is complicated enough to warrant a second pair of eyes, I think.
Attachment #8570761 - Flags: review?(dburns)
(Assignee)

Comment 17

3 years ago
https://reviewboard.mozilla.org/r/4457/#review3735

> The significance of this._hasRemotenessChange === null (as opposed to false) is a little confusing; is the 'null' case for non-Firefox runtimes?

On B2G it shouldn't ever be anything but null and the rest of the checks aren't applicable. I'll see if I can tighten that up a bit.

Yes, this was a hard patch, I agree lots of review is warranted. I'll post a try run shortly. Thanks!
(Assignee)

Comment 18

3 years ago
https://reviewboard.mozilla.org/r/4457/#review3737

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3aad983af1
(Assignee)

Updated

3 years ago
Attachment #8570761 - Flags: review?(dburns) → review?(jgriffin)
(Assignee)

Comment 19

3 years ago
Comment on attachment 8570761 [details]
MozReview Request: bz://1096488/chmanchester

/r/4459 - Bug 1096488 - Unskip marionette test navigating to non-remote pages.
/r/4461 - Bug 1096488 - Test that switching browser remoteness leaves marionette in a usable state.
/r/4463 - Bug 1096488 - Detect and handle switching from remote to non-remote pages and back in marionette.

Pull down these commits:

hg pull review -r 861920afb4af6cb7e49b18969328027571a896ef
(Assignee)

Comment 20

3 years ago
Comment on attachment 8570761 [details]
MozReview Request: bz://1096488/chmanchester

Pushed updates from review comments, didn't mean to reset this.
Attachment #8570761 - Flags: review?(jgriffin) → review?(dburns)
(Assignee)

Comment 21

3 years ago
https://reviewboard.mozilla.org/r/4457/#review3751

Try shows the same 7 failures for webapi tests as inbound.
Attachment #8570761 - Flags: review?(dburns)
Comment on attachment 8570761 [details]
MozReview Request: bz://1096488/chmanchester

https://reviewboard.mozilla.org/r/4457/#review3799

::: testing/marionette/client/marionette/tests/unit/test_about_pages.py
(Diff revision 2)
> +            self.multi_process_browser = self.marionette.execute_script("""

Would this be more useful as a session capability? happy to not have it as a capability but just curious
Comment on attachment 8570761 [details]
MozReview Request: bz://1096488/chmanchester

https://reviewboard.mozilla.org/r/4457/#review3813

Ship It!
Attachment #8570761 - Flags: review+
(Assignee)

Comment 24

3 years ago
https://reviewboard.mozilla.org/r/4457/#review3815

> Would this be more useful as a session capability? happy to not have it as a capability but just curious

I guess I don't think it would be that useful because as I understand it, as soon as e10s ships finding a firefox that doesn't use remote tabs will be extremely rare.
(Assignee)

Comment 25

3 years ago
https://reviewboard.mozilla.org/r/4457/#review3919

> This is a follow up bug if window handles persisting across a session is a requirement.

https://bugzilla.mozilla.org/show_bug.cgi?id=1140237
(Assignee)

Comment 26

3 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9e174f63f46f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d6956145538b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bca6e36655a4
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1131860
https://hg.mozilla.org/mozilla-central/rev/9e174f63f46f
https://hg.mozilla.org/mozilla-central/rev/d6956145538b
https://hg.mozilla.org/mozilla-central/rev/bca6e36655a4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
We kinda would like to have this support for Firefox 38 to be able to run our Firefox UI tests. Would it be possible to backport this patch?
status-firefox-esr38: --- → affected
Flags: needinfo?(dburns)
Flags: needinfo?(cmanchester)
(Assignee)

Comment 30

3 years ago
(In reply to Henrik Skupin (:whimboo) from comment #29)
> We kinda would like to have this support for Firefox 38 to be able to run
> our Firefox UI tests. Would it be possible to backport this patch?

This needs bug 1125377 and its dependent to work. There was a gecko dependency whose non-e10s component was uplifted in bug 1143623. If we want to test gecko 38 with e10s enabled (we probably don't), we'll need another part of bug 1077168 uplifted, as :mconley explains in https://bugzilla.mozilla.org/show_bug.cgi?id=1077168#c36
(Assignee)

Comment 31

3 years ago
(In reply to Chris Manchester [:chmanchester] from comment #30)
> (In reply to Henrik Skupin (:whimboo) from comment #29)
> > We kinda would like to have this support for Firefox 38 to be able to run
> > our Firefox UI tests. Would it be possible to backport this patch?
> 
> This needs bug 1125377 and its dependent to work. There was a gecko
> dependency whose non-e10s component was uplifted in bug 1143623. If we want
> to test gecko 38 with e10s enabled (we probably don't), we'll need another
> part of bug 1077168 uplifted, as :mconley explains in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1077168#c36

Actually, if we don't need to test firefox 38 with e10s, we don't need to uplift this bug at all.
(Assignee)

Updated

3 years ago
Flags: needinfo?(cmanchester)
We don't want to run the tests with e10s enabled for Firefox 38. This will first happen with a beta or release once it is officially enabled. Given that this will not happen for Firefox 38 and its ESR version, we can safely skip the backport request here then.
status-firefox-esr38: affected → ---
Flags: needinfo?(dburns)
(Assignee)

Updated

3 years ago
Blocks: 1122862
status-firefox38: --- → wontfix
Depends on: 1169798
(Assignee)

Comment 33

3 years ago
Comment on attachment 8570761 [details]
MozReview Request: bz://1096488/chmanchester
Attachment #8570761 - Attachment is obsolete: true
Attachment #8618585 - Flags: review+
Attachment #8618586 - Flags: review+
Attachment #8618587 - Flags: review+
(Assignee)

Comment 34

3 years ago
Created attachment 8618585 [details]
MozReview Request: Bug 1096488 - Unskip marionette test navigating to non-remote pages.
(Assignee)

Comment 35

3 years ago
Created attachment 8618586 [details]
MozReview Request: Bug 1096488 - Test that switching browser remoteness leaves marionette in a usable state.
(Assignee)

Comment 36

3 years ago
Created attachment 8618587 [details]
MozReview Request: Bug 1096488 - Detect and handle switching from remote to non-remote pages and back in marionette.
You need to log in before you can comment on or make changes to this bug.