Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Toolbar dial and other NTP extensions will show Brave NTP on restart #30890

Closed
Joovsr opened this issue Jun 7, 2023 · 5 comments · Fixed by brave/brave-core#18898
Closed
Assignees
Labels
bug feature/extensions feature/new-tab OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass-Win64 QA/Yes release-notes/include webcompat/not-shields-related Sites are breaking because of something other than Shields.

Comments

@Joovsr
Copy link

Joovsr commented Jun 7, 2023

Hello. I don't like the default new tab layout in this browser. Therefore, I use a third-party extension - "toolbar dial". It replaces the default new tab layout.
The bug is that if you create a new empty tab and then restart the browser, the old native start screen is displayed in the new tab! When creating further new tabs already the extension works fine.
In google chrome, everything works as it should - at startup, the previous tabs open and if there are confusing ones among them, then the "toolbar dial" works as it should.
I think the problem is with all extensions that replace the default new tab screen.
Thank you.

https://chrome.google.com/webstore/detail/toolbar-dial/nejdcfinmjpapnkjffcejgcidjmbipcp?hl=en

@rebron rebron added feature/extensions feature/new-tab webcompat/not-shields-related Sites are breaking because of something other than Shields. needs-investigation A bug not 100% confirmed/fixed labels Jun 9, 2023
@rebron rebron changed the title [BUG] The alternative express panel does not work after restarting the browser. [Bug] Toolbar dial and other NTP extensions will show Brave NTP on restart Jun 9, 2023
@rebron
Copy link
Collaborator

rebron commented Jun 9, 2023

We'll want to verify that isn't a problem with another extension too like https://chrome.google.com/webstore/detail/momentum/laookkfknpbbblfpciffpaejjkokdgca

@rebron rebron added the priority/P3 The next thing for us to work on. It'll ride the trains. label Jun 9, 2023
@MadhaviSeelam
Copy link

@rebron reproduced this issue in 1.52.122 with both the extensions.

Brave | 1.52.122 Chromium: 114.0.5735.110 (Official Build) (64-bit)
-- | --
Revision | 1c828682b85bbc70230a48f5e345489ec447373e-refs/branch-heads/5735_90@{#13}
OS | Windows 11 Version 22H2 (Build 22621.1702)
2023-06-09_14h40_00.mp4
2023-06-09_14h38_14.mp4

@rebron rebron added priority/P2 A bad problem. We might uplift this to the next planned release. QA/Yes release-notes/include bug and removed needs-investigation A bug not 100% confirmed/fixed priority/P3 The next thing for us to work on. It'll ride the trains. labels Jun 9, 2023
@MadhaviSeelam
Copy link

@rebron

brave://settings

image

another way to reproduce the issue

2023-06-09_14h56_36.mp4

@simonhong simonhong self-assigned this Jun 12, 2023
@simonhong simonhong added this to In progress in Front End Jun 12, 2023
@simonhong
Copy link
Member

simonhong commented Jun 12, 2023

Confirmed that newtab url overriding is done via ExtensionWebUI::RegisterOrActivateChromeURLOverrides() before tab restoring during the startup. but restored new tab doesn't use overridden url. Checking..

This only happens when NTP is restored during the startup.
At startup, tab is restored via chrome::AddRestoredTab().
Actual WebContents for restored tab is created by CreateRestoredTab().
In CreateRestoredTab(), NavigationEntry is created for brave://newtab.
In ContentSerializedNavigationBuilder::ToNavigationEntry(),
it creates NavigationEntry and re-calculated to extension's ntp url for brave://newtab in NavigationControllerImpl::CreateNavigationEntry().
However, that entry's url is reverted to brave://newtab again after entry is created in ContentSerializedNavigationBuilder::ToNavigationEntry().

In ToNavigationEntry(), restored navigation's encoded_page_state_ is empty.
Curious this empty page state is normal condition or not.
As it's empty, entry's page state created with navigation->virtual_url_(chrome://newtab) and set.
After that, entry->GetURL() is chrome://newtab) not extension's overridden url.

  if (navigation->encoded_page_state_.empty()) {
    // Ensure that the deserialized/restored content::NavigationEntry (and
    // the content::FrameNavigationEntry underneath) has a valid PageState.
    entry->SetPageState(
        // blink::PageState::CreateFromURL(navigation->virtual_url_),
        blink::PageState::CreateFromURL(entry->GetURL()),

If state is created with entry->GetURL() like above instead of virtual_url_,
extension's newtab is loaded properly during the startup and can't repro this issue.

Regarding to empty state from restored navigation,
newtab has always empty state but www.brave.com has valid page state when it's restored in startup.

Found the cause - This regression comes from brave/brave-core#14603.
As we sanitize chrome urls, its encoded page state is empty.
Because of this, new page state is created when chrome url is restored.
and page state is created with virtual_url, overwritten url(extension url) for newtab is not used.

When NavigationEntry is created, url is re-written for newtab but it's ignored if encoded page state isn't existed.
This is the root cause of this issue.

simonhong added a commit to brave/brave-core that referenced this issue Jun 14, 2023
…startup

fix brave/brave-browser#30890

As we sanitize serialzed navigation(brave/brave-browser#30890),
encoded page state is empty for all chrome url.
So, new page state is created with virtual url when it's restored during the startup.
ContentSerializedNavigationBuilder::ToNavigationEntry() set new page state if it's empty.
When new page state is created, navigation's virtual url is created.
If url is re-written when NavigationEntryImpl is created, that re-written url is ignored.
To fix this, including url info only for chrome url's encoded page state.
simonhong added a commit to brave/brave-core that referenced this issue Jun 14, 2023
…startup

fix brave/brave-browser#30890

As we sanitize serialzed navigation(brave/brave-browser#30890),
encoded page state is empty for all chrome url.
So, new page state is created with virtual url when it's restored during the startup.
ContentSerializedNavigationBuilder::ToNavigationEntry() set new page state if it's empty.
When new page state is created, navigation's virtual url is created.
If url is re-written when NavigationEntryImpl is created, that re-written url is ignored.
To fix this, including url info only for chrome url's encoded page state.
simonhong added a commit to brave/brave-core that referenced this issue Jun 14, 2023
…startup

fix brave/brave-browser#30890

As we sanitize serialzed navigation(brave/brave-browser#30890),
encoded page state is empty for all chrome url.
So, new page state is created with virtual url when it's restored during the startup.
ContentSerializedNavigationBuilder::ToNavigationEntry() set new page state if it's empty.
When new page state is created, navigation's virtual url is created.
If url is re-written when NavigationEntryImpl is created, that re-written url is ignored.
To fix this, including url info only for chrome url's encoded page state.
simonhong added a commit to brave/brave-core that referenced this issue Jun 14, 2023
…startup

fix brave/brave-browser#30890

As we sanitize serialzed navigation(brave/brave-browser#30890),
encoded page state is empty for all chrome url.
So, new page state is created with virtual url when it's restored during the startup.
ContentSerializedNavigationBuilder::ToNavigationEntry() set new page state if it's empty.
When new page state is created, navigation's virtual url is created.
If url is re-written when NavigationEntryImpl is created, that re-written url is ignored.
To fix this, including url info only for chrome url's encoded page state.
@brave-builds brave-builds added this to the 1.54.x - Nightly milestone Jun 14, 2023
@simonhong simonhong moved this from In progress to Completed in Front End Jun 14, 2023
@MadhaviSeelam
Copy link

MadhaviSeelam commented Jul 12, 2023

Verification PASSED using

Brave | 1.57.8 Chromium: 115.0.5790.75 (Official Build) beta (64-bit)
-- | --
Revision | 77bffd419b7e89d7e666c9695e7fa0d1d0367a99
OS | Windows 11 Version 22H2 (Build 22621.1992)

Case 1: Install Tooldial extension - PASSED

  1. Install 1.57.8
  2. launch Brave
  3. install following extension in chrome.google.com/webstore
  4. open a new tab
  5. verified ToolbarDial NTP page is shown
  6. clicked Keep it
  7. click a new tab
  8. verified Brave NTP page is not shown as expected
  9. close Brave and relaunch
  10. opened brave://settings

Confirmed Brave NTP is not shown.
Confirmed when clicked to open new tabs, Toolbar dial tabs are opened
Confirmed Toolbar is controlling this setting.. message shown in brave://settings

step 3 step 5 step 8 step 9 step 10
image image image image image

Case 2: Install Momentum extension - PASSED

  1. new profile
  2. launch Brave
  3. install following extension in chrome.google.com/webstore
  4. verified Momentum NTP page is shown in a new tab
  5. clicked Keep it
  6. click a new tab
  7. verified Brave NTP page is not shown as expected
  8. close Brave and relaunch
  9. opened brave://settings

Confirmed Brave NTP is not shown.
Confirmed when clicked to open new tabs, Momentum tabs are opened
Confirmed Momentum is controlling this setting.. message shown in brave://settings

step 3-4 step 8-9 step 10
image image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feature/extensions feature/new-tab OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass-Win64 QA/Yes release-notes/include webcompat/not-shields-related Sites are breaking because of something other than Shields.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants