New issue
Advanced search Search tips

Issue 852997 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Disallow reentrancy into TabStripModel.

Project Member Reported by erikc...@chromium.org, Jun 14 2018

Issue description

This causes subtle, difficult to find bugs. By adding an explicit CHECK, these problems will be easier to diagnose.

Example 1: https://bugs.chromium.org/p/chromium/issues/detail?id=851400
Example 2: http://crbug.com/529407
 
Plan: Add DCHECKs to prevent re-entrancy. Let that sit for a while. If albatross [DCHECK in wild] doesn't reveal any issues after, let's say 6 weeks, we can change the DCHECKs to CHECKs.
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aaa348eb3f97c273c4b831b8631b87692e67aef4

commit aaa348eb3f97c273c4b831b8631b87692e67aef4
Author: erikchen <erikchen@chromium.org>
Date: Fri Jun 15 22:18:01 2018

Add DCHECKs to TabStripModel to prevent re-entrancy.

TabStripModel is not re-entrant safe. Re-entrancy causes subtle bugs. This will
make it easier to catch those bugs.

Bug: 852997
Change-Id: I70e106b6a3c6f130afda14c493f17f97c8fe26ff
Reviewed-on: https://chromium-review.googlesource.com/1101844
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567812}
[modify] https://crrev.com/aaa348eb3f97c273c4b831b8631b87692e67aef4/chrome/browser/ui/tabs/tab_strip_model.cc
[modify] https://crrev.com/aaa348eb3f97c273c4b831b8631b87692e67aef4/chrome/browser/ui/tabs/tab_strip_model.h

Cc: w...@chromium.org
wez: I'm waiting for albatross to check for DCHECKs in the field before turning this into a CHECK. How long should I wait?

Comment 4 by w...@chromium.org, Jun 19 2018

Components: UI>Browser>TabStrip
Re #3: We push a fresh Albatross Canary daily, so if you know the signature you're expecting, you should be able to see it now, if it is firing in the wild.
> so if you know the signature you're expecting, you should be able to see it now, if it is firing in the wild.

I don't know what the signature would be...but I expect the DCHECKs I added to fire [or not]. How can I confirm? I'm expecting bugs to be filed against me?
Labels: Hotlist-DesktopUIToolingRequired Hotlist-DesktopUIChecked
*** UI Mass Triage ****

Sign in to add a comment