1853107 - Tab freezes, in part the contents of an adjacent tab are displayed, in part the tab contents are completely black

Status: VERIFIED FIXED

(Fenix :: Tabs, defect, P2)

Description

[Denis Müller [:Webworkr]]

User Agent: Mozilla/5.0 (Android 13; Mobile; rv:109.0) Gecko/117.0 Firefox/117.0

Steps to reproduce:

Several tabs are open in parallel.

Actual results:

The current tab freezes, in some cases the content of an adjacent tab is displayed.

Expected results:

The tab should not freeze.

Comment 1

[Denis Müller [:Webworkr]]

This report is based on https://github.com/mozilla-mobile/fenix/issues/20748.

Comment 2

[Denis Müller [:Webworkr]]

As a workaround currently only helps to close the affected tab and then restore it via the corresponding function.

Comment 3

[Adina Petridean QA [:apetridean]]

Hi, thanks for reporting. I was not able to reproduce the issue described on Firefox 119.0a1 with Google Pixel 7 PRO (Android 14) and Motorola Moto G9 plus (Android 11).

In order to further look into the issue you're experiencing, could you please provide the following additional information?

Details regarding the Device used, OS, Android version;

Video recording of the issue;

Thanks!

Comment 4

[Denis Müller [:Webworkr]]

Samsung Galaxy A13 5G (SM-A136B)
Android 13

Nightly
119.0a1 (Build #2015974145), afe25e2fd9+
GV: 119.0a1-20230914093829
AS: 119.20230914050403

Comment 5

[Denis Müller [:Webworkr]]

A video could at best document the procedure. The error occurs only occasionally. I think it's difficult to miss this moment exactly in a video.

Comment 6

[Adina Petridean QA [:apetridean]]

Retested this issue with a Samsung Galaxy Z Fold 4 (Android 13) and a Samsung Galaxy S9 (Android 8) but couldn't reproduce it. As far as we understand, this issue occurs intermittently for the user.

Comment 7

[Denis Müller [:Webworkr]]

I adjusted the title. Maybe a little long, if necessary shorten.

There are a total of 3 variants as far as I remembered / correctly observed:

• Tab freezes
• Tab freezes and displays contents of an adjacent tab
• Tab freezes and gets black

Comment 8

[Denis Müller [:Webworkr]]

As far as we understand, this issue occurs intermittently for the user.

This is correct.

Comment 9

[Denis Müller [:Webworkr]]

The black screen often occurs after a successful login (e.g. webmailer).

Nightly
119.0a1 (Build #2015974529), 19eec1f919+
GV: 119.0a1-20230916091445
AS: 119.20230916050403

In Fennec, however, this does not occur as far as I observe it.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:007, could you have a look please?

For more information, please visit BugBot documentation.

Comment 11

[Marc Leclair [:mleclair]]

I just noticed this but I do see it happening on the A54 at times. Here I was browsing reddit and went to another tab (I think it was amazon) and came back. The tab was effectively dead and unusable (no amount of back space, rewriting the URL made it work). Everything else is still working as intended (user can interact with awesome bar, settings, switch tabs). The other odd things that happen is that, while the preview of the tab is still available at first, I will sometimes see the other tab (in this case amazons) in my "dead" tab (in this case reddit). Not sure if that made sense the way I wrote it.

here's a profile of when it happened. Although, the profiler started after I switched to the dead tab.

Comment 12

[eclaudiu64]

I encountered this error many times. I think it happens after I'm on a tab and then exit Firefox but I let it run in the background and go to another app and then go back to Firefox the page content freezes and I can't scroll up and down it recovers after I close the tab and re-enter the address again. This error happens very often.

Comment 13

[eclaudiu64]

(In reply to eclaudiu64 from comment #12)

I encountered this error many times. I think it happens after I'm on a tab and then exit Firefox but I let it run in the background and go to another app and then go back to Firefox the page content freezes and I can't scroll up and down it recovers after I close the tab and re-enter the address again. This error happens very often.

The situation opened by me is more related to this error: https://bugzilla.mozilla.org/show_bug.cgi?id=1812691 , but it is somewhat related. In this error, I noticed that immediately upon opening the application, I quickly see the open tabs (having opened more than 100 tabs), being at the bottom of the tab list, some tabs show the contents of other tabs, but it happens quickly, maybe it has something to do with this ticket.

Comment 14

[eclaudiu64]

(In reply to eclaudiu64 from comment #13)

In this error, I noticed that immediately upon opening the application, I quickly see the open tabs (having opened more than 100 tabs), being at the bottom of the tab list, some tabs show the contents of other tabs, but it happens quickly, maybe it has something to do with this ticket.

With the setting in the developer options " Enable trab tray to compose rewrite", it can be seen in more detail.

Comment 15

[jackyzy823]

Some digging :

There's a rare case (i don't know how to 100% trigger) that when renderTab and CreateEngineSessionAction , engineState.engineSession is null AND engineState.initializing is true.

Wrong Flow:

1. in renderTab , engineState.engineSession=null -> Dispatch CreateEngineSessionAction

2. in CreateEngineSessionMiddleware's CreateEngineSessionAction case , engineState.initializing=true , so createEngineSession will not be called

3. createEngineSession is not called , so LinkEngineSessionAction is not dispatched -> UpdateEngineSessionInitializingAction(initializing = false) will not be dispatched. -> engineState.initializing will never be set to false.

4. createEngineSession is not called , engineState.engineSession=null is not changed -> (EngineViewPresenter monitors value's changing) -> renderTab will not be called again

---

So

1. when swiping from other tabs, the view's render is not updated , so it shows adjacent tab.
2. when switching to home and back , the view is black, since view is destroyed.

Comment 16

[jackyzy823]

Further digging:
The root cause is that, collect will emit multiple times

It will emit 2 records when LinkEngineSessionAction happens.

I added log in ifAnyChange to demonstrate.

@@ -88,6 +89,8 @@ fun <T, R> Flow<T>.ifAnyChanged(transform: (T) -> Array<R>): Flow<T> {
             ?.any()

         if (!observedValueOnce || hasChanges == true) {
+            Logger("WrongTab").debug("ifAnyChange: mapped:" + mapped.contentToString())
+            Logger("WrongTab").debug("ifAnyChange: lastMappedValues:" + lastMappedValues.contentToString())
             lastMappedValues = mapped
             observedValueOnce = true
             true

01-23 14:29:34.439  4801  4801 D WrongTab: ifAnyChange: mapped:[670ed72c-92b0-4343-b872-5136ae4ee512, null, false, false]
01-23 14:29:34.439  4801  4801 D WrongTab: ifAnyChange: lastMappedValues:null

01-23 14:29:34.439 4801 4801 D WrongTab: ifAnyChange: mapped:[670ed72c-92b0-4343-b872-5136ae4ee512, mozilla.components.browser.engine.gecko.GeckoEngineSession@69ca290, false, false]
01-23 14:29:34.439 4801 4801 D WrongTab: ifAnyChange: lastMappedValues:[670ed72c-92b0-4343-b872-5136ae4ee512, null, false, false

Note:
the array means [ tab?.id, tab?.engineState?.engineSession, tab?.engineState?.crashed, tab?.content?.firstContentfulPaint,]

Ideally, there should be one record emitted
from null to [670ed72c-92b0-4343-b872-5136ae4ee512, mozilla.components.browser.engine.gecko.GeckoEngineSession@69ca290, false, false]

but now
from null to [670ed72c-92b0-4343-b872-5136ae4ee512, null, false, false] to [670ed72c-92b0-4343-b872-5136ae4ee512, mozilla.components.browser.engine.gecko.GeckoEngineSession@69ca290, false, false]

And the middle state has the condition engineSession = null and initializing =true

---

Here's some solution

1. Use collectLatest to replace collect Note: It will cancel previous renderTab , but the deadlock may still happen.
2. check the situation in renderTab if engineSession == null && initializing == true , then return
3. check the situation in renderTab if engineSession == null && initializing == true , then dispatch UpdateEngineSessionInitializingAction(false)

I'd prefer the second way, because it's the simplest way.

Comment 17

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1853107 - Fix a race condition on renderTab (#5257)

Comment 18

[Matt Tighe [:matt-tighe]]

(In reply to jackyzy823 from comment #15)

Some digging :

There's a rare case (i don't know how to 100% trigger) that when renderTab and CreateEngineSessionAction , engineState.engineSession is null AND engineState.initializing is true.

Wrong Flow:

1. in renderTab , engineState.engineSession=null -> Dispatch CreateEngineSessionAction

2. in CreateEngineSessionMiddleware's CreateEngineSessionAction case , engineState.initializing=true , so createEngineSession will not be called

3. createEngineSession is not called , so LinkEngineSessionAction is not dispatched -> UpdateEngineSessionInitializingAction(initializing = false) will not be dispatched. -> engineState.initializing will never be set to false.

4. createEngineSession is not called , engineState.engineSession=null is not changed -> (EngineViewPresenter monitors value's changing) -> renderTab will not be called again

---

So

1. when swiping from other tabs, the view's render is not updated , so it shows adjacent tab.
2. when switching to home and back , the view is black, since view is destroyed.

Hey, thanks as always for digging so deeply into this and submitting a patch! That said, I have a few questions to try and understand the solution better.

I am not sure why the flow presented above is an issue. For engineState.initializing to be true, we must have already hit the CreateEngineSessionAction case and called this dispatch. This means the reducer chain hasn't finished, so renderTab has not yet been called as a result of the engineSession being linked by EngineStateReducer. Eventually, the reducer chain should finish and the collect call should be triggered with a non-null engineSession. This is shown in your collection of logs as the final state.

The middle state must mean the collect in EngineViewPresenter is triggered by a different property than the engineSession. in the ifAnyChanged. That's fine, because we should eventually hit the no-op in CreateEngineSessionMiddleware as you've described.

I am still wondering why renderTab fails after the engineSession becomes non-null. I am not sure if your patch solves that problem, but I am open to learning differently.

Comment 19

[jackyzy823]

Thanks for the questions and after thinking again , i think i found the real key point.

I am not sure if your patch solves that problem

Yes , it is just a mitigation but not a fix.

the key point: https://searchfox.org/mozilla-mobile/rev/99576d6e69eac8ee1bf6c8a654d5fa96a6f6bf84/firefox-android/android-components/components/browser/state/src/main/java/mozilla/components/browser/state/engine/middleware/CreateEngineSessionMiddleware.kt#89-102

Since in these situations , LinkEngineSessionAction is not called so local variable engineObserver is null , so UpdateEngineSessionInitializingAction(init=False) will not be called.

So the value of initializing will be True.

The next time renderTab called with enginesession is null (previously unlinked like recovery from low memory state) , it will go to CreateEngineSessionAction branch , however in this action , nothing will called because initializing is True.
and as said in comment the renderTab will not be triggered again , so so there's no chance to go into the branch calling engineView.render(engineSession).

The race condtion is the session engine is null [HERE}(https://searchfox.org/mozilla-mobile/rev/99576d6e69eac8ee1bf6c8a654d5fa96a6f6bf84/firefox-android/android-components/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt#80) but not null [HERE}(https://searchfox.org/mozilla-mobile/rev/99576d6e69eac8ee1bf6c8a654d5fa96a6f6bf84/firefox-android/android-components/components/browser/state/src/main/java/mozilla/components/browser/state/engine/middleware/CreateEngineSessionMiddleware.kt#99)

I'm not sure if this makes you understand well.

---------------- PLEASE IGNORE BELOW / These're previous thoughts-------------------------

For engineState.initializing to be true, we must have already hit the CreateEngineSessionAction case and called this dispatch.

Yes

This means the reducer chain hasn't finished,

Yes

so renderTab has not yet been called as a result of the engineSession being linked by EngineStateReducer. Eventually, the reducer chain should finish and the collect call should be triggered with a non-null engineSession. This is shown in your collection of logs as the final state.

No, i don't think so , EngineViewPresenter doesn't depend on reducer chain. In my understanding, EngineViewPresenter is just a thread periodically monitoring some key of SessionState 's change.

It have a chance that it hit and emit record between these two actions UpdateEngineSessionInitializingAction (init=True) (value from null to [tabid , sess = null , crash=false , firstpaint=false] ) and LinkEngineSessionAction (engineSession=SOME_ENGINESESSION) (value from [tabid , sess = null , crash=false , firstpaint=false]

---

That's fine, because we should eventually hit the no-op in CreateEngineSessionMiddleware as you've described.
I am still wondering why renderTab fails after the engineSession becomes non-null

Because at this time , EngineViewPresenter monitors no change, so renderTab will not be called again so there's no change to go into the branch calling engineView.render(engineSession).

Comment 20

[Denis Müller [:Webworkr]]

Error still occurs.

124.0a1 (Build #2016000071), c8248adb27+
GV: 124.0a1-20240127092204
AS: 124.20240127050245

Comment 21

[Matt Tighe [:matt-tighe]]

Every time the CreateEngineSessionAction is dispatched, it is processed individually by the reducer chain attached to BrowserStore. This should mean during the first dispatch, initializing is false and createEngineSession is called. This should then dispatch a LinkEngineSessionAction, which allows the EngineStateReducer to update BrowserState with a non-null engineSession. This should trigger the observation in EngineViewPresenter and call renderTab with a non-null session.

Typically, Stores handle dispatches through a synchronized queue. That should mean that the order of actions is handled deterministically, and the engineSession should eventually become null. Either something is overwriting the engineSession state before renderTab can complete successfully, or the Actions get processed out of order somehow.

Since this is critical infrastructure to the browser, I don't think we should risk landing a mitigation patch until we fully understand why the problem is happening. We may be able to address the root cause instead.

Comment 22

[jackyzy823]

Attachment: chart.svg

Comment 23

[jackyzy823]

Since this is critical infrastructure to the browser, I don't think we should risk landing a mitigation patch until we fully understand why the problem is happening. We may be able to address the root cause instead.

The first version of fix is a mitigation.
The newest version is targeing the root cause.

---

I created a chart to try to demonstrate:
https://excalidraw.com/#json=TJa9vhfWn18O6CfhpaaHE,oddd_tgtQQOyQvZi30ckfg

Attachment is the svg version of the chart

Comment 24

[jackyzy823]

The first version of fix is a mitigation.

Also i think including the first version patch does no harm , and is quite a defensive way to prevent further race condition.

diff --git a/android-components/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt b/android-components/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt
index af440358da87..71ee2671369b 100644
--- a/android-components/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt
+++ b/android-components/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt
@@ -78,6 +78,8 @@ internal class EngineViewPresenter(
         }
 
         if (engineSession == null) {
+            // To prevent a potential race condition here.
+            if (tab.engineState.initializing) { return }
             // This tab does not have an EngineSession that we can render yet. Let's dispatch an
             // action to request creating one. Once one was created and linked to this session, this
             // method will get invoked again.

Comment 25

[Noah Bond [:007]]

Dang, that flow chart is impressive! Nice work, Jacky!

One question: Is there a missing bubble that should be below CreateEngineSessionAction?

Comment 26

[jackyzy823]

One question: Is there a missing bubble that should be below CreateEngineSessionAction?

Do you mean the last CreateEngineSessionAction in the action queue ? If so, no . Followings are other actions in the queue which are not related to this bug , so i just omit them.

Comment 27

[jackyzy823]

Attachment: chart2.svg

I updated the chart according to the feedback: https://excalidraw.com/#json=7ERvN2vGI4r_tqjTFwqHX,VT2ioJyXC6povJ1P3E_dFQ

Also added the fix solution (two versions PR) in the chart.

Comment 28

[Matt Tighe [:matt-tighe]]

I have a couple more questions, thanks for patience in working out all these details. I added some highlighted boxes to your (very helpful) diagram here.

If you've found a strategy for reproduction, it would be helpful to provide some logs as well. Today I opened a patch that may help you capture some useful ones. You should be able to use this logger to more easily capture the exact ordering of actions, for example by configuring it such that:

        val changeDetectionMiddleware = ChangeDetectionMiddleware<List<EngineState>, BrowserState, BrowserAction>(
            selector = { it.tabs.map { it.engineState } },
            onChange = { action, pre, post ->
                logger("$action, $pre, $post")
            }
        )

Comment 29

[jackyzy823]

Attachment: debug_log.patch

The patch to collect logs.

Comment 30

[jackyzy823]

Attachment: debug.log

The buggy tab is 6050befe-301b-4b97-ad5a-da91c0385312

And the notable line is:

02-02 13:19:49.649 18729 18729 D Bug1853107: ifAnyChange:  Current: [6050befe-301b-4b97-ad5a-da91c0385312, null, false, false]
02-02 13:19:49.649 18729 18729 D Bug1853107: ifAnyChange: Previous: null
02-02 13:19:49.649 18729 18729 D Bug1853107: In EngineViewPresenter onTabToTender tab: 6050befe-301b-4b97-ad5a-da91c0385312 engineSession=null initializing=true url: https://www.google.com/search?q=66666&ie=utf-8&oe=utf-8&client=firefox-b-m crashed=false firstContentfulPaint=false

The rare case. (Other tabs from null to [afb79223-b812-48e8-8c16-f725de5de780, mozilla.components.browser.engine.gecko.GeckoEngineSession@4162a66, false, false] )

---

02-02 13:19:49.665 18729 18793 D Bug1853107: in CreateEngineSessionAction for 6050befe-301b-4b97-ad5a-da91c0385312 init?: false
02-02 13:19:49.665 18729 18793 D Bug1853107: EngineStateReducer UpdateEngineSessionInitializingAction for 6050befe-301b-4b97-ad5a-da91c0385312  val: true Prev: false
02-02 13:19:49.706 18729 18729 D Bug1853107: CreateEngineSessionMiddleware Engine session already exists for tab 6050befe-301b-4b97-ad5a-da91c0385312

You'll never find UpdateEngineSessionInitializingAction (false) for this tab in following logs now.

02-02 13:32:12.652 18729 18729 D Bug1853107: ifAnyChange:  Current: [6050befe-301b-4b97-ad5a-da91c0385312, null, false, true]
02-02 13:32:12.653 18729 18729 D Bug1853107: ifAnyChange: Previous: [ba1e5aca-1f54-40f3-90c6-faf3d7ff44e1, mozilla.components.browser.engine.gecko.GeckoEngineSession@224692a, false, true]
02-02 13:32:12.653 18729 18729 D Bug1853107: In EngineViewPresenter onTabToTender tab: 6050befe-301b-4b97-ad5a-da91c0385312 engineSession=null initializing=true url: https://www.google.com/search?q=66666&ie=utf-8&oe=utf-8&client=firefox-b-m crashed=false firstContentfulPaint=true

When tab's enginesession has been set to NULL (due to crash or unlink) , the buggy state occur.

Comment 31

[jackyzy823]

If you've found a strategy for reproduction

1. Set Android's "Developer Options" -> "Background Limit" to "No background Process"
2. Observe "DETECT UNWILLING CASE" in the log , basically it will happens on creating new tab and loading URL
3. When observing the buggy tab, make it not the selected Tab , (so it could be unlink when switching Fenix background)
4. Switching Fenix foreground and swipe tabs from and to the buggy tab to observe the tab adjacent.

---

Also answered the question in chart: https://excalidraw.com/#json=zlxAejoVNWsjqVzqWIkvV,xXWlvXDymxvC5tGe35OXww

Comment 32

[Matt Tighe [:matt-tighe]]

02-02 13:19:49.665 18729 18793 D Bug1853107: in CreateEngineSessionAction for 6050befe-301b-4b97-ad5a-da91c0385312 init?: false
02-02 13:19:49.665 18729 18793 D Bug1853107: EngineStateReducer UpdateEngineSessionInitializingAction for 6050befe-301b-4b97-ad5a-da91c0385312 val: true Prev: false
02-02 13:19:49.706 18729 18729 D Bug1853107: CreateEngineSessionMiddleware Engine session already exists for tab 6050befe-301b-4b97-ad5a-da91c0385312

Okay, so to summarize:

CreateEngineSessionAction results in engineState.initializing being set to true, even in cases where the engine session already exists.
Once initializing is set to true, it can never be reset because LinkEngineSessionAction is not called through CreateEngineSessionMiddleware::getOrCreateEngineSession. Then, if a tab crashes or suspends, we are stuck in a spot where CreateEngineSessionAction can never be fired again. Is that correct?

Comment 33

[jackyzy823]

(In reply to Matt Tighe [:matt-tighe] from comment #32)

Okay, so to summarize:

CreateEngineSessionAction results in engineState.initializing being set to true, even in cases where the engine session already exists.
Once initializing is set to true, it can never be reset because LinkEngineSessionAction is not called through CreateEngineSessionMiddleware::getOrCreateEngineSession. Then, if a tab crashes or suspends, we are stuck in a spot where CreateEngineSessionAction can never be fired again. Is that correct?

Yes

Comment 34

[Matt Tighe [:matt-tighe]]

This error seems more related to setting initializing to true while an engine session already exists. Maybe a more appropriate fix would be to update this line to be:

val engineState = context.state.findTabOrCustomTab(action.tabId)?.engineState
if (engineState?.initializing == false && engineState.engineSession != null) {

this is further informed by the comment for CreateEngineSessionAction:

    /**
     * Creates an [EngineSession] for the given [tabId] if none exists yet.
     */
    data class CreateEngineSessionAction(
        override val tabId: String,
        val skipLoading: Boolean = false,
        val followupAction: BrowserAction? = null,
    ) : EngineAction(), ActionWithTab

Comment 35

[jackyzy823]

if (engineState?.initializing == false && engineState.engineSession != null) {

Wait a second? Is this condition correct ?

( initializing = false and engineSession = SOME_SESSION) is the most common and the normal state.

and
if eng == NULL , we will go to the "else" branch of the condition, is that intentional ?

Comment 36

[Matt Tighe [:matt-tighe]]

Ah thanks for catching that, you're correct that we would want
if (engineState?.initializing == false && engineState.engineSession == null)

Comment 37

[jackyzy823]

Yes.

---

However,

    if (tab == null) {
        logger.warn("Requested engine session for tab. But tab does not exist. ($tabId)")
        return null
    }

    if (tab.engineState.crashed) {
        logger.warn("Not creating engine session, since tab is crashed. Waiting for restore.")
        return null
    }

This two situations are still not covered.

Comment 38

[Marc Leclair [:mleclair]]

Hey folks, I just noticed this bug so I'm not sure if this would be helpful since I'm late, but I did suffer of this bug a while back and was able to catch a profile of the aftermath. Here's what I was able to get. I believe I was scrolling reddit and switched tab and my tab was dead when I came back to it. It was stuck in a weird CreateEngineSession Loop. Unfortunately I wasn't able to reproduce and profile what happened before hand.

Comment 39

[Denis Müller [:Webworkr]]

Partial problem "black screen" with bug 1812359 solved?

Comment 40

[jackyzy823]

That's a different bug. (it only occurs on CustomTabs/PWA)

Comment 41

[Denis Müller [:Webworkr]]

That's a different bug. (it only occurs on CustomTabs/PWA)

Thanks for clarifying.

Comment 42

[Matt Tighe [:matt-tighe]]

(In reply to jackyzy823 from comment #37)

Yes.

---

However,

    if (tab == null) {
        logger.warn("Requested engine session for tab. But tab does not exist. ($tabId)")
        return null
    }

    if (tab.engineState.crashed) {
        logger.warn("Not creating engine session, since tab is crashed. Waiting for restore.")
        return null
    }

This two situations are still not covered.

I'll address each case separately:

1. tab == null: it does seem like we would want to avoid initializing an engine session entirely for null tabs, since the original condition for this branch requires a tab to not be null. Instead of retrieving the tab from the Store a second time here, we could pass the tab through the function callstack and get rid of the null check.

2. tab.engineState.crashed: this one may be a little more complex. I'll need to look into how tab restoration is handled, so that we can determine how engine sessions are created and linked for tabs that are being restored. I would assume that we may want to just move this condition up to the original condition and prevent initialization from starting in the first place.

In both cases, there are some asynchronous and mutable state issues that we will need to be careful of. Namely, that the tab is retrieved from the context a second time after a new coroutine is launched here, which means we have lost all guarantees that the tab has not changed between the two calls to findTab. I am not sure exactly why the scope.launch is even required, though I suspect it is because the engine operations create and restore in this function may want to be handled asynchronously. It would be interesting to see if there are any details in the git history or issues related to the patches that introduced those changes.

Comment 43

[jackyzy823]

I updated the patch.

---

I can't found anything in git log ( It was renamed and added these conditions in https://github.com/mozilla-mobile/firefox-android/commit/1f21fdada9504b38fb79234dd3ca447fa175137d#diff-132bc2cc57c7e7d1b84fbe1cad74ef1d5b838fbbe5f10d96f105048f97f04b2e )

Comment 44

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1853107 - Fix a race condition on CreateEngineSession (#5746)

Comment 45

[Matt Tighe [:matt-tighe]]

Thanks again for your patience in getting this patch landed. Things have been a little chaotic around here the last few weeks.

From a code perspective, I think I'm ready to approve and merge this PR. I'm having some trouble reproducing the failure state still though, so I don't feel confident I can test the patch. Would you be able to provide video of the STR in the failure case and with the fix? That would be incredibly helpful

Comment 46

[jackyzy823]

Hi ::matt-tighe,

I made two videos.

The first one before-fix) is based on the commit 13b08c4c4d535f87dc43973f7f37057cd4851e6f (the previous one commit before the PR), and add a log line in EngineViewPresenter.kt to observe which tab hit the bug.

         if (engineSession == null) {
+            if (tab.engineState.initializing) {  // Log line }

The second one (after-fix) is based on the PR, and also a log line.

Comment 47

[jackyzy823]

Attachment: before-fix-compress.mp4

0:00 - 1:20 find the buggy tab (via log line )-> it is qvc.com
1:00 - 5:35 do open some app and wait system to recycle memory
5:35 Pay attention to the tab qvc.com . at 5:49 the qvc.com shows left side one : google search "123" , at 5:57 qvc.com shows right side one: ebay.com , when switch out and in , it shows the black view.

Comment 48

[jackyzy823]

Attachment: after-fix-compress.mp4

The buggy tab is google.com

8:07 google is reloaded correctly.

Comment 49

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1853107 - Fix a race condition on CreateEngineSession (#5902)

Comment 50

[BMO Github Automation]

Authored by https://github.com/jackyzy823
https://github.com/mozilla-mobile/firefox-android/commit/cc37833fc34afd27595cd7af05f6ab7409d0b921
[main] Bug 1853107 - Fix a race condition on CreateEngineSession

Comment 51

[Matt Tighe [:matt-tighe]]

For QA: there are some potential reproduction steps listed in https://bugzilla.mozilla.org/show_bug.cgi?id=1853107#c31, but this bug does seem generally hard to test. Please just give it your best shot to prove it's been fixed, and a general smoke test of creating tabs, switching tabs, and tabs restoring correctly when the app comes back to the foreground would very helpful. Thanks!

Comment 52

[miralobontiu]

We've verified with the following devices, on the Fenix Nightly 125.0a1 builds from 3/7, and 3/11:

• Samsung Galaxy S24 (Android 14), and
• Samsung Galaxy S23 Ultra (Android 14).

We did a sanity check, and tested several scenarios regarding creating tabs, closing tabs, reordeting tabs, switching tabs, sending the app in the background, and reopening, restoring tabs. We did not encounter any crash, everything worked as expected.

Comment 61

[Denis Müller [:Webworkr]]

Thanks for fixing.

Comment 62

[Denis Müller [:Webworkr]]

Msybe related to Bug 1887659.

Comment 63

[Denis Müller [:Webworkr]]

Unlike bug 1887659, I occasionally observe that some tabs freeze again and the content of an adjacent tab is displayed. Could this be related to this ticket again?

Nightly
127.0a1 (Build #2016019535), hg-0d84aa48c2d6+
GV: 127.0a1-20240507214419
AS: 127.20240507050248

Comment 64

[jackyzy823]

I'm not sure, but i personally encountered tabs freezing too. I'll keep an eye on this. (maybe new bug introduced or the fix is not sufficient)

Comment 65

[Denis Müller [:Webworkr]]

It is possible that a new reference to the suspicion also makes sense in Bug 1887659. Unfortunately, I do not have write permission for closed bug reports that I have not opened myself. My observations tend to favor this one or a new one instead of 1887659.