Unable to switch to any tab on load when reloading windows from last time

[evq]

Test plan

#9589 (comment)

---

• Did you search for similar issues before submitting this one?
  Yes

• Describe the issue you encountered:
  After pulling the master branch and re-building, much of the browser appears "frozen" as no tab is selected, an all white window is displayed below the tab bar, clicking to switch to a tab does nothing, and the "kebab" menu does not work.

• Platform (Win7, 8, 10? macOS? Linux distro?):
  Linux, Arch AppVM on Qubes

• Brave Version (revision SHA):
  6478bcd

• Steps to reproduce:
  The following does not seem to consistently reproduce the issue:

  1. Ensure setting to "reopen closed windows from last time" is on
  2. Open up a number of tabs
  3. Close the browser
  4. Reopen the browser

• Actual result:
  Tabs appear to be reloaded, however no tab is selected and it is not possible to select a tab.

• Expected result:
  Tabs should be reloaded and it should be possible to select a tab. I would also expect that one tab is selected on load.

• Will the steps above reproduce in a fresh profile? If not what other info can be added?
  No, I don't seem to see this issue with a fresh profile. I opened an inspector using shift-f8 and saw the following error:

Uncaught TypeError: Expected iterable or array-like: null
    at forceIterator (immutable.js?1793:3622)
    at Map.updateIn (immutable.js?1793:1282)
    at Map.mergeIn (immutable.js?1793:1314)
    at doAction (windowStore.js:416)
    at callbacks.forEach (appDispatcher.js?c639:97)
    at Array.forEach (<anonymous>)
    at AppDispatcher.dispatchToOwnRegisteredCallbacks (appDispatcher.js?c639:96)
    at AppDispatcher.dispatchInternal (appDispatcher.js?c639:112)
    at async.cargo (appDispatcher.js?c639:146)
    at process (async.js?b52e:2316)

I went to the line mentioned in the trace in windowStore.js and commented out the following code section which results in the issue no longer happening:

    //case windowConstants.WINDOW_SET_LINK_HOVER_PREVIEW:
      //windowState = windowState.mergeIn(frameStateUtil.activeFrameStatePath(windowState), {
        //hrefPreview: action.href,
        //showOnRight: action.showOnRight
      //})
      //break

I see there is a guard in case windowConstants.WINDOW_SET_STATE: to check if there is an activeFrame, perhaps something similar is needed here? I will try that next.

• Can this issue be consistently reproduced?
  No.

[evq]

Some more observations:

The error seems to stem from a call to state.getIn(['framesInternal', 'index', frameKey.toString()]) where it returns null. The activeFrameKey doesn't seem to exist in framesInternal. Looking at activeFrameKey in my session store file it seems to reference a frame that is in closedFrames. Unfortunately, just changing the activeFrameKey to reference a closed frame in a new test profile does not seem to be sufficient to reproduce this issue. Maybe there is some code that handles the case where activeFrameKey is invalid on start but due to timing does not run in my profile?

[evq]

I took a swing at adding session store cleanup code that cleans up the activeFrameKey in the event it references a frame in closedFrames. It also pro-actively triggers the cleanup code to run on load by trying to detect this condition. Seems like this part could probably be dropped to just rely on the version bump to run the cleanup code.

master...evq:fix/wip-active-frame-closed-cleanup

[bsclifton]

awesome, thanks for bringing this up @evq (and also for offering a solution). I'll assign this to the 0.17.x milestone so that we can triage appropriately

[bridiver]

@evq I think this might be fixing a symptom instead of the problem. activeFrameKey should never point to a frame in closeFrames and I think we should find out why that is happening instead of trying to clean up the mess when it does happen

[evq]

@bridiver Yup, +1 for finding the root cause. Unfortunately I have been unable to reliability reproduce the cause, so I could only work from a state of already having a messed up activeFrameKey.

My branch was more of a suggestion to consider being able to gracefully recover in the case that it does somehow get messed up. Perhaps a more general check that activeFrameKey is valid before writing out the session store would be appropriate? Let me know if you think this might be useful in addition to solving the root cause.

[bridiver]

It's a tough call. On one hand we don't want users to be left in an unrecoverable state, but we also don't want to mask problems with workarounds. What we really need is a way to get "crash reports" for errors that we recover from and then this wouldn't be a big issue

[bridiver]

I think I see what the issue is @evq. We are adding to closedFrames on will-destroy in frameStateUtil.removeFrame and frameReducer WINDOW_CLOSE_FRAMES, but that happens before the activeFrame is updated. We should move closedFrames to APP_TAB_CLOSED and possibly get rid of the will-destroy handler and move everything from windowConstants.WINDOW_CLOSE_FRAME to appConstants. APP_TAB_CLOSED in frameReducer

[bridiver]

another option is to move closedFrames to appState which is what we want to do with most things long-term anyway, but I didn't look to see how much work that would be

[evq]

@bridiver fyi after rebuilding browser-laptop-bootstrap I'm consistently seeing a related issue with activeFrameKey when starting without any profile at all. seems it's starting off as null so on first launch the browser is totally unresponsive, (and subsequent loads since null is written into the session store on close.) I'll look into this one, it seems to be a different root cause (with the same symptoms) based on your notes above.

[bridiver]

@evq it's muon-version dependent so it's something between 4.0.x and 4.1.x, but feel free to take a stab at fixing the closed frame issue if you want

[bridiver]

actually there are two independent issues. The active frame is not related to the console error and I have a fix for that. The console error just needs a null check for activeFrameStatePath I think