Closed Bug 1264259 Opened 8 years ago Closed 6 years ago

Add support for unhandledPromptBehavior capability

Categories

(Remote Protocol :: Marionette, defect, P1)

defect

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: ato, Assigned: whimboo)

References

(Blocks 1 open bug, )

Details

(Keywords: pi-marionette-server)

Attachments

(1 file)

The WebDriver specification has a concept of a user prompt handler (http://w3c.github.io/webdriver/webdriver-spec.html#dfn-handle-any-user-prompts) that Marionette currently does not implement.  We must implement it to be WebDriver compatible.

The idea is that you can set a handler state that will automatically run an action tied to said state when a user prompt is present.
Depends on: 1359004
Priority: -- → P1
David, why do you think it is dependent on bug 1311041?
Flags: needinfo?(dburns)
because its going to be simpler to implement when 1311041 is complete.
Flags: needinfo?(dburns)
Tab modal prompts are not tight to window ids but to the window it lives in. And those we track perfectly because the listener registers itself and as such we register for the modal dialog observer notification. So bug 1311041 is not a hard blocker, and currently I don't see what could make it simpler.
(In reply to Henrik Skupin (:whimboo) from comment #4)

> Tab modal prompts are not tight to window ids but to the window
> it lives in.  And those we track perfectly because the listener
> registers itself and as such we register for the modal dialog
> observer notification. So bug 1311041 is not a hard blocker, and
> currently I don't see what could make it simpler.

I guess I don’t really understand what you’re saying here, but
Marionette currently keeps exactly one reference to the last modal
dialogue to appear.  This means we don’t track modals on a per-tab
level, which we need to do because it is possible to switch to and
interact with other tabs.

In my opinion the current code we have for tracking user prompts is
far too complicated.  We could do away with keeping a reference to
the modal, and also stop tracking global modal dialogues because
they are not used when interacting with web content.

The implementation of a specification conforming user prompt handler
is not impossible without bug 1311041, but easier with it because
each browsing context would have a unique abstraction, as opposed to
the way curBrowser works right now.
Blocks: 1270585
Blocks: 1241679
Part of the conformance work we are doing this quarter, but not
actively worked on.
Priority: P1 → P2
Version: Version 3 → Trunk
Depends on: 1325738
Blocks: 1439995
Blocks: 1375451
Blocks: 1434872
Blocks: 1459118
Blocks: 1457244
Depends on: 1468185
As mentioned in the last meetings to get the various prompt behaviors implemented as the WebDriver specification refers to, we do not have to wait for the window tracking patch to be done.

I already have a local patch which is mostly done for Marionette, but needs a bit of work for wdspec tests, so that we can run the user prompt tests for all commands.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
No longer depends on: marionette-window-tracking
Priority: P2 → P1
Depends on: 1469752
While I wait on the dependency to be fixed I continue to work on this patch but won't bring it up for review. By doing so I noticed the following...

Overall we have 44 different commands we have to check for user prompt handling. This is a huge amount given that for each of them we have 6 tests (default, accept, accept and notify, dismiss, dismiss and notify, ignore). Between each test we restart Firefox which adds a certain amount of time to each of the 244 tests. When I only run the user prompt tests for `execute_script` it takes about 1 minute with a debug build (opt builds will be faster). Extrapolating this to all the commands would mean we have a duration of 44 minutes only for those user prompt tests!

I'm not satisfied with this at the moment given that we know there are some shutdown and startup hangs in Firefox once in a while. But also I'm worried of the overall duration of the Wd job. So if we want to see all the tests being executed we would have to start chunking the Wd spec job. I don't know what are usually the limits, and how long each chunk usually should be. Joel, do you have any recommendations?

Andreas, and David, please also clarify on the above. I assume chunking comes for free from the wpt-runner, and we only have to specify the configuration in taskcluster.
Flags: needinfo?(jmaher)
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
I have to correct... Not all 44 commands have that check. We have to clearly exclude all the alert commands and a couple others.
(In reply to Henrik Skupin (:whimboo) from comment #10)

> Overall we have 44 different commands we have to check for user
> prompt handling. This is a huge amount given that for each of them
> we have 6 tests (default, accept, accept and notify, dismiss,
> dismiss and notify, ignore).  Between each test we restart
> Firefox which adds a certain amount of time to each of the 244
> tests. When I only run the user prompt tests for `execute_script`
> it takes about 1 minute with a debug build (opt builds will be
> faster). Extrapolating this to all the commands would mean we have
> a duration of 44 minutes only for those user prompt tests!

We need to run the user prompt tests for each command that invokes
the user prompt handler.  There is no other way of testing that each
implementation is conforming.  It’s unfortunate that WebDriver needs
a new session each time we have to change the capabilities, but I
feel this especially impacts Firefox negatively because the startup
is so incredibly slow.

We might be able to employ “a cheat” to not restart Firefox when
deleting and creating the session like we do for the Mn tests.  I
haven’t thought carefully about this but it might be possible to
instruct geckodriver not to shut down the Firefox process through a
new extension capability.

As I’ve mentioned in the past there is limited value in running
wdspec tests against debug builds, but chunking seems to be the way
forward to ensure the tests complete in a reasonable timeframe.

> I assume chunking comes for free from the wpt-runner, and we only
> have to specify the configuration in taskcluster.

Chunking seems to be the way forward here to ensure the tests
complete in a timely manner.  I don’t know the details of the TC
chunking setup, but in theory there is nothing in WPT or in wdspec
that prevents chunking.
Flags: needinfo?(ato)
Depends on: 1470098
(In reply to Andreas Tolfsen 「:ato」 from comment #12)  

> We might be able to employ “a cheat” to not restart Firefox when
> deleting and creating the session like we do for the Mn tests.  I
> haven’t thought carefully about this but it might be possible to
> instruct geckodriver not to shut down the Firefox process through
> a new extension capability.

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1470120 to run
an experiment on this.  Who knows, we might make the tests a lot
faster, if only for development purposes.
I am happy to wait for the bug 147010 results but for the most part I think its fine to see about chunking these as wptrunner should handle it
Flags: needinfo?(dburns)
I say opt chunks should be 15-20 minutes, debug twice as long- more chunks!
Flags: needinfo?(jmaher)
Alright. So current Wd jobs take ~15min for opt, and ~30min for debug. It means that depending on the amount of extra time this patch adds, I will create chunks based on the total execution time. Thanks.
Blocks: 1473553
(In reply to Henrik Skupin (:whimboo) [away 07/08 - 07/22] from comment #16)
> Alright. So current Wd jobs take ~15min for opt, and ~30min for debug. It
> means that depending on the amount of extra time this patch adds, I will
> create chunks based on the total execution time. Thanks.

I thought about all this and I think it will be better when I just push this work out to a follow-up bug. This bug is about to add the unhandledPromptBehavior capability, which I got working. So adding all the remaining user prompt tests goes really out of scope for this bug.

Given that this is my last day before PTO I would like to maybe even see this landed today.
Blocks: 1469752
No longer depends on: 1469752
Blocks: 1473814
No longer blocks: 1473814
Summary: Implement user prompt handler → Add support for unhandledPromptBehavior capability
Blocks: 1473814
Comment on attachment 8990218 [details]
Bug 1264259 - [marionette] Add support for unhandledPromptBehavior capability.

https://reviewboard.mozilla.org/r/255244/#review262148


Code analysis found 8 defects in this patch:
 - 8 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/driver.js:3240
(Diff revision 1)
> +      break;
> +
> +    case UnhandledPromptBehavior.AcceptAndNotify:
> +      this.acceptDialog();
> +      throw new UnexpectedAlertOpenError(undefined, alertText);
> +      break;

Error: Unreachable code. [eslint: no-unreachable]

::: testing/marionette/driver.js:3249
(Diff revision 1)
> +      break;
> +
> +    case UnhandledPromptBehavior.DismissAndNotify:
> -    this.dismissDialog();
> +      this.dismissDialog();
> -    throw e;
> +      throw new UnexpectedAlertOpenError(null, alertText);
> +      break;

Error: Unreachable code. [eslint: no-unreachable]

::: testing/marionette/driver.js:3253
(Diff revision 1)
> -    throw e;
> +      throw new UnexpectedAlertOpenError(null, alertText);
> +      break;
> +
> +    case UnhandledPromptBehavior.Ignore:
> +      throw new UnexpectedAlertOpenError(null, alertText);
> +      break;

Error: Unreachable code. [eslint: no-unreachable]

::: testing/marionette/test/unit/test_capabilities.js:370
(Diff revision 1)
>  
>    run_next_test();
>  });
>  
> +add_test(function test_UnhandledPromptBehavior() {
> +  equal(session.UnhandledPromptBehavior.Accept, "accept");

Error: 'session' is not defined. [eslint: no-undef]

::: testing/marionette/test/unit/test_capabilities.js:371
(Diff revision 1)
>    run_next_test();
>  });
>  
> +add_test(function test_UnhandledPromptBehavior() {
> +  equal(session.UnhandledPromptBehavior.Accept, "accept");
> +  equal(session.UnhandledPromptBehavior.AcceptAndNotify, "accept and notify");

Error: 'session' is not defined. [eslint: no-undef]

::: testing/marionette/test/unit/test_capabilities.js:372
(Diff revision 1)
>  });
>  
> +add_test(function test_UnhandledPromptBehavior() {
> +  equal(session.UnhandledPromptBehavior.Accept, "accept");
> +  equal(session.UnhandledPromptBehavior.AcceptAndNotify, "accept and notify");
> +  equal(session.UnhandledPromptBehavior.Dismiss, "dismiss");

Error: 'session' is not defined. [eslint: no-undef]

::: testing/marionette/test/unit/test_capabilities.js:373
(Diff revision 1)
>  
> +add_test(function test_UnhandledPromptBehavior() {
> +  equal(session.UnhandledPromptBehavior.Accept, "accept");
> +  equal(session.UnhandledPromptBehavior.AcceptAndNotify, "accept and notify");
> +  equal(session.UnhandledPromptBehavior.Dismiss, "dismiss");
> +  equal(session.UnhandledPromptBehavior.DismissAndNotify, "dismiss and notify");

Error: 'session' is not defined. [eslint: no-undef]

::: testing/marionette/test/unit/test_capabilities.js:374
(Diff revision 1)
> +add_test(function test_UnhandledPromptBehavior() {
> +  equal(session.UnhandledPromptBehavior.Accept, "accept");
> +  equal(session.UnhandledPromptBehavior.AcceptAndNotify, "accept and notify");
> +  equal(session.UnhandledPromptBehavior.Dismiss, "dismiss");
> +  equal(session.UnhandledPromptBehavior.DismissAndNotify, "dismiss and notify");
> +  equal(session.UnhandledPromptBehavior.Ignore, "ignore");

Error: 'session' is not defined. [eslint: no-undef]
Attachment #8990218 - Flags: review?(ato)
Comment on attachment 8990218 [details]
Bug 1264259 - [marionette] Add support for unhandledPromptBehavior capability.

https://reviewboard.mozilla.org/r/255244/#review262190

This is pretty good, but I found a couple of omissions.  I don’t
think you will find any of my issues very surprising, so I would
say you’re fine to fix those up and land this without any need for
further review.

::: testing/marionette/capabilities.js:391
(Diff revision 2)
> +      ["acceptInsecureCerts", false],
>        ["browserName", appinfo.name],
>        ["browserVersion", appinfo.version],
>        ["platformName", getWebDriverPlatformName()],
>        ["platformVersion", Services.sysinfo.getProperty("version")],
>        ["pageLoadStrategy", PageLoadStrategy.Normal],
> -      ["acceptInsecureCerts", false],
> -      ["timeouts", new Timeouts()],
>        ["proxy", new Proxy()],
> +      ["timeouts", new Timeouts()],
> +      ["unhandledPromptBehavior", UnhandledPromptBehavior.DismissAndNotify],

This probably does not matter functionally, but these capabilities
are ordered as they are in the specification.  This has the benefit
that browserName, browserVersion, platformName, et al. will appear
first in the matched capabilities list.  I think this is worth
preserving.

::: testing/marionette/capabilities.js:399
(Diff revision 2)
>        ["pageLoadStrategy", PageLoadStrategy.Normal],
> -      ["acceptInsecureCerts", false],
> -      ["timeouts", new Timeouts()],
>        ["proxy", new Proxy()],
> +      ["timeouts", new Timeouts()],
> +      ["unhandledPromptBehavior", UnhandledPromptBehavior.DismissAndNotify],

This highlights a bug in the specification.  It says that the default
value should be null which it refers to as the default missing
state, but it makes more sense for it to be set to the "dismiss and
notify" state.

Currently this line is in violation of the specification because
it says the returned matched capabilities should have
unhandledPromptBehavior set to null.

I think the spec is wrong here as this behaviour deviates from how
we treat all other capabilities, so it’s probably sufficient to
file a spec bug on this.

::: testing/marionette/capabilities.js:500
(Diff revision 2)
> +          assert.string(v,
> +              pprint`Expected ${k} to be a string, got ${v}`);

I note that pageLoadStrategy above also checks for null, which I
think is probably wrong.  I will follow up on that later.

::: testing/marionette/driver.js:3252
(Diff revision 2)
> +
> +    case UnhandledPromptBehavior.Ignore:
> +      throw new UnexpectedAlertOpenError();
> +
> +    default:
> +      throw new UnknownError(`Unknown unhandledPromptBehavior "${behavior}"`);

TypeError is probably more appropriate since you can’t find the
desired variation on the enum.

Also I’m not sure about the need for a string here since we validate
the input.  If there’s a programming error that leads us here it’s
pretty clear what the problem is from the stacktrace.  Messages are
only really useful when we need to inform the user of something or
provide additional debug information.

::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:164
(Diff revision 2)
>          self.marionette.start_session(caps)
>          self.assertIn("timeouts", self.marionette.session_capabilities)
>          self.assertDictEqual(self.marionette.session_capabilities["timeouts"], timeouts)
>          self.assertDictEqual(self.marionette._send_message("getTimeouts"), timeouts)
> +
> +    def test_unhandled_prompt_behavior(self):

You will also want to test the default value.

::: testing/marionette/harness/marionette_harness/tests/unit/test_unhandled_prompt_behavior.py:23
(Diff revision 2)
> +        try:
> +            alert = self.marionette.switch_to_alert()
> +            alert.dismiss()
> +
> +            Wait(self.marionette).until(lambda _: not self.alert_present)
> +        except:

Be more specific about the exception so that we don’t ignore unwanted
error scenarios.  This should be a NoAlertPresentException IIRC.

::: testing/marionette/harness/marionette_harness/tests/unit/test_unhandled_prompt_behavior.py:98
(Diff revision 2)
> +    @parameterized("alert", "alert", None)
> +    @parameterized("confirm", "confirm", None)
> +    @parameterized("prompt", "prompt", None)
> +    def test_ignore(self, prompt_type, result):
> +        self.marionette.start_session({"unhandledPromptBehavior": "ignore"})
> +        self.perform_user_prompt_check(prompt_type, "foo {}".format(prompt_type), result,
> +                                       expected_close=False)

Also a test for the default behaviour.
Attachment #8990218 - Flags: review?(ato) → review+
Comment on attachment 8990218 [details]
Bug 1264259 - [marionette] Add support for unhandledPromptBehavior capability.

https://reviewboard.mozilla.org/r/255244/#review262190

> This probably does not matter functionally, but these capabilities
> are ordered as they are in the specification.  This has the benefit
> that browserName, browserVersion, platformName, et al. will appear
> first in the matched capabilities list.  I think this is worth
> preserving.

I find it strange that an internal data structure has to care how toJSON() operates. But anyway I did not revert this change but actually sorted correctly as listed by the Spec. Even the former order was not correct. With the upcoming patch we are aligned again.

> This highlights a bug in the specification.  It says that the default
> value should be null which it refers to as the default missing
> state, but it makes more sense for it to be set to the "dismiss and
> notify" state.
> 
> Currently this line is in violation of the specification because
> it says the returned matched capabilities should have
> unhandledPromptBehavior set to null.
> 
> I think the spec is wrong here as this behaviour deviates from how
> we treat all other capabilities, so it’s probably sufficient to
> file a spec bug on this.

Good catch. I didn't read it that way, but what you say makes sense and that we should update the spec. I filed https://github.com/w3c/webdriver/issues/1276.

> I note that pageLoadStrategy above also checks for null, which I
> think is probably wrong.  I will follow up on that later.

Thank you. That is indeed a bug and should be fixed on our end. When you follow-up please add a wdspec test for it.

> TypeError is probably more appropriate since you can’t find the
> desired variation on the enum.
> 
> Also I’m not sure about the need for a string here since we validate
> the input.  If there’s a programming error that leads us here it’s
> pretty clear what the problem is from the stacktrace.  Messages are
> only really useful when we need to inform the user of something or
> provide additional debug information.

I will change it to `TypeError`.

Regarding the second paragraph we add a message to all other TypeError instances too, so I will just follow that. Also the stacktrace will not necessary tell you which behavior is currently set. You could only assume it when going back in the log and check the response of the new session command. I will keep that to stay in sync with the rest of the file.

> You will also want to test the default value.

Ups. I forgot that!

> Be more specific about the exception so that we don’t ignore unwanted
> error scenarios.  This should be a NoAlertPresentException IIRC.

Yes, I will change.

> Also a test for the default behaviour.

The default is already covered with `test_default`. I will move the test to the bottom to make it more visible.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44861ed17925
[marionette] Add support for unhandledPromptBehavior capability. r=ato
https://hg.mozilla.org/mozilla-central/rev/44861ed17925
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
No longer blocks: 1375451
No longer blocks: 1241679
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: