New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 672134 link

Starred by 1 user
Status: Available
Owner:
msarda@chromium.org
Cc:
█ bmcquade@chromium.org
jam@chromium.org
msarda@chromium.org
sky@chromium.org
█ mlerman@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Write test and add CHECKs to ensure we don't attempt to load profiles too eary

Project Member Reported by sky@chromium.org, Dec 7 2016 
671579 seems to be the result of some code trying to load profiles before we've gotten to the point where we should load profiles. Specifically we're showing a dialog saying the profile is in use and because displaying the dialog is a nested message loop we process a task that ends up loading the profile and we're not in a very weird state. We should have tests to exercise this path.

An alternative is to move showing of dialogs to a separate thread/process so that we can block main and be sure we do not run a nested message loop and pump random tasks we don't really want to.

Without tests we're likely to regress this again and end up with weird crashes. I'm making this a P1 and randomly picking an OWNER of profiles (sorry anthony).

Comment 1 by anthonyvd@chromium.org, Dec 7 2016

Cc: mlerman@chromium.org sky@chromium.org
Status: Assigned (was: Untriaged)
I'm not sure I'm familiar enough with the intricacies of the profile init code to fully grasp what we want the tests to assert. Can you elaborate a bit more? Maybe a test that spins up/shuts down a browser in the "profile already in use" state and just checks if no crashes happen?

Looking at Issue 671579 I think I understand that the issue was using the profile pointer too early (before PostProfileInit perhaps?). What's the check we want to put in place there that would prevent other components from triggering this?

I've never seen this dialog, any idea what code triggers it so I can look into it?

I don't mind tackling this, just trying to get an idea of what you have in mind :-)

Comment 2 by sky@chromium.org, Dec 7 2016 

I think it would be best if we ensure no profiles are created after the 'profile already in use' dialog is shown.

To add a CHECK we would need to add a global indicating it's safe to create profiles, maybe on browser_process, but that many not be necessary.

Probably easiest to show dialog is modify startup to force it being shown.

Comment 3 by mlerman@chromium.org, Dec 8 2016 

Idea - add a flag to ProfileManager (which is owned by browser_process) that declares whether Profiles are allowed to be created. Defaults to false, only gets set to true by the appropriate StartupBrowser code. Add a CHECK to ProfileManager.CreateProfile code for the value of the flag.

Comment 4 by mlerman@chromium.org, Dec 8 2016

Cc: msarda@chromium.org

Comment 5 by anthonyvd@chromium.org, Apr 18 2017

Owner: msarda@chromium.org
+msarda since I no longer work on Identity.

Comment 6 by msarda@chromium.org, May 17 2017

Components: UI>Browser>Profiles
Labels: -Pri-1 Pri-2
Lowering its priority as I do not have the bandwidth to work on it.

Feel free to pick this bug up if you know the profiles code and believe this bug is higher priority.

Comment 7 by msarda@chromium.org, Oct 15

Status: Available (was: Assigned)
I do not know this code well, so I am not certain what needs to be done here. Moving to available - feel free to pick it up if you have cycles to work on it.
►

Sign in to add a comment