[Bug] Crash during first run if Chrome is started in incognito mode |
|||||||||
Issue descriptionReported over email by a developer working on the Vivaldi browser: ------------------------------------------------------------------------------------- I am one of the developers of the Vivaldi browser, which is based on the Chromium project, and I am presently upgrading our source code to v62. As part of our testing we encountered a crash in StartupTabProviderImpl::GetOnboardingTabs which seems to have been introduced by your patch <https://chromium-review.googlesource.com/620752> The reason for the crash is that, apparently, signin_manager can be NULL when the application is being started in incognito mode (using "--incognito" on the commandline), and possibly other cases as well. At present, I will be adding NULL pointer checks to our branch, but I suggest you add these to the v62 and and master branches in the main repository, as well. ------------------------------------------------------------------------------------- Notes: I am marking this as RBS as it leads to crashes for our users and it is hard for me to estimate how often this may appear on stable. The fix should be trivial.
,
Sep 27 2017
,
Sep 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3d657e127e0350bcabb74c12031be4c6e1fc15d8 commit 3d657e127e0350bcabb74c12031be4c6e1fc15d8 Author: Mihai Sardarescu <msarda@chromium.org> Date: Fri Sep 29 16:55:39 2017 Return empty startup tabs for incognito profile. StartupTabProvider may be called when Chrome starts in an incognito profile, which leads to a crash at start-up. This CL addresses this and adds a unit test to make sure it does not regress in the future. Bug: 769241 Change-Id: Idea489bc05e31d7ac693cb935b1481979099bd09 Reviewed-on: https://chromium-review.googlesource.com/689535 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Mihai Sardarescu <msarda@chromium.org> Cr-Commit-Position: refs/heads/master@{#505395} [modify] https://crrev.com/3d657e127e0350bcabb74c12031be4c6e1fc15d8/chrome/browser/ui/startup/startup_tab_provider.cc [modify] https://crrev.com/3d657e127e0350bcabb74c12031be4c6e1fc15d8/chrome/browser/ui/startup/startup_tab_provider_unittest.cc
,
Sep 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b242320c5d00898ab616b00aa6e98ce3569274e8 commit b242320c5d00898ab616b00aa6e98ce3569274e8 Author: Devlin <rdevlin.cronin@chromium.org> Date: Fri Sep 29 19:29:32 2017 Revert "Return empty startup tabs for incognito profile." This reverts commit 3d657e127e0350bcabb74c12031be4c6e1fc15d8. Reason for revert: New unittest fails: [ RUN ] StartupTabProviderTest.IncognitoProfile ../../chrome/browser/ui/startup/startup_tab_provider_unittest.cc(453): error: Value of: StartupTabProviderImpl().GetOnboardingTabs(incognito).empty() Actual: false Expected: true [ FAILED ] StartupTabProviderTest.IncognitoProfile (16 ms) https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds/16448 Original change's description: > Return empty startup tabs for incognito profile. > > StartupTabProvider may be called when Chrome starts in an incognito profile, > which leads to a crash at start-up. This CL addresses this and adds a unit > test to make sure it does not regress in the future. > > Bug: 769241 > Change-Id: Idea489bc05e31d7ac693cb935b1481979099bd09 > Reviewed-on: https://chromium-review.googlesource.com/689535 > Reviewed-by: Scott Violet <sky@chromium.org> > Commit-Queue: Mihai Sardarescu <msarda@chromium.org> > Cr-Commit-Position: refs/heads/master@{#505395} TBR=sky@chromium.org,msarda@chromium.org Change-Id: I70ea9825d6dab8084292b40fa2d36d65488ff2e7 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 769241 Reviewed-on: https://chromium-review.googlesource.com/692566 Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#505440} [modify] https://crrev.com/b242320c5d00898ab616b00aa6e98ce3569274e8/chrome/browser/ui/startup/startup_tab_provider.cc [modify] https://crrev.com/b242320c5d00898ab616b00aa6e98ce3569274e8/chrome/browser/ui/startup/startup_tab_provider_unittest.cc
,
Oct 5 2017
msarda@, do we have any further updates on the fix? since this is marked as M62 RB-Stable. Thank you!
,
Oct 5 2017
I fixed it a week ago but the CL got reverted due to a stupid bug in the test. I have another CL https://chromium-review.googlesource.com/c/chromium/src/+/695406 in review but this review took longer than expected. I think the CL should be LGTMed now, so I hope to land it soon tomorrow.
,
Oct 5 2017
Sure, thank you for the update.
,
Oct 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/71b25bc67d18d8b6503c7c62bdf4620cf9737651 commit 71b25bc67d18d8b6503c7c62bdf4620cf9737651 Author: Mihai Sardarescu <msarda@chromium.org> Date: Fri Oct 06 10:47:38 2017 Reland: Return empty startup tabs for incognito profile. StartupTabProvider may be called when Chrome starts in an incognito profile, which leads to a crash at start-up. This CL addresses this and adds a unit test to make sure it does not regress in the future. This CL relands https://chromium-review.googlesource.com/689535 (and fixes the unit test on Windows 10). Bug: 769241 Change-Id: I99b7d41b3bad21678b11d9358a195c15cc20ee25 Reviewed-on: https://chromium-review.googlesource.com/695406 Commit-Queue: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#507026} [modify] https://crrev.com/71b25bc67d18d8b6503c7c62bdf4620cf9737651/chrome/browser/ui/startup/startup_tab_provider.cc [modify] https://crrev.com/71b25bc67d18d8b6503c7c62bdf4620cf9737651/chrome/browser/ui/startup/startup_tab_provider_unittest.cc
,
Oct 6 2017
,
Oct 6 2017
[Auto-generated comment by a script] We noticed that this issue is targeted for M-62; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-62 label, otherwise remove Merge-TBD label. Thanks.
,
Oct 6 2017
The fix is risk free (it also has a unit test) and we should merge to M62.
,
Oct 6 2017
This bug requires manual review: We are only 10 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 9 2017
msarda@, are you requesting a merge for CL listed at #8? If yes, is this change well baked/verified in Canary?
,
Oct 9 2017
,
Oct 9 2017
Tested on Canary today and the crash is gone, so the fix looks good.
,
Oct 9 2017
Approving merge to M62 branch 3202 based on comment #11 and #15. Please merge ASAP. Thank you.
,
Oct 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c49b23e5bc191cb2fcda8f80e25402e9b660b772 commit c49b23e5bc191cb2fcda8f80e25402e9b660b772 Author: Mihai Sardarescu <msarda@chromium.org> Date: Mon Oct 09 18:27:43 2017 Reland: Return empty startup tabs for incognito profile. StartupTabProvider may be called when Chrome starts in an incognito profile, which leads to a crash at start-up. This CL addresses this and adds a unit test to make sure it does not regress in the future. This CL relands https://chromium-review.googlesource.com/689535 (and fixes the unit test on Windows 10). Bug: 769241 Change-Id: I99b7d41b3bad21678b11d9358a195c15cc20ee25 Reviewed-on: https://chromium-review.googlesource.com/695406 Commit-Queue: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#507026}(cherry picked from commit 71b25bc67d18d8b6503c7c62bdf4620cf9737651) Reviewed-on: https://chromium-review.googlesource.com/707115 Reviewed-by: Boris Sazonov <bsazonov@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#615} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/c49b23e5bc191cb2fcda8f80e25402e9b660b772/chrome/browser/ui/startup/startup_tab_provider.cc [modify] https://crrev.com/c49b23e5bc191cb2fcda8f80e25402e9b660b772/chrome/browser/ui/startup/startup_tab_provider_unittest.cc |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by msarda@chromium.org
, Sep 27 2017