Use the proper index when dragging a tab group #25160

Ilie-Lesan · 2024-08-16T05:03:17Z

Resolves brave/brave-browser#39486

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
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 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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

TabDragControllerTest.*
See the linked issue for manual test.

Ilie-Lesan · 2024-08-22T05:39:29Z

@sangwoo108 Can you have a look. Thanks!

simonhong · 2024-09-15T07:08:33Z

@tech-zilla Thanks for submitting.. I think this is the right fix.
As first item in drag_data_ is group header when detaching entire tab group,
we should iterate from first_tab_index() instead of 0. 👍🏼
Can you add test code for this also?

Ilie-Lesan · 2024-09-16T17:43:57Z

@tech-zilla Thanks for submitting.. I think this is the right fix. As first item in drag_data_ is group header when detaching entire tab group, we should iterate from first_tab_index() instead of 0. 👍🏼 Can you add test code for this also?

@simonhong Thanks for the review. I will try to find some spare time for a test.

Ilie-Lesan · 2024-09-30T00:33:14Z

@simonhong I've added an interactive UI test for this crash and fixed the interactive_ui_tests. Please have a look to see if everything is ok.

simonhong · 2024-09-30T03:49:04Z

@Ilie-Lesan Thanks for adding test.
We've not been running that test target on CI so far so interactive ui test seems broken now.
I think our CI we should run that target also in the future.
Before that, I think we should use browser test target.
Can you add your test code to browser test target instead as we don't run interactive target now?

simonhong · 2024-09-30T03:50:33Z

and if you want to fix broken interactive ui test taget build as a separated issue, it's really welcome.

bsclifton · 2024-09-30T06:04:16Z

browser/ui/views/tabs/tab_drag_controller.cc

@@ -187,9 +187,9 @@ void TabDragController::DetachAndAttachToNewContext(
    std::vector<tabs::TabHandle> tabs;
    auto* tab_strip_model = browser->tab_strip_model();
    DCHECK_EQ(tab_strip_model, attached_context_->GetTabStripModel());
-    for (const auto& tab_drag_data : drag_data_) {
+    for (size_t i = first_tab_index(); i < drag_data_.size(); ++i) {


Nice- took me a minute to spot the fix 😄 Basically starting at first_tab_index and not iterating every item

Please change this to something like:

auto drag_data = base::span(drag_data_).subspan(first_tab_index()); for (const auto& tab_drag_data : drag_data) { ...

This will avoid build failures with the unsafe buffers warning as we don't access the subscript directly.

Oops - I missed this before merging; @Ilie-Lesan you're welcome to do a follow up or I can do that. Just let me know!

Pushed #26055

bsclifton

Code change looks great! Verified the fix in local build. Nice work 😄

+1 from me on the comments from @simonhong (RE: browser test would be great)

Ilie-Lesan · 2024-09-30T11:13:59Z

Code change looks great! Verified the fix in local build. Nice work 😄

+1 from me on the comments from @simonhong (RE: browser test would be great)

I will try to add a browser test(let's hope this can be done using the browser test target). Thanks for your time! :)

bsclifton · 2024-09-30T16:23:46Z

Code change looks great! Verified the fix in local build. Nice work 😄
+1 from me on the comments from @simonhong (RE: browser test would be great)

I will try to add a browser test(let's hope this can be done using the browser test target). Thanks for your time! :)

You should be able to try it out via npm run test -- brave_browser_tests --filter="ClassNameOfTest.Testname". Basically, our CI is running npm run test -- brave_unit_tests and npm run test -- brave_browser_tests

Ilie-Lesan · 2024-10-05T11:22:05Z

@simonhong I've added a browser test for the tab group drag issue. Let me know if everything is ok

bsclifton · 2024-10-15T22:47:17Z

Rebased and created a CI run here: #26018

simonhong · 2024-10-16T03:24:44Z

checking the CI failures. My last commit didn't fix. Got same result after adding tab_strip->StopAnimating(true);
Disabled this test on macOS & Linux. Upstream also runs group detach test on Windows due to its flaky.

…window when split view is enabled and fix interactive ui test build

… of the window when split view is enabled and fix interactive ui test build" This reverts commit 40ed7c1.

upstream also only runs this test on Windows. Also use //brave/browser/ui/views/tabs::browser_tests target for tab browser test cases.

simonhong · 2024-10-16T13:38:56Z

@bsclifton #26018 got all greens.
I think this PR can be merged but needs admin power.

cdesouza-chromium

LGTM % changes.

Thanks for submitting this PR.

bsclifton · 2024-10-16T15:51:39Z

Nice work @Ilie-Lesan! Thanks for putting in the time on this one 😄

brave-builds · 2024-10-16T16:04:53Z

Released in v1.73.18

Ilie-Lesan · 2024-10-16T17:40:31Z

@bsclifton You're welcome, thanks too! :)

issue: brave/brave-browser#39486 f/u PR for #25160

bsclifton assigned Ilie-Lesan Aug 22, 2024

bsclifton self-requested a review August 30, 2024 20:22

simonhong self-requested a review September 13, 2024 08:47

Ilie-Lesan requested a review from a team as a code owner September 30, 2024 00:28

bsclifton reviewed Sep 30, 2024

View reviewed changes

bsclifton approved these changes Sep 30, 2024

View reviewed changes

bsclifton force-pushed the browser-crashed-while-dragging-tab-group-out-of-current-window branch from ecd32a8 to 890a61a Compare October 15, 2024 22:43

brave-builds mentioned this pull request Oct 15, 2024

CI run for contributor PR #25160 #26018

Draft

simonhong removed the request for review from a team October 16, 2024 01:07

Ilie-Lesan added 4 commits October 16, 2024 13:40

Use the proper index when dragging a tab group

b37cceb

Add a test for the scenario when a user drags a tab group out of the …

3c3f49d

…window when split view is enabled and fix interactive ui test build

Revert "Add a test for the scenario when a user drags a tab group out…

1474d64

… of the window when split view is enabled and fix interactive ui test build" This reverts commit 40ed7c1.

Add a browser test for tab group drag issue

81c1d26

simonhong force-pushed the browser-crashed-while-dragging-tab-group-out-of-current-window branch from 971c077 to 2543edb Compare October 16, 2024 06:08

simonhong approved these changes Oct 16, 2024

View reviewed changes

Disabled group detach test on macOS and Linux

60990c1

upstream also only runs this test on Windows. Also use //brave/browser/ui/views/tabs::browser_tests target for tab browser test cases.

simonhong force-pushed the browser-crashed-while-dragging-tab-group-out-of-current-window branch from 2543edb to 60990c1 Compare October 16, 2024 07:42

cdesouza-chromium approved these changes Oct 16, 2024

View reviewed changes

bsclifton merged commit 2724f43 into brave:master Oct 16, 2024
6 of 7 checks passed

bsclifton added this to the 1.73.x - Nightly milestone Oct 16, 2024

simonhong added a commit that referenced this pull request Oct 17, 2024

Use spanification to avoid unsafe buffer operations

04dfaab

issue: brave/brave-browser#39486 f/u PR for #25160

simonhong mentioned this pull request Oct 17, 2024

Use spanification to avoid unsafe buffer operations #26055

Merged

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the proper index when dragging a tab group #25160

Use the proper index when dragging a tab group #25160

Ilie-Lesan commented Aug 16, 2024 •

edited by simonhong

Loading

Ilie-Lesan commented Aug 22, 2024

simonhong commented Sep 15, 2024

Ilie-Lesan commented Sep 16, 2024

Ilie-Lesan commented Sep 30, 2024

simonhong commented Sep 30, 2024 •

edited

Loading

simonhong commented Sep 30, 2024

bsclifton Sep 30, 2024 •

edited

Loading

cdesouza-chromium Oct 16, 2024 •

edited

Loading

bsclifton Oct 16, 2024

simonhong Oct 17, 2024

bsclifton left a comment

Ilie-Lesan commented Sep 30, 2024 •

edited

Loading

bsclifton commented Sep 30, 2024

Ilie-Lesan commented Oct 5, 2024

bsclifton commented Oct 15, 2024

simonhong commented Oct 16, 2024 •

edited

Loading

simonhong commented Oct 16, 2024

cdesouza-chromium left a comment

bsclifton commented Oct 16, 2024

brave-builds commented Oct 16, 2024

Ilie-Lesan commented Oct 16, 2024

Use the proper index when dragging a tab group #25160

Use the proper index when dragging a tab group #25160

Conversation

Ilie-Lesan commented Aug 16, 2024 • edited by simonhong Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Ilie-Lesan commented Aug 22, 2024

simonhong commented Sep 15, 2024

Ilie-Lesan commented Sep 16, 2024

Ilie-Lesan commented Sep 30, 2024

simonhong commented Sep 30, 2024 • edited Loading

simonhong commented Sep 30, 2024

bsclifton Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

cdesouza-chromium Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

bsclifton Oct 16, 2024

Choose a reason for hiding this comment

simonhong Oct 17, 2024

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

Ilie-Lesan commented Sep 30, 2024 • edited Loading

bsclifton commented Sep 30, 2024

Ilie-Lesan commented Oct 5, 2024

bsclifton commented Oct 15, 2024

simonhong commented Oct 16, 2024 • edited Loading

simonhong commented Oct 16, 2024

cdesouza-chromium left a comment

Choose a reason for hiding this comment

bsclifton commented Oct 16, 2024

brave-builds commented Oct 16, 2024

Ilie-Lesan commented Oct 16, 2024

Ilie-Lesan commented Aug 16, 2024 •

edited by simonhong

Loading

simonhong commented Sep 30, 2024 •

edited

Loading

bsclifton Sep 30, 2024 •

edited

Loading

cdesouza-chromium Oct 16, 2024 •

edited

Loading

Ilie-Lesan commented Sep 30, 2024 •

edited

Loading

simonhong commented Oct 16, 2024 •

edited

Loading