Race condition when dismissing a user prompt in GeckoView from Marionette
Categories
(GeckoView :: General, defect)
Tracking
(firefox99 unaffected, firefox100 fixed, firefox101 fixed)
Tracking | Status | |
---|---|---|
firefox99 | --- | unaffected |
firefox100 | --- | fixed |
firefox101 | --- | fixed |
People
(Reporter: whimboo, Assigned: olivia)
References
Details
(Whiteboard: [geckoview:m101] )
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
diannaS
:
approval-mozilla-beta+
|
Details |
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
ondismiss
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 beforeGeckoView:Prompt:Dismiss
completed, it could giveNone
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 aFalse
for dismissing aconfirm
. I then added aSleep(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 fordismiss
, with the note that right now we calldismiss
insideconfirm
too, maybe we can have a_dismissUi
or something like that that does whatdismiss
does today? and then havedismiss() { // 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.
Reporter | ||
Updated•3 years ago
|
Reporter | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
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.
Assignee | ||
Updated•3 years ago
|
Comment 3•3 years ago
|
||
bugherder |
Comment 5•3 years ago
|
||
Is this Marionette issue causing GeckoView test problems on Beta 100?
Assignee | ||
Comment 6•3 years ago
|
||
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.
Assignee | ||
Comment 7•3 years ago
|
||
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•3 years ago
|
||
Comment on attachment 9270498 [details]
Bug 1762543 - Race Condition When Dismissing a GeckoView Prompt in Marionette
Approved for 100.0b4
Comment 9•3 years ago
|
||
bugherder uplift |
Description
•