Allow multiple sequential webdriver sessions

[asajeffrey]

Thank you for contributing to Servo! Please replace each [ ] by [X] when the step is complete, and replace __ with appropriate data:

• ./mach build -d does not report any errors
• ./mach test-tidy --faster does not report any errors
• These changes fix #11253

Either:

• There are tests for these changes OR
• These changes do not require tests because the wdspec tests are in wpt/webdriver

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @asajeffrey: components/webdriver_server/lib.rs
• @jgraham: components/webdriver_server/lib.rs

[asajeffrey]

r? @jgraham

[asajeffrey]

@jgraham?

[jgraham]

The issue here is that I can't really tell if this is the desired behaviour to align with other implementations. @andreastt might have some insight.

[asajeffrey]

@jgraham this was my reading of the spec, but I don't know what the other implementations do.

[asajeffrey]

We either should land this or rewrite the servo wdspec driver in wptrunner to start up a fresh servo instance each time. At the moment we can only run 1 wdspec test!

[jgraham]

I think making the change in wptrunner is the right thing to do in the short term.

Sorry, I didn't realise you were so blocked on this.

[asajeffrey]

@jgraham ok I'll have a look at rewriting wptrunner to spawn a separate servo instance per test.

[andreastt]

@asajeffrey Sorry it took me so long to get around to this, but I was at a conference all of last week.

This patch looks mostly fine to me, except that we should navigate on creating- and deleting sessions.

Previously, asajeffrey (Alan Jeffrey) wrote…

@jgraham ok I'll have a look at rewriting wptrunner to spawn a separate servo instance per test.

---

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.

---

components/webdriver_server/lib.rs, line 286 [r1] (raw file):

                    session.id.to_string(),
                    Json::Object(capabilities))));
            try!(self.handle_get(&GetParameters { url: String::from("about:blank") }));

Creating a new session should not change the current document.

---

components/webdriver_server/lib.rs, line 296 [r1] (raw file):

    fn handle_delete_session(&mut self) -> WebDriverResult<WebDriverResponse> {
        try!(self.handle_get(&GetParameters { url: String::from("about:blank") }));

I don’t think you should change the current document when the session is deleted. The browser should be left in the state it was in.

In theory deleting a session should cause the browser process to quit and I can’t see this is handled here, but is probably outside the scope of this patch.

---

Comments from Reviewable

[andreastt]

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[asajeffrey]

@andreastt this PR is to allow one servo instance to handle multiple sequential sessions, without quitting when the session is closed. The current code base quits the servo instance when the session is terminated, if that's the behaviour that's desired we can just close this PR.

[andreastt]

@asajeffrey Yes, that is the desired behaviour and it’s a bug that Marionette/Firefox does not do this right now when calling Delete Session.

(I just pointed this out in mozilla/geckodriver#70 (comment) too.)

[asajeffrey]

Closing this PR in favour of fixing the servo wdspec driver.