1762543 - Race condition when dismissing a user prompt in GeckoView from Marionette

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

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

With bug 1712414 fixed we are now able to handle basic alert, prompt and confirm user prompts via Marionette in GeckoView applications. But a couple of tests are failing and it's all manifesting around dismissing the prompt.

Agi already had a look and this is what he found out on bug 1762123:

(In reply to Agi Sferro | :agi | [slow ni? rn sorry] | ⏰ PST | he/him from bug 1762123 comment #2)

(In reply to Olivia Hall from comment #1)

Hi Henrik, thanks for looking at this. I think there could potentially be a situation where the value isn't returned in time for the test to pickup. GeckoViewPrompter on dismiss sends a message back to the Java side to close the prompt, which will then cause a return value. I think if the check of the value occurred before GeckoView:Prompt:Dismiss completed, it could give None because the prompter would still be waiting for the prompt to finish still.

Agi, does that seem like it could be the case or do you think it could be something else? Should we complete the callback in the JavaScript prompter like we do for accept? When I ran the test locally without any changes, it seemed to complete in time and returned as expected and gave a False for dismissing a confirm. I then added a Sleep(1000) on the Java side to test and was then able to replicate the test failure.

I think you have it right, we do need to either wait for the UI to tell us that it's done dismissing or calling the callback ourselves. Because we call the callback ourselves in confirm I would do the same for dismiss, with the note that right now we call dismiss inside confirm too, maybe we can have a _dismissUi or something like that that does what dismiss does today? and then have

dismiss() {
    // notify the app to hide the prompt
    this._dismissUi();
    this.callback(null);
}

this should work

Olivia could you work on this? Note that tests on Android are running in CI now and should soon be merged to mozilla-central.

Comment 1

[Olivia Hall [:olivia]]

Attachment: Bug 1762543 - Race Condition When Dismissing a GeckoView Prompt in Marionette

Added _dismissUi() to GeckoViewPrompter.jsm to send the request to
dismiss the prompt UI. Updated dismiss() to answer the callback to
close the prompt directly and seperatly dismiss the prompt UI.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9de425089053
Race Condition When Dismissing a GeckoView Prompt in Marionette r=geckoview-reviewers,agi,webdriver-reviewers,whimboo

Comment 3

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/9de425089053

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Can we nominate this for Beta uplift please?

Comment 5

[Chris Peterson [:cpeterson]]

Is this Marionette issue causing GeckoView test problems on Beta 100?

Comment 6

[Olivia Hall [:olivia]]

From what I understand, the Wdspec tests were first enabled for Android in 100 and this issue was causing some of those newly enabled tests to fail (so they were then set to be ignored for testing) on 1749444. This patch fixes a couple of the tests that were failing due to a race condition when dismissing and re-enables them.

Comment 7

[Olivia Hall [:olivia]]

Comment on attachment 9270498 [details]
Bug 1762543 - Race Condition When Dismissing a GeckoView Prompt in Marionette

Beta/Release Uplift Approval Request

• User impact if declined: No user impact is expected because these changed methods are only called during testing.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): These methods for accepting/dismissing prompts are currently only used during testing, so this has lower risk. The inherit risk is this class is called when opening prompts in Fenix.
• String changes made/needed: None

Comment 8

[Dianna Smith [:diannaS]]

Comment on attachment 9270498 [details]
Bug 1762543 - Race Condition When Dismissing a GeckoView Prompt in Marionette

Approved for 100.0b4

Comment 9

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8bd417d3fe32