|
|
DescriptionFixing WKWebView QuickBack
This patch changes a DCHECK used to validate
WKBackForwardListItems before using them to an if check.
This fixes a crash caused by QuickBack.
BUG=529890
Committed: https://crrev.com/03f902257334935fa479fbc0e5fef241b0a6dd1f
Cr-Commit-Position: refs/heads/master@{#348746}
Patch Set 1 #Patch Set 2 : Revised fix #
Total comments: 1
Patch Set 3 : Fixing comment #Messages
Total messages: 18 (3 generated)
bhnascar@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@chromium.org
It is unclear why BFLI is alive. If it's because Preload maintains 2 web views, then clearing BFLI is not the right way to go.
The prerender controller creates a separate tab with its own WebState for omnibox autocomplete suggestions, and we swap out the prerender tab with the current tab if the user taps on one of the autocomplete search suggestions. Since we're losing the old WKWebView when we swap it out with the prerender tab, removing the old BFLIs seems like a reasonable solution. It's not great, since we'd lose any form resubmission information if the user ever taps on a search suggestion, but off the top of my head, it looks like the only way to preserve the BFLIs 100% of the time would be to remove prerendering completely...
On 2015/09/14 18:25:58, kkhorimoto_ wrote: > The prerender controller creates a separate tab with its own WebState for > omnibox autocomplete suggestions, and we swap out the prerender tab with the > current tab if the user taps on one of the autocomplete search suggestions. > Since we're losing the old WKWebView when we swap it out with the prerender tab, > removing the old BFLIs seems like a reasonable solution. It's not great, since > we'd lose any form resubmission information if the user ever taps on a search > suggestion, but off the top of my head, it looks like the only way to preserve > the BFLIs 100% of the time would be to remove prerendering completely... This is a Quick Back bug (not Preload) and I suspect that it is happening because one Tab has 2 Web Views. If that's true then there is no need to remove old BFLIs, because they are valid (just not for current web view).
On 2015/09/14 18:29:38, eugenebut wrote: > On 2015/09/14 18:25:58, kkhorimoto_ wrote: > > The prerender controller creates a separate tab with its own WebState for > > omnibox autocomplete suggestions, and we swap out the prerender tab with the > > current tab if the user taps on one of the autocomplete search suggestions. > > Since we're losing the old WKWebView when we swap it out with the prerender > tab, > > removing the old BFLIs seems like a reasonable solution. It's not great, > since > > we'd lose any form resubmission information if the user ever taps on a search > > suggestion, but off the top of my head, it looks like the only way to preserve > > the BFLIs 100% of the time would be to remove prerendering completely... > This is a Quick Back bug (not Preload) and I suspect that it is happening > because one Tab has 2 Web Views. > If that's true then there is no need to remove old BFLIs, because they are valid > (just not for current web view). Can we apply valid BFLIs to a different WKWebView than the one for which they were originally created? I was under the assumption that they could only be applied to their originating WKWebView. The function that this CL is modifying is used for both prerendering and quickback, and both code paths seem to be swapping out the Tab (and its underlying WKWebView) in the tab model, which would explain why the BFLIs correspond to two different web views (one for preload/quickback, one for the main tab).
On 2015/09/14 18:36:23, kkhorimoto_ wrote: > On 2015/09/14 18:29:38, eugenebut wrote: > > On 2015/09/14 18:25:58, kkhorimoto_ wrote: > > > The prerender controller creates a separate tab with its own WebState for > > > omnibox autocomplete suggestions, and we swap out the prerender tab with the > > > current tab if the user taps on one of the autocomplete search suggestions. > > > Since we're losing the old WKWebView when we swap it out with the prerender > > tab, > > > removing the old BFLIs seems like a reasonable solution. It's not great, > > since > > > we'd lose any form resubmission information if the user ever taps on a > search > > > suggestion, but off the top of my head, it looks like the only way to > preserve > > > the BFLIs 100% of the time would be to remove prerendering completely... > > This is a Quick Back bug (not Preload) and I suspect that it is happening > > because one Tab has 2 Web Views. > > If that's true then there is no need to remove old BFLIs, because they are > valid > > (just not for current web view). > > Can we apply valid BFLIs to a different WKWebView than the one for which they > were originally created? No, so this code should probably take into account that BFLI can be valid but belong to another Web View.
On 2015/09/14 18:44:42, eugenebut wrote: > On 2015/09/14 18:36:23, kkhorimoto_ wrote: > > On 2015/09/14 18:29:38, eugenebut wrote: > > > On 2015/09/14 18:25:58, kkhorimoto_ wrote: > > > > The prerender controller creates a separate tab with its own WebState for > > > > omnibox autocomplete suggestions, and we swap out the prerender tab with > the > > > > current tab if the user taps on one of the autocomplete search > suggestions. > > > > Since we're losing the old WKWebView when we swap it out with the > prerender > > > tab, > > > > removing the old BFLIs seems like a reasonable solution. It's not great, > > > since > > > > we'd lose any form resubmission information if the user ever taps on a > > search > > > > suggestion, but off the top of my head, it looks like the only way to > > preserve > > > > the BFLIs 100% of the time would be to remove prerendering completely... > > > This is a Quick Back bug (not Preload) and I suspect that it is happening > > > because one Tab has 2 Web Views. > > > If that's true then there is no need to remove old BFLIs, because they are > > valid > > > (just not for current web view). > > > > Can we apply valid BFLIs to a different WKWebView than the one for which they > > were originally created? > No, so this code should probably take into account that BFLI can be valid but > belong to another Web View. Okay, so update after digging for half an hour: When you click on a link on a Google SRP, QuickBackLoadController creates a NEW tab (let's call this tab B) to load the search result. It saves the tab that has the Google SRP (let's call this tab A) and hides it. When you click the back button to go back to the SRP, it swaps tab A back in right away. Theoretically, no load is required since we're using the saved tab. However, when you hit the back button, it still sends the |goBack| call to tab B. Tab B's web view is different from tab A's web view, but they use the same session entries and navigation items. This causes a problem because tab B will fail when it calls |goToBackForwardListItem:| with a WKBackForwardListItem that belongs to tab A's web view. The DCHECK in |loadRequestForCurrentNavigationItem| catches this.
On 2015/09/14 20:50:21, bhnascar wrote: > On 2015/09/14 18:44:42, eugenebut wrote: > > On 2015/09/14 18:36:23, kkhorimoto_ wrote: > > > On 2015/09/14 18:29:38, eugenebut wrote: > > > > On 2015/09/14 18:25:58, kkhorimoto_ wrote: > > > > > The prerender controller creates a separate tab with its own WebState > for > > > > > omnibox autocomplete suggestions, and we swap out the prerender tab with > > the > > > > > current tab if the user taps on one of the autocomplete search > > suggestions. > > > > > Since we're losing the old WKWebView when we swap it out with the > > prerender > > > > tab, > > > > > removing the old BFLIs seems like a reasonable solution. It's not > great, > > > > since > > > > > we'd lose any form resubmission information if the user ever taps on a > > > search > > > > > suggestion, but off the top of my head, it looks like the only way to > > > preserve > > > > > the BFLIs 100% of the time would be to remove prerendering completely... > > > > This is a Quick Back bug (not Preload) and I suspect that it is happening > > > > because one Tab has 2 Web Views. > > > > If that's true then there is no need to remove old BFLIs, because they are > > > valid > > > > (just not for current web view). > > > > > > Can we apply valid BFLIs to a different WKWebView than the one for which > they > > > were originally created? > > No, so this code should probably take into account that BFLI can be valid but > > belong to another Web View. > > Okay, so update after digging for half an hour: > > When you click on a link on a Google SRP, QuickBackLoadController creates a NEW > tab (let's call this tab B) to load the search result. It saves the tab that has > the Google SRP (let's call this tab A) and hides it. > > When you click the back button to go back to the SRP, it swaps tab A back in > right away. Theoretically, no load is required since we're using the saved tab. > However, when you hit the back button, it still sends the |goBack| call to tab > B. Tab B's web view is different from tab A's web view, but they use the same > session entries and navigation items. This causes a problem because tab B will > fail when it calls |goToBackForwardListItem:| with a WKBackForwardListItem that > belongs to tab A's web view. The DCHECK in |loadRequestForCurrentNavigationItem| > catches this. Is there a way to suppress |goBack| call on Tab B?
On 2015/09/14 20:50:21, bhnascar wrote: > On 2015/09/14 18:44:42, eugenebut wrote: > > On 2015/09/14 18:36:23, kkhorimoto_ wrote: > > > On 2015/09/14 18:29:38, eugenebut wrote: > > > > On 2015/09/14 18:25:58, kkhorimoto_ wrote: > > > > > The prerender controller creates a separate tab with its own WebState > for > > > > > omnibox autocomplete suggestions, and we swap out the prerender tab with > > the > > > > > current tab if the user taps on one of the autocomplete search > > suggestions. > > > > > Since we're losing the old WKWebView when we swap it out with the > > prerender > > > > tab, > > > > > removing the old BFLIs seems like a reasonable solution. It's not > great, > > > > since > > > > > we'd lose any form resubmission information if the user ever taps on a > > > search > > > > > suggestion, but off the top of my head, it looks like the only way to > > > preserve > > > > > the BFLIs 100% of the time would be to remove prerendering completely... > > > > This is a Quick Back bug (not Preload) and I suspect that it is happening > > > > because one Tab has 2 Web Views. > > > > If that's true then there is no need to remove old BFLIs, because they are > > > valid > > > > (just not for current web view). > > > > > > Can we apply valid BFLIs to a different WKWebView than the one for which > they > > > were originally created? > > No, so this code should probably take into account that BFLI can be valid but > > belong to another Web View. > > Okay, so update after digging for half an hour: > > When you click on a link on a Google SRP, QuickBackLoadController creates a NEW > tab (let's call this tab B) to load the search result. It saves the tab that has > the Google SRP (let's call this tab A) and hides it. > > When you click the back button to go back to the SRP, it swaps tab A back in > right away. Theoretically, no load is required since we're using the saved tab. > However, when you hit the back button, it still sends the |goBack| call to tab > B. Tab B's web view is different from tab A's web view, but they use the same > session entries and navigation items. This causes a problem because tab B will > fail when it calls |goToBackForwardListItem:| with a WKBackForwardListItem that > belongs to tab A's web view. The DCHECK in |loadRequestForCurrentNavigationItem| > catches this. So then, I think the correct fix is to turn the DCHECK in |loadRequestForCurrentNavigationItem| into an if {...} instead. We don't want to reset the BFLIs because that would kill form resubmission for any forms in tab A.
lgtm https://codereview.chromium.org/1307103004/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1307103004/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:453: // the current WKWebView's back-forward list, we can only fall back to the NIT: please do not use we in the comments.
The CQ bit was checked by bhnascar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/1307103004/#ps40001 (title: "Fixing comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1307103004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307103004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/03f902257334935fa479fbc0e5fef241b0a6dd1f Cr-Commit-Position: refs/heads/master@{#348746}
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/03f902257334935fa479fbc0e5fef241b0a6dd1f Cr-Commit-Position: refs/heads/master@{#348746} |