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

Adding Direct Feeds to Brave News makes the whole browser janky. #23909

Closed
fallaciousreasoning opened this issue Jul 6, 2022 · 10 comments · Fixed by brave/brave-core#14064
Closed
Assignees
Labels
feature/brave-news formerly brave-today OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass-macOS QA/Test-Plan-Specified QA/Yes release-notes/exclude

Comments

@fallaciousreasoning
Copy link

fallaciousreasoning commented Jul 6, 2022

Description

If you add a direct feed to Brave News, the whole browser will lag/freeze when subscribing/unsubscribing from a feed. As best I can tell, this is because the BraveNewsController is fetching the feed on the UI thread, so the main thread is blocked while it downloads the latest info.

It doesn't seem to be a problem for the default publishers as it is much faster (0.2s vs 3.5s), so it might be that we're still blocking the UI thread, it's just not as bad.

My best guess is that the SimpleURLLoaders used by brave/components/api_request_helper/api_request_helper.h are running on the main thread. The feed/sources download is retriggered whenever a feed is modified.

In the meantime, there is a simple fix for the jank - just remove all direct feeds, however, this is obviously not ideal.

Steps to Reproduce

  1. Subscribe to a new direct feed from the new tab page. I've been using https://reactjs.org/.
    image
  2. Press the Add Feed button. Observe that the browser chrome is frozen while the feed is adding.
  3. Now you are subscribed to the direct feed all of the following actions will trigger a freeze:
    • Refreshing the feed
    • Unsubscribing from a publisher
    • Subscribing to a publisher
    • Opening the new tab page for the first time in a window (this is when BraveNews loads the initial list of publishers/feeds).
    • Any other call to PublishersController::EnsurePublishersIsUpdating

Actual result:

Interacting with Brave News causes small freezes of about 3s with each refresh/subscribe/unsubscribe.

Expected result:

Interacting with Brave News does not cause the Browser UI to freeze.

Reproduces how often:

Every time. Tested in Dev, Nightly, Beta

Brave version (brave://version info)

1.41.86 Chromium: 103.0.5060.66 (Official Build) beta (64-bit)
1.43.3 Chromium: 103.0.5060.114 (Official Build) nightly (64-bit)
1.43.2 Chromium: 103.0.5060.114 (Developer Build) (64-bit)

Version/Channel Information:

  • Can you reproduce this issue with the current release? Not tested, but probably
  • Can you reproduce this issue with the beta channel? Yes
  • Can you reproduce this issue with the nightly channel? Yes

Other Additional Information:

  • Is the issue reproducible on the latest version of Chrome?
    No. This is a Brave only issue.

Miscellaneous Information:

You can remove all direct feeds, which will minimise the issue. However, I believe there's still jank, it's just smaller/less noticeable. In addition, I would expect this to be much, much worse with a bad internet connection as the jank is while waiting for a network response.

Screencast from 07-07-22 11:14:22.webm
Note: In the screencast you can't see my mouse but I'm clicking back and forward on the tab buttons, which is why the OS throws up the "Application is not responding" dialog. It doesn't normally do this. You can see that once I remove the direct feed I can swap back and forward normally.

@fallaciousreasoning fallaciousreasoning added priority/P2 A bad problem. We might uplift this to the next planned release. OS/Desktop feature/brave-news formerly brave-today labels Jul 6, 2022
@fallaciousreasoning
Copy link
Author

@petemill I think this is kinda serious, but I don't know how many people use direct feeds. Maybe we should disable it?

@fallaciousreasoning
Copy link
Author

I might have been on the wrong track with this - it looks like its actually CPU bound (parsing the RSS feed). When you toggle a subscription the CPU for the browser process gets maxed out
image

@fallaciousreasoning
Copy link
Author

@fallaciousreasoning
Copy link
Author

fallaciousreasoning commented Jul 7, 2022

I've narrowed this down a bit more - seems to be the call to parse_feed_string in DirectFeedController::OnResponse which is causing this.

This seems to be much slower than it should be. For example, parsing the 360k body from here takes more than 3 seconds. In addition, parsing the same document from JavaScript takes 7ms using the following snippet of code in the console:

const timeIt = () => {
    const start = performance.now();
    const text = document.body.outerHTML;
    const parser = new DOMParser();
    doc = parser.parseFromString(text, 'application/xml');

    const end = performance.now();
    console.log("Parsing took", end - start);
}
timeIt(); // Parsing took 7.4

It's not exactly equivalent (just parsing the body tag, and it's tweaked a bit by the browser), but the content length is actually longer (~700k instead of 360k). Loading the page takes significantly less time than 3s, including talking to the server.

@fallaciousreasoning
Copy link
Author

Test Plan

  1. Open the New Tab Page
  2. Click the Customise button
  3. Go to the Brave News tab
  4. Add a new direct feed (I use https://reactjs.org/)
  5. Try to keep scrolling up and down the dialog.

If the browser freezes up / is janky for a few seconds, the issue is not fixed. If scrolling is normal, the issue is better :)

@stephendonner
Copy link

@fallaciousreasoning this looks amazingly similar to #21490; can you confirm?

@fallaciousreasoning
Copy link
Author

fallaciousreasoning commented Jul 14, 2022

Yup, that sounds exactly like this. Shall I close it as a duplicate?

@stephendonner
Copy link

Yup, that sounds exactly like this. Shall I close it as a duplicate?

Yes, thanks.

@stephendonner
Copy link

Verified PASSED using

Brave 1.43.26 Chromium: 103.0.5060.114 (Official Build) nightly (x86_64)
Revision a1c2360c5b02a6d4d6ab33796ad8a268a6128226-refs/branch-heads/5060@{#1124}
OS macOS Version 13.0 (Build 22A5295i)

Steps:

  1. installed 1.43.26
  2. launched Brave
  3. opened a new-tab page
  4. clicked on Customize on the bottom right
  5. clicked on Brave News
  6. entered (one at a time)https://reactjs.org, https://planet.mozilla.org/rss10.xml, and https://planet.mozilla.org/rss20.xml
  7. confirmed I could add and remove each feed source without beach-balling/killing the main thread. No input / scrolling events were blocked

rss-feeds

@fallaciousreasoning
Copy link
Author

Cheers, thanks @stephendonner!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/brave-news formerly brave-today OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass-macOS QA/Test-Plan-Specified QA/Yes release-notes/exclude
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants