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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 1

[Andrew Halberstadt [:ahal]]

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

Comment 2

[Andrew Halberstadt [:ahal]]

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

Comment 3

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

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

Comment 5

[Chris Manchester (limited bugmail, email directly)]

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

Comment 6

[Chris Manchester (limited bugmail, email directly)]

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

Comment 7

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a994a986ba45
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c725cabfe5da

Comment 9

[Chris Manchester (limited bugmail, email directly)]

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

Comment 10

[Chris Manchester (limited bugmail, email directly)]

(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

Comment 11

[Chris Manchester (limited bugmail, email directly)]

(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

Comment 12

[Chris Manchester (limited bugmail, email directly)]

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

Comment 13

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 14

[Chris Manchester (limited bugmail, email directly)]

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

Comment 15

[Jonathan Griffin (: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 16

[Jonathan Griffin (:jgriffin)]

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

This is complicated enough to warrant a second pair of eyes, I think.

Comment 17

[Chris Manchester (limited bugmail, email directly)]

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!

Comment 18

[Chris Manchester (limited bugmail, email directly)]

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

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

Comment 19

[Chris Manchester (limited bugmail, email directly)]

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

Comment 20

[Chris Manchester (limited bugmail, email directly)]

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

Pushed updates from review comments, didn't mean to reset this.

Comment 21

[Chris Manchester (limited bugmail, email directly)]

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

Try shows the same 7 failures for webapi tests as inbound.

Comment 22

[David Burns :automatedtester]

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 23

[David Burns :automatedtester]

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

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

Ship It!

Comment 24

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 25

[Chris Manchester (limited bugmail, email directly)]

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

Comment 26

[Chris Manchester (limited bugmail, email directly)]

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

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e174f63f46f
https://hg.mozilla.org/mozilla-central/rev/d6956145538b
https://hg.mozilla.org/mozilla-central/rev/bca6e36655a4

Comment 29

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

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?

Comment 30

[Chris Manchester (limited bugmail, email directly)]

(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

Comment 31

[Chris Manchester (limited bugmail, email directly)]

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

Comment 32

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

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.

Comment 34

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1096488 - Unskip marionette test navigating to non-remote pages.

Comment 35

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1096488 - Test that switching browser remoteness leaves marionette in a usable state.

Comment 36

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1096488 - Detect and handle switching from remote to non-remote pages and back in marionette.