Fix Brave News crash on opt-in after navigating & cleanup settings #16788

fallaciousreasoning · 2023-01-23T01:51:09Z

Resolves brave/brave-browser#27914

Note: I also removed a bunch of unused code I encountered.

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

See issue

fallaciousreasoning · 2023-01-23T01:52:30Z

components/brave_new_tab_ui/components/default/braveToday/customize/FeedCard.tsx

@@ -69,8 +69,8 @@ export default function FeedCard (props: {
  const publisher = usePublisher(props.publisherId)
  const { followed, setFollowed } = usePublisherFollowed(props.publisherId)

-  const backgroundColor = publisher.backgroundColor || getCardColor(publisher.feedSource?.url || publisher.publisherId)
-  const { url: coverUrl, setElementRef } = useLazyUnpaddedImageUrl(publisher.coverUrl?.url, {
+  const backgroundColor = publisher?.backgroundColor || getCardColor(publisher?.feedSource?.url || publisher?.publisherId)


Note: The fix doesn't actually need this but its should prevent future crashes in the same vein

fallaciousreasoning · 2023-01-23T01:52:59Z

components/brave_new_tab_ui/containers/newTab/settings.tsx

@@ -362,19 +359,6 @@ export default class Settings extends React.PureComponent<Props, State> {
                    />
                  ) : null
              }
-              {


This is no longer used, now we've switched completely to Brave News V2

fallaciousreasoning · 2023-01-23T01:53:25Z

components/brave_today/browser/brave_news_controller.cc

@@ -630,6 +630,14 @@ void BraveNewsController::ConditionallyStartOrStopTimer() {
      timer_prefetch_.Start(FROM_HERE, base::Minutes(1), this,
                            &BraveNewsController::Prefetch);
    }
+
+    GetPublishers(base::BindOnce([](BraveNewsController* controller, Publishers publishers) {


This is the actual fix: Push publishers to all listeners on opt-in

I'm curious what would happen if there's no in-memory publisher data and it takes too long for the api request helper in publisher controller to get the publisher data. Shortly, would the NTP crash if the callback here is invoked later than the page loading?

So the actual crash was being caused because the page was asking for some suggestions, and getting them back. However, we hadn't pushed the publishers to the front end at that point, so we had an empty list of publishers, but a bunch of suggestedPublisherIds.

GetSuggestedPublisherIds depends on GetPublishers, so if we get suggestions then the api request for publishers must have completed.

So to answer your question, no 😄

Thanks @fallaciousreasoning ! It can't happen logically.

components/brave_new_tab_ui/components/default/braveToday/index.tsx

sangwoo108 · 2023-01-23T12:35:20Z

components/brave_today/browser/brave_news_controller.cc

@@ -630,6 +630,14 @@ void BraveNewsController::ConditionallyStartOrStopTimer() {
      timer_prefetch_.Start(FROM_HERE, base::Minutes(1), this,
                            &BraveNewsController::Prefetch);
    }
+
+    GetPublishers(base::BindOnce([](BraveNewsController* controller, Publishers publishers) {


I'm curious what would happen if there's no in-memory publisher data and it takes too long for the api request helper in publisher controller to get the publisher data. Shortly, would the NTP crash if the callback here is invoked later than the page loading?

sangwoo108

LGTM 👍

simonhong

++ 👍🏼

brave-builds · 2023-02-05T21:12:16Z

A Storybook has been deployed to preview UI for the latest push

…16788) * Use hook instead of pass through * Remove unused properties * Ensure publishers are pushed when opting in * Fix Storybook

…earn More content in its tab (#17287) Fix Brave News crash on opt-in after navigating & cleanup settings (#16788) * Use hook instead of pass through * Remove unused properties * Ensure publishers are pushed when opting in * Fix Storybook

github-actions bot assigned fallaciousreasoning Jan 23, 2023

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Jan 23, 2023

fallaciousreasoning commented Jan 23, 2023

View reviewed changes

fallaciousreasoning requested review from sangwoo108 and simonhong January 23, 2023 01:54

sangwoo108 reviewed Jan 23, 2023

View reviewed changes

fallaciousreasoning requested a review from sangwoo108 January 23, 2023 20:38

sangwoo108 approved these changes Jan 24, 2023

View reviewed changes

fallaciousreasoning force-pushed the bn-crash branch from 2d75dc0 to c109c7c Compare January 24, 2023 19:38

simonhong approved these changes Jan 25, 2023

View reviewed changes

fallaciousreasoning force-pushed the bn-crash branch 3 times, most recently from 48a9918 to 18d2c5a Compare February 1, 2023 00:27

fallaciousreasoning added 4 commits February 6, 2023 09:01

Use hook instead of pass through

449f7a9

Remove unused properties

6566e77

Ensure publishers are pushed when opting in

47c3461

Fix Storybook

e490a50

fallaciousreasoning force-pushed the bn-crash branch from 18d2c5a to e490a50 Compare February 5, 2023 20:01

fallaciousreasoning enabled auto-merge (squash) February 5, 2023 21:41

fallaciousreasoning merged commit 29335e0 into master Feb 6, 2023

fallaciousreasoning deleted the bn-crash branch February 6, 2023 21:02

github-actions bot added this to the 1.50.x - Nightly milestone Feb 6, 2023

fallaciousreasoning mentioned this pull request Feb 19, 2023

[Uplift] [1.49]: Brave News Customize window is blank after loading Learn More content in its tab #17287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Brave News crash on opt-in after navigating & cleanup settings #16788

Fix Brave News crash on opt-in after navigating & cleanup settings #16788

fallaciousreasoning commented Jan 23, 2023 •

edited

Loading

fallaciousreasoning Jan 23, 2023

fallaciousreasoning Jan 23, 2023

fallaciousreasoning Jan 23, 2023

sangwoo108 Jan 23, 2023

fallaciousreasoning Jan 23, 2023

sangwoo108 Jan 24, 2023

sangwoo108 Jan 23, 2023

sangwoo108 left a comment

simonhong left a comment

brave-builds commented Feb 5, 2023

Fix Brave News crash on opt-in after navigating & cleanup settings #16788

Fix Brave News crash on opt-in after navigating & cleanup settings #16788

Conversation

fallaciousreasoning commented Jan 23, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

fallaciousreasoning Jan 23, 2023

Choose a reason for hiding this comment

fallaciousreasoning Jan 23, 2023

Choose a reason for hiding this comment

fallaciousreasoning Jan 23, 2023

Choose a reason for hiding this comment

sangwoo108 Jan 23, 2023

Choose a reason for hiding this comment

fallaciousreasoning Jan 23, 2023

Choose a reason for hiding this comment

sangwoo108 Jan 24, 2023

Choose a reason for hiding this comment

sangwoo108 Jan 23, 2023

Choose a reason for hiding this comment

sangwoo108 left a comment

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

brave-builds commented Feb 5, 2023

fallaciousreasoning commented Jan 23, 2023 •

edited

Loading