Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780403003:
Removed -[CRWWebDelegate webWillInitiateLoadWithParams:]. (Closed)
|
Created:
3 years, 8 months ago by Eugene But (OOO till 7-30) Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), ios-reviews+web_chromium.org, noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoved -[CRWWebDelegate webWillInitiateLoadWithParams:].
Method implementation was moved into
|webDidUpdateSessionForLoadWithParams:wasInitialNavigation:| which is
also called from -[CRWWebController loadWithParams:]. This is done, because
WebStateObserver will have only one DidStartNavigation callback to avoid extra
API complexity.
This slightly changes the logic of marking the next crash as non-startup crash.
After this change if app crashes during adding the pending entry, the crash will be
marked as a startup crash, which should not make any practical different for the user.
BUG=674991
Review-Url: https://codereview.chromium.org/2780403003
Cr-Commit-Position: refs/heads/master@{#461807}
Committed: https://chromium.googlesource.com/chromium/src/+/72543a7b6abcd436594bbe771ba1e4b39e29df67
Patch Set 1 #Patch Set 2 : Self review #Patch Set 3 : Cleaned up #Patch Set 4 : Actually fixed compilation #Patch Set 5 : Rebased #Patch Set 6 : Fixed compilation (once and for all) #Patch Set 7 : Fixed tests #
Total comments: 7
Patch Set 8 : Addressed review comment #
Messages
Total messages: 48 (31 generated)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
eugenebut@chromium.org changed reviewers: + sdefresne@chromium.org
PTAL current EG tests failures are unrelated
sdefresne@chromium.org changed reviewers: + rohitrao@chromium.org
https://codereview.chromium.org/2780403003/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (left): https://codereview.chromium.org/2780403003/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:1801: [_delegate webWillInitiateLoadWithParams:params]; It looks like this is introducing a functional change. -webWillInitiateLoadWithParams: says that it want to count a crash occurring after loading started as a non-startup crash. However, with this change the method is only called after the load actually started. This mean that if there is a crash between line 1801-1837 (of the old file), then it will now be counted as a startup crash while it was not counted like that before. +rohirao: is this functional change okay?
On 2017/04/03 13:46:01, sdefresne wrote: > https://codereview.chromium.org/2780403003/diff/120001/ios/web/web_state/ui/c... > File ios/web/web_state/ui/crw_web_controller.mm (left): > > https://codereview.chromium.org/2780403003/diff/120001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_web_controller.mm:1801: [_delegate > webWillInitiateLoadWithParams:params]; > It looks like this is introducing a functional change. > -webWillInitiateLoadWithParams: says that it want to count a crash occurring > after loading started as a non-startup crash. However, with this change the > method is only called after the load actually started. This mean that if there > is a crash between line 1801-1837 (of the old file), then it will now be counted > as a startup crash while it was not counted like that before. > > +rohirao: is this functional change okay? Rohit?
https://codereview.chromium.org/2780403003/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (left): https://codereview.chromium.org/2780403003/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:1801: [_delegate webWillInitiateLoadWithParams:params]; Is there any way to avoid making a functional change here? Is there a WebStateDelegate or WebStateObserver method that gets called anywhere near here, or is webDidUpdateSessionForLoadWithParams: the very first hook point that we have? Who calls loadWithParams? Does the caller have an observer method that we can use? If there's no other way to hook in, then this is probably ok, but there's a lot of work between lines 1801-1837. Worst case we'll end up incorrectly flagging crashes as a startup crash loop, which is what the original code was trying to avoid.
Description was changed from ========== Removed -[CRWWebDelegate webWillInitiateLoadWithParams:]. Method implementation was moved into |webDidUpdateSessionForLoadWithParams:wasInitialNavigation:| which is also called from -[CRWWebController loadWithParams:]. BUG=674991 ========== to ========== Removed -[CRWWebDelegate webWillInitiateLoadWithParams:]. Method implementation was moved into |webDidUpdateSessionForLoadWithParams:wasInitialNavigation:| which is also called from -[CRWWebController loadWithParams:]. This is done, because WebStateObserver will have only one DidStartNavigation callback to avoid extra API complexity. This slightly changes the logic of marking the next crash as non-startup crash. After this change if app crashes during adding the pending entry, the crash will be marked as a startup crash, which should not make any practical different for the user. BUG=674991 ==========
https://codereview.chromium.org/2780403003/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (left): https://codereview.chromium.org/2780403003/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:1801: [_delegate webWillInitiateLoadWithParams:params]; On 2017/04/04 01:49:34, rohitrao wrote: > Is there any way to avoid making a functional change here? Is there a > WebStateDelegate or WebStateObserver method that gets called anywhere near here, > or is webDidUpdateSessionForLoadWithParams: the very first hook point that we > have? Who calls loadWithParams? Does the caller have an observer method that > we can use? > > If there's no other way to hook in, then this is probably ok, but there's a lot > of work between lines 1801-1837. Worst case we'll end up incorrectly flagging > crashes as a startup crash loop, which is what the original code was trying to > avoid. Currently there is no hook and the plan was to have only one DidStartNavigation callback to avoid unnecessary complexity. loadWithParams is called for all kinds of URL loads, but for the purpose of this CL it it fair to say that |loadWithParams| will be called once foreground tab is displayed to the user. I would argue that if code crashes between lines 1801-1837 it is no different than a startup crash (at least from the user perspective). WDYT? Updating CL description to reflect this discussion.
rohitrao@chromium.org changed reviewers: + justincohen@chromium.org, pkl@chromium.org
+Peter and Justin to sign off on this, since they're the best OWNERS for stability and safemode. If there's no hook, there's no hook. We did make an explicit decision to set the no-longer-startup flag as early as possible, though, so if we're going to push that back, we should be equally explicit about signing off on the change =)
The comment starting with 'As soon as an URL is loaded,...' explains the reasoning behind why the crash detection reset is placed here. eugenebut@ can you suggest an earlier place to put this that indicates user action to load a url? If not, then this LG. https://codereview.chromium.org/2780403003/diff/120001/ios/chrome/browser/tab... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2780403003/diff/120001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab.mm:1070: if (navUrl.host() != kChromeUINewTabHost) { Do we still need to extract params.url into navUrl?
> The comment starting with 'As soon as an URL is loaded,...' explains the > reasoning behind why the crash detection reset is placed here. eugenebut@ can > you suggest an earlier place to put this that indicates user action to load a > url? > > If not, then this LG. Thanks! That comment is incorrect BTW. Does it mean that |ResetFailedStartupAttemptCount| call should be in DidFinishNavigation callback? :) https://codereview.chromium.org/2780403003/diff/120001/ios/chrome/browser/tab... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2780403003/diff/120001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab.mm:1070: if (navUrl.host() != kChromeUINewTabHost) { On 2017/04/04 16:45:24, justincohen wrote: > Do we still need to extract params.url into navUrl? No. Done. https://codereview.chromium.org/2780403003/diff/120001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab.mm:1073: // As soon as an URL is loaded, a crash shouldn't be counted as a startup Actually this comment is not correct and was not correct. At this point URL is not loaded yet, in fact the load did not even start. Updated the comment.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Putting reset in didFinishNavigation would create even more false positives -- the goal of safe mode is to catch startup crashes. Once the user initiates a load safe mode should be reset. Moving safe mode reset from webWill*Initiate*Load to -webDidUpdateSessionForLoadWithParams will likely increase the number of safe mode false positive crashes. Given the update comment with s/load/initiate, any ideas on an earlier place to put this code, or is this the earliest possible? https://codereview.chromium.org/2780403003/diff/120001/ios/chrome/browser/tab... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2780403003/diff/120001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab.mm:1073: // As soon as an URL is loaded, a crash shouldn't be counted as a startup On 2017/04/04 16:58:15, Eugene But wrote: > Actually this comment is not correct and was not correct. At this point URL is > not loaded yet, in fact the load did not even start. Updated the comment. I think the meaning behind 'load' means initiate load, e.g. user action initiated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/04 17:08:13, justincohen wrote: > Putting reset in didFinishNavigation would create even more false positives -- > the goal of safe mode is to catch startup crashes. Once the user initiates a > load safe mode should be reset. > > Moving safe mode reset from webWill*Initiate*Load to > -webDidUpdateSessionForLoadWithParams will likely increase the number of safe > mode false positive crashes. > > Given the update comment with s/load/initiate, any ideas on an earlier place to > put this code, or is this the earliest possible? > > https://codereview.chromium.org/2780403003/diff/120001/ios/chrome/browser/tab... > File ios/chrome/browser/tabs/tab.mm (right): > > https://codereview.chromium.org/2780403003/diff/120001/ios/chrome/browser/tab... > ios/chrome/browser/tabs/tab.mm:1073: // As soon as an URL is loaded, a crash > shouldn't be counted as a startup > On 2017/04/04 16:58:15, Eugene But wrote: > > Actually this comment is not correct and was not correct. At this point URL is > > not loaded yet, in fact the load did not even start. Updated the comment. > > I think the meaning behind 'load' means initiate load, e.g. user action > initiated. The earliest callback from WebState is WebStatePolicyDecider::ShouldAllowRequest. But it happens after the load is initiated, not before. Adding WebStateObserver::WillInitiateLoadWithParams: just to preserve current behavior does not feel right to me. "will initiate load" callback is only called because the embedder initiated the load, so it feel silly to tell embedder that the load was initiated (embedder already knows about that). If the purpose of ResetFailedStartupAttemptCount is to weed out non-startup crashes, then maybe ResetFailedStartupAttemptCount() should be called after startup? I have hard time understanding why is it important to call ResetFailedStartupAttemptCount() after user initiated the load, but before adding a pending entry. Comment says "Since initiating a url load is a significant source of crashes", but that comment was written before UIWebView and I strongly doubt that it is accurate at this point. I'm I missing something obvious here? If we really want to preserve the current logic, I think we should wrap all NavigationManager::LoadURLWithParams calls in a method that will do 2 things: 1.) reset failed startup attempt count 2.) call LoadURLWithParams So I guess my question is: "why do we want to preserve the current logic, taking into account that it does not weed out non-startup crashes"?
NavigationManager::LoadURLWithParams might work, except there are various places (see bvc) where load is done with [webController loadWithParams:params]; ResetFailedStartupAttemptCount should be called after startup, but that means after any code that automatically runs on startup before the user can interact. There is quite a bit of code that runs automatically -- including loading a web page automagically if the app was killed in the background.
On 2017/04/04 18:36:17, justincohen wrote: > NavigationManager::LoadURLWithParams might work, except there are various places > (see bvc) where load is done with [webController loadWithParams:params]; > > ResetFailedStartupAttemptCount should be called after startup, but that means > after any code that automatically runs on startup before the user can interact. > There is quite a bit of code that runs automatically -- including loading a web > page automagically if the app was killed in the background. We talked with Justin and agreed that there is no harm in postponing ResetFailedStartupAttemptCount call (even it it will slightly change the logic). Keeping the logic would require more changes and probably not worth doing.
lgtm
Ok, thanks for discussing. LGTM
Thank you for bearing with me!
The CQ bit was checked by eugenebut@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491335123986580,
"parent_rev": "6a474c2a4eb3a3c1c8913547c6ce2e314238ceeb", "commit_rev":
"72543a7b6abcd436594bbe771ba1e4b39e29df67"}
Message was sent while issue was closed.
Description was changed from ========== Removed -[CRWWebDelegate webWillInitiateLoadWithParams:]. Method implementation was moved into |webDidUpdateSessionForLoadWithParams:wasInitialNavigation:| which is also called from -[CRWWebController loadWithParams:]. This is done, because WebStateObserver will have only one DidStartNavigation callback to avoid extra API complexity. This slightly changes the logic of marking the next crash as non-startup crash. After this change if app crashes during adding the pending entry, the crash will be marked as a startup crash, which should not make any practical different for the user. BUG=674991 ========== to ========== Removed -[CRWWebDelegate webWillInitiateLoadWithParams:]. Method implementation was moved into |webDidUpdateSessionForLoadWithParams:wasInitialNavigation:| which is also called from -[CRWWebController loadWithParams:]. This is done, because WebStateObserver will have only one DidStartNavigation callback to avoid extra API complexity. This slightly changes the logic of marking the next crash as non-startup crash. After this change if app crashes during adding the pending entry, the crash will be marked as a startup crash, which should not make any practical different for the user. BUG=674991 Review-Url: https://codereview.chromium.org/2780403003 Cr-Commit-Position: refs/heads/master@{#461807} Committed: https://chromium.googlesource.com/chromium/src/+/72543a7b6abcd436594bbe771ba1... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/72543a7b6abcd436594bbe771ba1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
