Added checks on shields v2 related classes #12927

nullhook · 2022-04-07T00:07:45Z

Resolves brave/brave-browser#22031

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, npm run lint, 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:

simonhong · 2022-04-07T00:14:13Z

browser/ui/views/brave_actions/brave_shields_action_view.cc

@@ -215,6 +216,14 @@ void BraveShieldsActionView::OnTabStripModelChanged(
    TabStripModel* tab_strip_model,
    const TabStripModelChange& change,
    const TabStripSelectionChange& selection) {
+
+  if (change.type() == TabStripModelChange::kRemoved) {


I assume that this kRemoved noti will be sent after we receive active tab changed for that tab first.
Can you check this?

I think noti will be like below?

Closing non active tab - maybe Removed is only sent.

Closing active tab - maybe we get active tab changing noti. and then kRemoved will be arrived.

If so, I think handling kRemoved is not needed here.

The flow is as below:

Non active tab: removed is only fired

Active tab:
[27785:259:0406/172038.657028:ERROR:brave_shields_action_view.cc(221)] removed
[27785:259:0406/172038.668865:ERROR:brave_shields_action_view.cc(229)] active tab

Ok, in that case selection.old_contents is null or removed one?

selection.old_contents is the removed one. It's not null.

If BraveShieldsActionView is destroyed first before tab strip model handles all tabs active state while shutdown,
we should do call RemoveObserver for current active tab in dtor.

WillCloseAllTabs is called first and then dtor of BraveShieldsActionView - so, tab strip model handles all tabs first and then dtor gets called

as discussed in DM this is an issue with the view closing before the active tab. We originally discussed doing this in the destructor and then I got off track and forgot about the original issue when we looked at the tab strip observer stuff. I think both changes are necessary.

implemented the dtor based on our discussion in DM

@simonhong I actually don't think it should be possible for the view to be destroyed before the webcontents because it's the button in the urlbar, but I think it's still best to add it to the dtor in case we're missing something in the tab strip observing

simonhong

How about adding more defence code?
and it seems you need to check the result of npm run lint

browser/ui/views/brave_actions/brave_shields_action_view.cc

simonhong · 2022-04-07T14:25:36Z

browser/ui/webui/brave_shields/shields_panel_data_handler.cc

@@ -121,6 +121,8 @@ ShieldsPanelDataHandler::GetActiveShieldsDataController() {
  DCHECK(profile);

  Browser* browser = chrome::FindLastActiveWithProfile(profile);
+  if (!browser) return nullptr;


How about handling like below on all caller?

auto* shields_data_ctrlr = GetActiveShieldsDataController(); if (!shields_data_ctrlr) return;

@bridiver suggested to add a check on the browser

do we really need a check on all callers if we're doing a check that can be caught in debug mode?

simonhong · 2022-04-07T14:27:01Z

browser/brave_shields/brave_shields_web_contents_observer.cc

-        ->HandleItemBlocked(block_type, subresource);
-#endif
+    auto* shields_data_ctrlr = brave_shields::BraveShieldsDataController::FromWebContents(web_contents);
+    DCHECK(shields_data_ctrlr);


@bridiver wanted this DCHECK

added the check

@simonhong a WebContentsUserData should never be destroyed before the webcontents. The only time this could be null is if the webcontents is not associated with a tab, but that seems like it would be a bug in the networking code and hence the DCHECK here

Ok, I was worried some WebContents that not associated with tab might be reached here.

@bridiver btw, curious how networking code is tied with tab's webcontents. can you point out where it is?

bridiver · 2022-04-07T16:51:24Z

browser/ui/views/brave_actions/brave_shields_action_view.cc

  if (selection.active_tab_changed()) {
    if (selection.new_contents) {
      brave_shields::BraveShieldsDataController::FromWebContents(
          selection.new_contents)
          ->AddObserver(this);
+      active_web_contents_ = selection.new_contents->GetWeakPtr();


if new_contents is null we should set active_web_contents_ = base::WeakPtr<WebContents>()

bridiver · 2022-04-07T16:58:13Z

browser/ui/views/brave_actions/brave_shields_action_view.cc

+  // and if that's true, the last registered web_contents won't unregister
+  // itself. By keeping a cache of last active web contents we ensure to
+  // unregister from this edge case.
+  if (active_web_contents_) {


on second thought I think this should be DCHECK(!active_web_contents_) since a toolbar button should outlive the webcontents in the tab strip

petemill · 2022-04-13T23:28:45Z

browser/ui/webui/brave_shields/shields_panel_data_handler.cc

@@ -121,6 +121,9 @@ ShieldsPanelDataHandler::GetActiveShieldsDataController() {
  DCHECK(profile);

  Browser* browser = chrome::FindLastActiveWithProfile(profile);


I'm wondering if it would be better / possible to get the Browser that the webui_controller_ is already associated with via FindBrowserWithWebContents but maybe that only works for WebContents on the TabStrip, and not bubbles. Although if we have access to the tab strip, perhaps we could provide it directly to the controller.

solved here: #12984 - we get the right browser in ShieldsPanelUI as PanelHandler::CreatePanel is async and it might get the wrong browser

github-actions bot assigned nullhook Apr 7, 2022

bridiver approved these changes Apr 7, 2022

View reviewed changes

bridiver requested review from mariospr, simonhong and petemill April 7, 2022 00:09

simonhong reviewed Apr 7, 2022

View reviewed changes

nullhook requested a review from simonhong April 7, 2022 12:13

simonhong reviewed Apr 7, 2022

View reviewed changes

nullhook force-pushed the shields_panel_fixes branch 2 times, most recently from e4e7112 to 46fc972 Compare April 7, 2022 15:04

nullhook requested a review from bridiver April 7, 2022 15:07

bridiver reviewed Apr 7, 2022

View reviewed changes

nullhook force-pushed the shields_panel_fixes branch from 38cc5e1 to e528181 Compare April 7, 2022 20:44

Added checks on shields v2 related classes

4781eb5

nullhook force-pushed the shields_panel_fixes branch from e528181 to 4781eb5 Compare April 7, 2022 20:46

nullhook requested review from bridiver and simonhong April 7, 2022 20:46

bridiver approved these changes Apr 7, 2022

View reviewed changes

simonhong approved these changes Apr 7, 2022

View reviewed changes

nullhook merged commit 602e27c into master Apr 8, 2022

nullhook deleted the shields_panel_fixes branch April 8, 2022 06:53

nullhook added this to the 1.39.x - Nightly milestone Apr 8, 2022

nullhook mentioned this pull request Apr 8, 2022

Added checks on shields v2 related classes #12948

Merged

7 tasks

petemill reviewed Apr 13, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added checks on shields v2 related classes #12927

Added checks on shields v2 related classes #12927

nullhook commented Apr 7, 2022

simonhong Apr 7, 2022

simonhong Apr 7, 2022 •

edited

Loading

nullhook Apr 7, 2022

simonhong Apr 7, 2022 •

edited

Loading

nullhook Apr 7, 2022

simonhong Apr 7, 2022

nullhook Apr 7, 2022 •

edited

Loading

bridiver Apr 7, 2022

nullhook Apr 7, 2022

bridiver Apr 7, 2022

simonhong left a comment •

edited

Loading

simonhong Apr 7, 2022

nullhook Apr 7, 2022

nullhook Apr 7, 2022

simonhong Apr 7, 2022

nullhook Apr 7, 2022

nullhook Apr 7, 2022

bridiver Apr 7, 2022

simonhong Apr 7, 2022

simonhong Apr 7, 2022

bridiver Apr 7, 2022

bridiver Apr 7, 2022

petemill Apr 13, 2022

nullhook Apr 14, 2022 •

edited

Loading

		@@ -121,6 +121,9 @@ ShieldsPanelDataHandler::GetActiveShieldsDataController() {
		DCHECK(profile);

		Browser* browser = chrome::FindLastActiveWithProfile(profile);

Added checks on shields v2 related classes #12927

Added checks on shields v2 related classes #12927

Conversation

nullhook commented Apr 7, 2022

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

simonhong Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullhook Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullhook Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

simonhong Apr 7, 2022 •

edited

Loading

simonhong Apr 7, 2022 •

edited

Loading

nullhook Apr 7, 2022 •

edited

Loading

simonhong left a comment •

edited

Loading

nullhook Apr 14, 2022 •

edited

Loading