Queue PromptRequest instead of override them #8989

Amejia481 · 2020-11-16T22:25:07Z

When a new prompt request is created we are overwritten the existing one, that could cause some issues as we only are allowing one prompt to be visible at the time, some pages could create other prompt requests without the first one be proceeded and break some functionality for some pages. Below an example:

<!DOCTYPE html>
<html>
<body>

<p>Click the button to wait 3 seconds, then alert "Hello".</p>
<p>After clicking away the alert box, an new alert box will appear in 3 seconds. This goes on forever...</p>
<input type="file" onclick="myFunction()">Try it</button>

<script>
function myFunction() {
  setInterval(function(){ alert("Hello"); }, 2000);
}
</script>

</body>
</html>

┆Issue is synchronized with this Jira Task

This try-catch was added in mozilla-mobile#6687 as an effort to better individualize prompt cast exceptions in our crash reporting platforms to better asses their frequency and hopefully get more data to debug and fix the underlying issue. The original issue is resolved in the first commit of this patch so there is no need to keep the try-catch.

Following crash reports it was seen that it is possible for multiple prompts to be shown at the same time with an edgecase being that one prompt request comes after the user interacted with a previous prompt but before the consume call completing in AC / GV time at which this code will try to use the new prompt, not the one the user interacted with. Having support for multiple prompt requests in ContentState and tightly coupling a PromptDialogFragment with it's PromptRequest ensures any action consuming a PromptDialogFragment will always consume the PromptRequest for which that dialog was shown irrespective of the number of prompts or which is currently shown on top.

This try-catch was added in mozilla-mobile#6687 as an effort to better individualize prompt cast exceptions in our crash reporting platforms to better asses their frequency and hopefully get more data to debug and fix the underlying issue. The original issue is resolved in the first commit of this patch so there is no need to keep the try-catch.

Mugurell · 2021-05-20T13:48:30Z

Talked with Arturo a bit more and looked more into the code from PromptFeature.
I was thinking of just having to keep a stack of PromptRequests but saw that we need these mapped to a stack of PromptDialogs (there are also two Views (as opposed to Dialogs) to display 2 specific PromptRequests).
Then this are to be updated - added to / removed from in multiple places in PromptFeature.
Working on this idea I found some issues

Related to how prompts are restored after the Activity was killed in the background
- [Bug] Prompts are dismissed when putting the app in background but recreated if "Don't keep activities" is enabled fenix#19481
- [Bug] Duplicated SaveLogin prompt if the Activity is killed in background fenix#19479
Probably issues seen in code
- We always set the last prompt as activePromptRequest but in the above onPromptRequested that will not always mean a new prompt shown to the user
- Here we dismiss the prompt only in specific situations but we always clear the activePrompt reference.
- In the same above code the prompt can be dismissed but the request is not removed from the session.

@Amejia481 It's hard to work around these and trying to fix them exceeds the initial scope. To me it seems one of those situations in which a feature needs to get back to the drawing board and rethought/rewritten to best fit the current needs.

Following crash reports it was seen that it is possible for multiple prompts to be shown at the same time with an edgecase being that one prompt request comes after the user interacted with a previous prompt but before the consume call completing in AC / GV time at which this code will try to use the new prompt, not the one the user interacted with. Having support for multiple prompt requests in ContentState and tightly coupling a PromptDialogFragment with it's PromptRequest ensures any action consuming a PromptDialogFragment will always consume the PromptRequest for which that dialog was shown irrespective of the number of prompts or which is currently shown on top.

This try-catch was added in mozilla-mobile#6687 as an effort to better individualize prompt cast exceptions in our crash reporting platforms to better asses their frequency and hopefully get more data to debug and fix the underlying issue. The original issue is resolved in the first commit of this patch so there is no need to keep the try-catch.

Following crash reports it was seen that it is possible for multiple prompts to be shown at the same time with an edgecase being that one prompt request comes after the user interacted with a previous prompt but before the consume call completing in AC / GV time at which this code will try to use the new prompt, not the one the user interacted with. Having support for multiple prompt requests in ContentState and tightly coupling a PromptDialogFragment with it's PromptRequest ensures any action consuming a PromptDialogFragment will always consume the PromptRequest for which that dialog was shown irrespective of the number of prompts or which is currently shown on top.

This try-catch was added in mozilla-mobile#6687 as an effort to better individualize prompt cast exceptions in our crash reporting platforms to better asses their frequency and hopefully get more data to debug and fix the underlying issue. The original issue is resolved in the first commit of this patch so there is no need to keep the try-catch.

Following crash reports it was seen that it is possible for multiple prompts to be shown at the same time with an edgecase being that one prompt request comes after the user interacted with a previous prompt but before the consume call completing in AC / GV time at which this code will try to use the new prompt, not the one the user interacted with. Having support for multiple prompt requests in ContentState and tightly coupling a PromptDialogFragment with it's PromptRequest ensures any action consuming a PromptDialogFragment will always consume the PromptRequest for which that dialog was shown irrespective of the number of prompts or which is currently shown on top.

This try-catch was added in mozilla-mobile#6687 as an effort to better individualize prompt cast exceptions in our crash reporting platforms to better asses their frequency and hopefully get more data to debug and fix the underlying issue. The original issue is resolved in the first commit of this patch so there is no need to keep the try-catch.

Following crash reports it was seen that it is possible for multiple prompts to be shown at the same time with an edgecase being that one prompt request comes after the user interacted with a previous prompt but before the consume call completing in AC / GV time at which this code will try to use the new prompt, not the one the user interacted with. Having support for multiple prompt requests in ContentState and tightly coupling a PromptDialogFragment with it's PromptRequest ensures any action consuming a PromptDialogFragment will always consume the PromptRequest for which that dialog was shown irrespective of the number of prompts or which is currently shown on top.

This try-catch was added in #6687 as an effort to better individualize prompt cast exceptions in our crash reporting platforms to better asses their frequency and hopefully get more data to debug and fix the underlying issue. The original issue is resolved in the first commit of this patch so there is no need to keep the try-catch.

Following crash reports it was seen that it is possible for multiple prompts to be shown at the same time with an edgecase being that one prompt request comes after the user interacted with a previous prompt but before the consume call completing in AC / GV time at which this code will try to use the new prompt, not the one the user interacted with. Having support for multiple prompt requests in ContentState and tightly coupling a PromptDialogFragment with it's PromptRequest ensures any action consuming a PromptDialogFragment will always consume the PromptRequest for which that dialog was shown irrespective of the number of prompts or which is currently shown on top.

This try-catch was added in mozilla-mobile#6687 as an effort to better individualize prompt cast exceptions in our crash reporting platforms to better asses their frequency and hopefully get more data to debug and fix the underlying issue. The original issue is resolved in the first commit of this patch so there is no need to keep the try-catch.

Amejia481 added 🐞 bug Something isn't working <prompts> Component: feature-prompts labels Nov 16, 2020

Amejia481 self-assigned this Nov 16, 2020

Amejia481 mentioned this issue Apr 29, 2021

[crash] IllegalArgumentException in SaveLoginDialogFragment PromptFeature mozilla-mobile/fenix#18877

Closed

Mugurell added a commit to Mugurell/android-components that referenced this issue May 17, 2021

For mozilla-mobile#8989 - Support multiple prompts in the same Session

835ce7f

Mugurell added a commit to Mugurell/android-components that referenced this issue May 17, 2021

For mozilla-mobile#8989 - Support multiple prompts in the same Session

3a6a2c3

Mugurell added a commit to Mugurell/android-components that referenced this issue May 18, 2021

For mozilla-mobile#8989 - Support multiple prompts in the same Session

d4ffdd2

Mugurell mentioned this issue May 24, 2021

PromptFeature refactoring #10323

Closed

Amejia481 mentioned this issue Jun 2, 2021

Issue #10396 - Quick cleanup of the PromptFeature #10397

Closed

4 tasks

gabrielluong closed this as completed Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Queue PromptRequest instead of override them #8989

Queue PromptRequest instead of override them #8989

Amejia481 commented Nov 16, 2020 •

edited by data-sync-user

Loading

Mugurell commented May 20, 2021

Queue PromptRequest instead of override them #8989

Queue PromptRequest instead of override them #8989

Comments

Amejia481 commented Nov 16, 2020 • edited by data-sync-user Loading

Mugurell commented May 20, 2021

Amejia481 commented Nov 16, 2020 •

edited by data-sync-user

Loading