Closed Bug 960931 Opened 6 years ago Closed 3 years ago

Marionette should fail in a more obvious way if we try to start more than one session

Categories

(Testing :: Marionette, defect)

x86_64
Linux
defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: wlach, Assigned: automatedtester)

Details

(Keywords: pi-marionette-client, pi-marionette-server)

Attachments

(2 files)

I just spent quite a few hours debugging an error related to accidentally creating multiple simultaneous marionette sessions in bug 960063 (without realizing that was what was actually happening). We should generate a noisy assertion at the gecko layer if the user tries to create a new session if one is there already, instead of just silently hanging waiting for the existing one to close.
Summary: Marionette should fail in a more obvious way if we start more than one simultaneous session → Marionette should fail in a more obvious way if we start more than one session
Summary: Marionette should fail in a more obvious way if we start more than one session → Marionette should fail in a more obvious way if we try to start more than one session
Is it meant to be possible to have multiple sessions? Could start_session return the active session if there is one, rather than throwing?
(In reply to Dave Hunt (:davehunt) from comment #1)
> Is it meant to be possible to have multiple sessions? Could start_session
> return the active session if there is one, rather than throwing?

No, it is not meant to be possible as I understand it.

I think having more than one marionette object accessing the session indicates a problem with the code which could cause race conditions or other problems, especially in a multi-threading application. I think we're best off throwing an exception/assertion in this case.
What we can do is send back a 'A Marionette session is already active' message from the server when someone tries to make another session. What do you two think?
That would work for me.
We create the variables with what is returned from the session creation
but never do anything with them.

Review commit: https://reviewboard.mozilla.org/r/55124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55124/
There is actually on technical reason Marionette can’t support more than one concurrent session.  In fact, the new dispatching technique I put in place about a year ago ensures it can handle >1 command requests at the same time without confusing or messing up the response sequence.
Assignee: nobody → dburns
Status: NEW → ASSIGNED
Comment on attachment 8756375 [details]
MozReview Request: Bug 960931: Throw SessionNotCreatedError when requesting 2nd Active Session r?ato

https://reviewboard.mozilla.org/r/55122/#review52170

::: testing/marionette/driver.js:209
(Diff revision 1)
>   *     JSON serialisable object to send to the listener.
>   * @param {number=} cmdId
>   *     Command ID to ensure synchronisity.
>   */
>  GeckoDriver.prototype.sendAsync = function(name, msg, cmdId) {
> +  logger.info(`sendAsync ${this.sessionId}`)

Debug?

::: testing/marionette/driver.js:314
(Diff revision 1)
>   *     Window whose browser we need to access.
>   * @param {boolean=false} isNewSession
>   *     True if this is the first time we're talking to this browser.
>   */
>  GeckoDriver.prototype.startBrowser = function(win, isNewSession=false) {
> +  logger.info(`startBrowser ${this.sessionId}`)

Debug?

::: testing/marionette/driver.js:474
(Diff revision 1)
>  };
>  
>  /** Create a new session. */
>  GeckoDriver.prototype.newSession = function*(cmd, resp) {
> +  if (this.sessionId) {
> +    throw new SessionNotCreatedError("Maximum number of active sessions.")

Remove . and add ;

::: testing/marionette/harness/marionette/tests/unit/test_session.py:46
(Diff revision 1)
>          caps = self.marionette.start_session(session_id="ILoveCheese")
>  
>          self.assertEqual(self.marionette.session_id, "ILoveCheese")
>          self.assertTrue(isinstance(self.marionette.session_id, unicode))
> +
> +    def test_we_only_support_one_active_session_at_a_time(self):

`test_maximum_allowed_sessions` is more aligned with the spec prose.

::: testing/marionette/harness/marionette/tests/unit/test_session.py:48
(Diff revision 1)
>          self.assertEqual(self.marionette.session_id, "ILoveCheese")
>          self.assertTrue(isinstance(self.marionette.session_id, unicode))
> +
> +    def test_we_only_support_one_active_session_at_a_time(self):
> +        self.marionette.start_session()
> +        self.assertTrue(isinstance(self.marionette.session_id, unicode))

We should have a different test for whether `self.marionette.session_id` is the correct type.

The right test here, I think, is if it not `None`.
Attachment #8756375 - Flags: review?(ato) → review+
Comment on attachment 8756376 [details]
MozReview Request: Bug 960931: Remove unused variables from tests r?ato

https://reviewboard.mozilla.org/r/55124/#review52172
Attachment #8756376 - Flags: review?(ato) → review+
https://hg.mozilla.org/mozilla-central/rev/050485808a59
https://hg.mozilla.org/mozilla-central/rev/8e7bb0f5c506
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.