Chris Manchester (limited bugmail, email directly) Assignee
	Description • 10 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.

	Andrew Halberstadt [:ahal]
	Comment 1 • 10 years ago

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

	Andrew Halberstadt [:ahal]
	Comment 2 • 10 years ago

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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 3 • 10 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.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 4 • 10 years ago

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.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 5 • 10 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.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 6 • 10 years ago

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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 7 • 10 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.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 8 • 9 years ago

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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 9 • 9 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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 10 • 9 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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 11 • 9 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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 12 • 9 years ago

/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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 13 • 9 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.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 14 • 9 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

	Jonathan Griffin (:jgriffin)
	Comment 15 • 9 years ago

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.

	Jonathan Griffin (:jgriffin)
	Comment 16 • 9 years ago

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

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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 17 • 9 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!

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 18 • 9 years ago

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

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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 19 • 9 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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 20 • 9 years ago

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

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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 21 • 9 years ago

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

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

	David Burns :automatedtester
	Comment 22 • 9 years ago

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

	David Burns :automatedtester
	Comment 23 • 9 years ago

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

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

Ship It!

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 24 • 9 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.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 25 • 9 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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 26 • 9 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

	Carsten Book [:Tomcat]
	Comment 28 • 9 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 29 • 9 years ago

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?

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 30 • 9 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

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 31 • 9 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.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 32 • 9 years ago

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.