Chrome_iOS: Crash Report - -[LocationBarCoordinator locationBarHasResignedFirstResponder] |
|||||||||||||||
Issue descriptionThis is a crash that is a subset of the crashes in crbug.com/913065. It seems like we can half of those bugs by doing something different in [LocationBarCoordinator locationBarHasResignedFirstResponder] if the app is deallocing. What do you think stkhapugin@? https://crash.corp.google.com/browse?q=product_name%3D%27Chrome_iOS%27+AND+expanded_custom_data.ChromeCrashProto.channel%3D%27%27+AND+expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27-%5BCRWWebController+ensureWebViewCreatedWithConfiguration%3A%5D%27+AND+product.Version%3D%2771.0.3578.77%27+AND+EXISTS+%28SELECT+1+FROM+UNNEST%28CrashedStackTrace.StackFrame%29+WHERE+FunctionName%3D%27-%5BLocationBarCoordinator+locationBarHasResignedFirstResponder%5D%27%29&stbtiq=&reportid=&index=0
,
Dec 11
This represents half crashes from crbug.com/913079. Tentatively adding RBS.
,
Dec 14
This issue is marked as a release blocker with no milestone associated. Please add an appropriate milestone. All release blocking issues should have milestones associated to it, so that the issue can tracked and the fixes can be pushed promptly. This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label. All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 14
Tentatively M-72
,
Dec 17
This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label. All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 17
,
Dec 19
I assume we're talking about [BVC locationBarDidResignFirstResponder], specifically this part:
// If a load was cancelled by an omnibox edit, but nothing is loading when
// editing ends (i.e., editing was cancelled), restart the cancelled load.
if (_locationBarEditCancelledLoad) {
_locationBarEditCancelledLoad = NO;
web::WebState* webState = self.tabModel.currentTab.webState;
if (webState && ![self.helper isToolbarLoading:webState])
webState->GetNavigationManager()->Reload(web::ReloadType::NORMAL,
false /* check_for_repost */);
}
Is there any way to know that we're shutting down?
I'm honestly not sure what to do here.
,
Dec 20
,
Dec 21
I'm going OOO for two weeks, Eugene, please take a look, maybe you can fix this before I'm back?
,
Dec 21
I will not be around for a while. Ed, this crash happens because BVC reloads WebState during cookie clearing on shutdown. Do you know if BVC knows that the app is being shut down? We should not reload WebState on shutdown anyway, so if we fix that then the app will stop crashing.
,
Dec 21
Yes, BVC has a shutdown method and also an ivar _isShutdown.
,
Dec 21
,
Dec 21
,
Dec 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/13b74278b669560599283c3ed8ab715a8e727392 commit 13b74278b669560599283c3ed8ab715a8e727392 Author: edchin <edchin@chromium.org> Date: Thu Dec 27 15:26:13 2018 [ios] Merge BVC's |browserStateDestroyed| and |shutdown| |BrowserStateDestroyed| and |shutdown| are only ever called together, so this CL merges their implementation and removes the public API for |BrowserStateDestroyed|. The has the benefit of being able to bookkeep that shutdown is happening (|_isShutdown|) at the beginning of |BrowserStateDestroyed|. This CL also prevents a crash when BVC reloads WebState during cookie clearing on shutdown. The change above allows for preventing WebState reloads if shutdown is happening. Bug: 913709 Change-Id: I0ea3f8a60c4962849198f669d9e5b0e70e8ba979 Reviewed-on: https://chromium-review.googlesource.com/c/1388804 Commit-Queue: edchin <edchin@chromium.org> Reviewed-by: Mark Cogan <marq@chromium.org> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: edchin <edchin@chromium.org> Cr-Commit-Position: refs/heads/master@{#619036} [modify] https://crrev.com/13b74278b669560599283c3ed8ab715a8e727392/ios/chrome/browser/test/perf_test_with_bvc_ios.mm [modify] https://crrev.com/13b74278b669560599283c3ed8ab715a8e727392/ios/chrome/browser/ui/browser_view_controller+private.h [modify] https://crrev.com/13b74278b669560599283c3ed8ab715a8e727392/ios/chrome/browser/ui/browser_view_controller.mm [modify] https://crrev.com/13b74278b669560599283c3ed8ab715a8e727392/ios/chrome/browser/ui/browser_view_controller_unittest.mm [modify] https://crrev.com/13b74278b669560599283c3ed8ab715a8e727392/ios/chrome/browser/ui/main/browser_coordinator.mm [modify] https://crrev.com/13b74278b669560599283c3ed8ab715a8e727392/ios/chrome/browser/ui/main/browser_view_wrangler.mm
,
Dec 27
This will need to be cherry-picked to M72. I will verify fix on canary tomorrow.
,
Dec 27
[Auto-generated comment by a script] We noticed that this issue is targeted for M-72; it appears the fix may have landed after branch point, meaning a merge might be required. The owner of this bug should confirm if a merge is required here. If so, add Merge-Request-72 label and indicate which commits/CLs are to be merged. Otherwise, remove Merge-TBD label. Thanks.
,
Dec 29
I am requesting merge to M72. There aren't any repro steps, but I was not able to repro with various steps related to clearing the cookie and shutting down. This is important because it prevents a crash. The worst that can happen is that the app continues to crash.
,
Dec 29
This bug requires manual review: Less than 27 days to go before AppStore submit on M72 Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 3
Ok thanks for explanation. Approved. Please merge asap so we have more time test this out in beta.
,
Jan 3
Having a merge conflict. Will try to resolve on the branch.
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2191203dbe56e2f75135764aa468fe35e30bedc1 commit 2191203dbe56e2f75135764aa468fe35e30bedc1 Author: edchin <edchin@chromium.org> Date: Fri Jan 04 03:46:25 2019 Merge to M72 [ios] Merge BVC's |browserStateDestroyed| and |shutdown| |BrowserStateDestroyed| and |shutdown| are only ever called together, so this CL merges their implementation and removes the public API for |BrowserStateDestroyed|. The has the benefit of being able to bookkeep that shutdown is happening (|_isShutdown|) at the beginning of |BrowserStateDestroyed|. This CL also prevents a crash when BVC reloads WebState during cookie clearing on shutdown. The change above allows for preventing WebState reloads if shutdown is happening. TBR=eugenebut@chromium.org, marq@chromium.org (cherry picked from commit 13b74278b669560599283c3ed8ab715a8e727392) Bug: 913709 Change-Id: I0ea3f8a60c4962849198f669d9e5b0e70e8ba979 Reviewed-on: https://chromium-review.googlesource.com/c/1388804 Commit-Queue: edchin <edchin@chromium.org> Reviewed-by: Mark Cogan <marq@chromium.org> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: edchin <edchin@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#619036} Reviewed-on: https://chromium-review.googlesource.com/c/1395877 Cr-Commit-Position: refs/branch-heads/3626@{#558} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/2191203dbe56e2f75135764aa468fe35e30bedc1/ios/chrome/browser/test/perf_test_with_bvc_ios.mm [modify] https://crrev.com/2191203dbe56e2f75135764aa468fe35e30bedc1/ios/chrome/browser/ui/browser_view_controller.h [modify] https://crrev.com/2191203dbe56e2f75135764aa468fe35e30bedc1/ios/chrome/browser/ui/browser_view_controller.mm [modify] https://crrev.com/2191203dbe56e2f75135764aa468fe35e30bedc1/ios/chrome/browser/ui/browser_view_controller_unittest.mm [modify] https://crrev.com/2191203dbe56e2f75135764aa468fe35e30bedc1/ios/chrome/browser/ui/main/browser_coordinator.mm [modify] https://crrev.com/2191203dbe56e2f75135764aa468fe35e30bedc1/ios/chrome/browser/ui/main/browser_view_wrangler.mm
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2191203dbe56e2f75135764aa468fe35e30bedc1 Commit: 2191203dbe56e2f75135764aa468fe35e30bedc1 Author: edchin@chromium.org Commiter: edchin@chromium.org Date: 2019-01-04 03:46:25 +0000 UTC Merge to M72 [ios] Merge BVC's |browserStateDestroyed| and |shutdown| |BrowserStateDestroyed| and |shutdown| are only ever called together, so this CL merges their implementation and removes the public API for |BrowserStateDestroyed|. The has the benefit of being able to bookkeep that shutdown is happening (|_isShutdown|) at the beginning of |BrowserStateDestroyed|. This CL also prevents a crash when BVC reloads WebState during cookie clearing on shutdown. The change above allows for preventing WebState reloads if shutdown is happening. TBR=eugenebut@chromium.org, marq@chromium.org (cherry picked from commit 13b74278b669560599283c3ed8ab715a8e727392) Bug: 913709 Change-Id: I0ea3f8a60c4962849198f669d9e5b0e70e8ba979 Reviewed-on: https://chromium-review.googlesource.com/c/1388804 Commit-Queue: edchin <edchin@chromium.org> Reviewed-by: Mark Cogan <marq@chromium.org> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: edchin <edchin@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#619036} Reviewed-on: https://chromium-review.googlesource.com/c/1395877 Cr-Commit-Position: refs/branch-heads/3626@{#558} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Jan 4
This has been merged to M72.
,
Jan 9
As there are no steps to reproduce this crash, we will keep monitoring this crash for M72 Beta. |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by eugene...@chromium.org
, Dec 11