When navigating back new tab page doesn't load #5249
Comments
|
Looked into reproducing the issue and it seems non-deterministic in terms of when I get the "blank" menu versus "last known state of NTP." However, it does require a multiple NTPs to be open at the same time. Just from a glance it looks some conflict/race-condition is causing our |
|
From what you're saying that'll most likely be caused by calling initialize only on if (document.readyState === 'interactive') {
initialize()
} else {
document.addEventListener('DOMContentLoaded', initialize)
}...or probably more clearly use |
|
@petemill I dug around a bit further and it doesn't seem like it isn't a problem with initialize function as thought previously. It's defaulting our NTP settings to Perhaps it's a race condition where we are requesting the preferences object from chrome before it exists? |
|
@imptrx good find, we may have to convert to using the more-common way of setting properties on the webui, using |
|
I don't think I have the required depth to make that change 😞 - would you mind taking this over? |
|
Verified that when navigating back, the page doens't initially have access to anything set via However, the page does have access to properties set via
This screenshot is some logging I added taken of a page after navigating 'back'. It shows that the page does not have access to the |
rebron
added
the
priority/P3
Fix brave/brave-browser#5249 The previous method of wvh->SetWebUIProperty has issues in that the rvh does not always fire the 'ready' event, especially when navigating 'back'. Using DataSource is a more consistent chromium method for sending properties to the JS context. However, this is usually used for non-changing data. Whilst an UpdateDataSource function does exist, that kind of data is usually pulled from the JS via an explicit call to the C++ WebUI, so we may have to go down that route. The issue with the implementation in this commit is that a page may miss the 'updated' events that fire, for example due to have being navigated away and then back to the page. - Creates JS -> C++ functions to get data for: Preferences, Stats and PrivateProperties (alternativeSearchEngine). loadTimeData is also not a great place for data which can change. Although a DataSource (i.e. strings.js and loadTimeData.data) can be updated, it is complicated and is not normal chromium pattern. Since this data is not required exactly at page creation (since it is not read via static HTML but via React in JS), then we can afford the time it takes to request this data via JS. - Converting to a JS API required refactoring some of the app store / reducer initialization, including creating the concept of 'initial data' which is any data required to render the page. Since we need Preferences to know what to show, and we need Stats since they are shown on the initial page load, then those are contained within. Includes some minor cleanup with regard to 'API' separation and definition. - Fixes storage being loaded multiple times during redux init (the function is called with an empty state param 3 times, so the state was being loaded from storage 3 times). - Only saves to local storage the state which we will be re-used.
Fix brave/brave-browser#5249 The previous method of wvh->SetWebUIProperty has issues in that the rvh does not always fire the 'ready' event, especially when navigating 'back'. Using DataSource is a more consistent chromium method for sending properties to the JS context. However, this is usually used for non-changing data. Whilst an UpdateDataSource function does exist, that kind of data is usually pulled from the JS via an explicit call to the C++ WebUI, so we may have to go down that route. The issue with the implementation in this commit is that a page may miss the 'updated' events that fire, for example due to have being navigated away and then back to the page. SetWebUIProperty was also causing issues with browser tests in guest windows. - Creates JS -> C++ functions to get data for: Preferences, Stats and PrivateProperties (alternativeSearchEngine). loadTimeData is also not a great place for data which can change. Although a DataSource (i.e. strings.js and loadTimeData.data) can be updated, it is complicated and is not normal chromium pattern. Since this data is not required exactly at page creation (since it is not read via static HTML but via React in JS), then we can afford the time it takes to request this data via JS. - Converting to a JS API required refactoring some of the app store / reducer initialization, including creating the concept of 'initial data' which is any data required to render the page. Since we need Preferences to know what to show, and we need Stats since they are shown on the initial page load, then those are contained within. Includes some minor cleanup with regard to 'API' separation and definition. - Fixes storage being loaded multiple times during redux init (the function is called with an empty state param 3 times, so the state was being loaded from storage 3 times). - Only saves to local storage the state which we will be re-used.
|
Verification passed on
Verified STR from the description
Verification passed on
Verification PASSED on
|
Description
On the nightly version of Chrome
Version 0.69.44 Chromium: 75.0.3770.100 (Official Build) nightly (64-bit)
Steps to Reproduce
Actual result:
Expected result:
Reproduces how often:
pretty much every time
Brave version (brave://version info)
Version/Channel Information:
Nightly
no
no
no
yes
Other Additional Information:
Miscellaneous Information:
The text was updated successfully, but these errors were encountered: