Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Closing tab page makes browser unresponsive #11028

Closed
srirambv opened this issue Sep 20, 2017 · 16 comments · Fixed by #11219
Closed

Closing tab page makes browser unresponsive #11028

srirambv opened this issue Sep 20, 2017 · 16 comments · Fixed by #11219

Comments

@srirambv
Copy link
Collaborator

srirambv commented Sep 20, 2017

Test plan

#11311 (comment)


Description

Closing tab page makes browser unresponsive

Steps to Reproduce

  1. Create enough tabs to create a tab page
  2. Set focus to tab on second page and close the first tab set
  3. Entire browser becomes unresponsive, active tab is not shown
  4. Unable to quit have to kill process and restart to use it again

Actual result:

tabpage
image

Expected result:
Should work normally after closing tab page
Reproduces how often: [What percentage of the time does it reproduce?]
100%

Brave Version

Brave 0.19.7
rev c8481a9
Muon 4.4.16

Additional Information

If a YT video is playing in the tab page that is closed. then it creates an empty tab which cannot be switched.

Found while verifying #9793

@srirambv srirambv added bug feature/tabsbar OS/Windows priority/P1 Blocks development or testing. Product cannot run. Must be fixed immediately, shipped next release. QA/test-plan-specified release/blocking labels Sep 20, 2017
@srirambv srirambv added this to the 0.19.x (Beta Channel) milestone Sep 20, 2017
@luixxiul
Copy link
Contributor

I cannot reproduce the issue on Debian.

@bbondy
Copy link
Member

bbondy commented Sep 22, 2017

I fixed this but there's another problem that isn't related to this fix to be aware of when doing QA:
#11085

cc @srirambv @LaurenWags

bbondy added a commit that referenced this issue Sep 22, 2017
bbondy added a commit that referenced this issue Sep 22, 2017
@bbondy bbondy closed this as completed in aadb3bb Sep 22, 2017
@srirambv
Copy link
Collaborator Author

@bbondy on 0.19.13 which has the fix, it still happens even when all tabs are completely loaded
11028.zip

@cezaraugusto
Copy link
Contributor

can't repro with local build same sha per #11028 (comment) video, even by unflagging welcome page in dev builds. I'll try the packed build.

@cezaraugusto
Copy link
Contributor

consistently reproduced in package builds

@cezaraugusto
Copy link
Contributor

"Uncaught TypeError: Cannot read property 'get' of undefined", source: chrome://brave/Users/cezaraugusto/dev/browser-laptop/app/extensions/brave/gen/app.entry.js (73252)

and

Uncaught TypeError: Cannot read property 'get' of undefined
    at frameReducer (frameReducer.js:111)
    at reduce (windowStore.js:227)
    at Array.reduce (<anonymous>)
    at applyReducers (windowStore.js:225)
    at doAction (windowStore.js:249)
    at callbacks.forEach (appDispatcher.js?c639:107)
    at Array.forEach (<anonymous>)
    at AppDispatcher.dispatchToOwnRegisteredCallbacks (appDispatcher.js?c639:106)
    at AppDispatcher.dispatchInternal (appDispatcher.js?c639:122)
    at run (setImmediate.js:40)

@cezaraugusto
Copy link
Contributor

so we're having a race condition where the tab page is updated but the new active frame is not yet defined, ending up with unusable tabs. I have a quick fix

@cezaraugusto
Copy link
Contributor

@srirambv this was fixed w/ sha 1301ad9 in 0.19.x. Could you re-test please?

@cezaraugusto
Copy link
Contributor

0.20.x d738190
0.19.x 1301ad9
master 7b99b63

@kjozwiak
Copy link
Member

kjozwiak commented Oct 3, 2017

I can still reproduce this on my Win 10 x64 machine using the STR from #11028 (comment). Once you close the tab page, the entire browser becomes unresponsive. Error displayed in the browser console:

Uncaught TypeError: Cannot read property 'get' of undefined app.entry.js:9088 
    at state.get.filter.frame (app.entry.js:9088)
    at app.entry.js:3172
    at List.__iterate (app.entry.js:2294)
    at IndexedIterable.filterSequence.__iterateUncached (app.entry.js:3171)
    at seqIterate (app.entry.js:692)
    at IndexedIterable.IndexedSeq.__iterate (app.entry.js:408)
    at IndexedIterable.forEach (app.entry.js:4469)
    at app.entry.js:2157
    at List.Map.withMutations (app.entry.js:1441)
    at List [as constructor] (app.entry.js:2155)

Build being used:

Brave: 0.19.25
rev: d4fd17a
Muon: 4.4.25
libchromiumcontent: 61.0.3163.100

Example:

issue

@cezaraugusto, could you please take a look? Is this related to #11091?

@kjozwiak kjozwiak reopened this Oct 3, 2017
@bbondy
Copy link
Member

bbondy commented Oct 3, 2017

I have a partial fix but I think it's the wrong fix:
66eba7d

I'm not sure why this only happens on Windows but maybe simply due to timing.

The fix I did above stops the browser from freezes but will leave the dead tabs that happen.

I'm pretty sure what happens is we do a state update after a tab is closed (which is why it happens with loading tabs) and it re-adds the removed frame, i.e. the dead tab.

The real fix should involve not having dead tabs.

CC @darkdh for help, I'll be away the whole day Tuesday but will have some time to pick it back up Wednesday or Thursday if @darkdh doesn't do it first.

@bbondy
Copy link
Member

bbondy commented Oct 4, 2017

I'll take this @cezaraugusto @darkdh
Mainly I think just converting to an app action and doing it all at once might be enough to fix.

@srirambv
Copy link
Collaborator Author

srirambv commented Oct 6, 2017

This seems fixed on 0.19.30. Browser continues to work normal after closing tab page

@bbondy
Copy link
Member

bbondy commented Oct 6, 2017

0.19.x: 511bf20
0.20.x: 7bb5286
master: bf4726d

syuan100 pushed a commit to syuan100/browser-laptop that referenced this issue Nov 9, 2017
syuan100 pushed a commit to syuan100/browser-laptop that referenced this issue Nov 9, 2017
race condition was updating tabPage without an active frame leading to browser freeze

Auditors: @bbondy

fix brave#11028

sibling commits:
- 0.20.x d738190
- 0.19.x 1301ad9
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants