New issue
Advanced search Search tips

Issue 913709 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Bug

Blocking:
issue 913079



Sign in to add a comment

Chrome_iOS: Crash Report - -[LocationBarCoordinator locationBarHasResignedFirstResponder]

Project Member Reported by thegreenfrog@chromium.org, Dec 10

Issue description

Blocking: -913065 913079
The code when the app is trying to reload current web state during shut down. Is it possible to avoid this reload?
Components: -UI>Browser UI>Browser>Omnibox
Labels: ReleaseBlock-Stable
This represents half crashes from crbug.com/913079. Tentatively adding RBS.
Project Member

Comment 3 by sheriffbot@chromium.org, 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
Labels: M-72
Tentatively M-72
Project Member

Comment 5 by sheriffbot@chromium.org, 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
Labels: OS-iOS
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.
Labels: Needs-Feedback
Cc: -eugene...@chromium.org stkhapugin@chromium.org
Owner: eugene...@chromium.org
I'm going OOO for two weeks, Eugene, please take a look, maybe you can fix this before I'm back? 
Cc: kkhorimoto@chromium.org
Owner: edchin@chromium.org
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.
Yes, BVC has a shutdown method and also an ivar _isShutdown. 
Status: Started (was: Assigned)
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
This will need to be cherry-picked to M72. I will verify fix on canary tomorrow. 
Labels: Merge-TBD
[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.
Labels: Merge-Request-72
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. 

Project Member

Comment 18 by sheriffbot@chromium.org, Dec 29

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
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
Labels: -Hotlist-Merge-Review -Merge-TBD -Merge-Review-72 Merge-Approved-72
Ok thanks for explanation. Approved. Please merge asap so we have more time test this out in beta.
Having a merge conflict. Will try to resolve on the branch. 
Project Member

Comment 21 by bugdroid1@chromium.org, Jan 4

Labels: -merge-approved-72 merge-merged-3626
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

Labels: Merge-Merged-72-3626
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}
This has been merged to M72.
As there are no steps to reproduce this crash, we will keep monitoring this crash for M72 Beta.

Sign in to add a comment