Issue metadata
Sign in to add a comment
|
App crashes when navigating back and forward after scanning QR code |
||||||||||||||||||||||
Issue descriptionApp Version: 67.0.3362.0 canary iOS Version: 11.2.6 Device: iPad and iPhones URL: any Precondition: Enable #slim-navigation-manager from about://flags Steps to reproduce: 1. Launch Google Chrome 2. Navigate to facebook.com 3. Navigate to about:version (in the same tab) 4. Scan a QR code (in the same tab) 5. Tap back arrow twice to reach the first page 6. Tap forward Observed results: App Crashes Crash ID: http://crbug/632226bc39655c25 Expected results: App Shouldn't be crashed Number of times you were able to reproduce: 5/5 Bug reproducible after clean install: Yes Bug reproducible after clearing cache and cookies: Yes Bug reproducible on Chrome Mobile on Android: NA Bug reproducible on Safari/Firefox: Firefox: NA, Safari: NA Bug reproducible on current stable build (App Version, iOS Version): M64 NO (New Feature) Bug reproducible on the current beta channel build (App Version, iOS Version): M65 NO (New Feature) Link to video/image: https://drive.google.com/file/d/1WzKf70kGOa6-CP3IEM_aKlI1t_Mr0FdV/view
,
Mar 12 2018
Is this issue in M66 as well? If yes then can we make sure to address it in M66?
,
Mar 12 2018
Only when #slim-navigation-manager feature is enabled. YES
,
Mar 29 2018
I was able to reproduce this in canary 67.0.3382.0 as well. https://crash.corp.google.com/browse?stbtiq=50c68c17ef1e1098 0x0000000100d87064 (Chrome -crw_web_controller.mm:2739 ) -[CRWWebController webPageChangedWithContext:] 0x0000000100d87060 (Chrome -crw_web_controller.mm:2736 ) -[CRWWebController webPageChangedWithContext:] 0x0000000100d8fcdc (Chrome -crw_web_controller.mm:4589 ) -[CRWWebController webView:didCommitNavigation:] 0x0000000190ce26a4 (WebKit + 0x000766a4 ) WebKit::NavigationState::NavigationClient::didCommitNavigation(WebKit::WebPageProxy&, API::Navigation*, API::Object*) 0x0000000190e4c02c (WebKit + 0x001e002c ) WebKit::WebPageProxy::didCommitLoadForFrame(unsigned long long, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, std::optional<WebCore::HasInsecureContent>, WebKit::UserData const&) 0x0000000190e6f8cc (WebKit + 0x002038cc ) void IPC::callMemberFunctionImpl<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long long, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, std::optional<WebCore::HasInsecureContent>, WebKit::UserData const&), std::__1::tuple<unsigned long long, unsigned long long, WTF::String, bool, unsigned int, WebCore::CertificateInfo, bool, std::optional<WebCore::HasInsecureContent>, WebKit::UserData>, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul>(WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long long, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, std::optional<WebCore::HasInsecureContent>, WebKit::UserData const&), std::__1::tuple<unsigned long long, unsigned long long, WTF::String, bool, unsigned int, WebCore::CertificateInfo, bool, std::optional<WebCore::HasInsecureContent>, WebKit::UserData>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul>) 0x0000000190e689c8 (WebKit + 0x001fc9c8 ) void IPC::handleMessage<Messages::WebPageProxy::DidCommitLoadForFrame, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long long, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, std::optional<WebCore::HasInsecureContent>, WebKit::UserData const&)>(IPC::Decoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long long, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, std::optional<WebCore::HasInsecureContent>, WebKit::UserData const&)) 0x0000000190cdee60 (WebKit + 0x00072e60 ) IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) 0x0000000190ec087c (WebKit + 0x0025487c ) WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) 0x0000000190c9e690 (WebKit + 0x00032690 ) IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) 0x0000000190ca10a0 (WebKit + 0x000350a0 ) IPC::Connection::dispatchOneMessage() 0x0000000188e3c980 (JavaScriptCore + 0x00991980 ) WTF::RunLoop::performWork() 0x0000000188e3cc40 (JavaScriptCore + 0x00991c40 ) WTF::RunLoop::performWork(void*) 0x0000000181a8f778 (CoreFoundation + 0x000eb778 ) __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ 0x0000000181a8f6f8 (CoreFoundation + 0x000eb6f8 ) __CFRunLoopDoSource0 0x0000000181a8ef80 (CoreFoundation + 0x000eaf80 ) __CFRunLoopDoSources0 0x0000000181a8cb58 (CoreFoundation + 0x000e8b58 ) __CFRunLoopRun 0x00000001819acc54 (CoreFoundation + 0x00008c54 ) CFRunLoopRunSpecific 0x0000000183858f80 (GraphicsServices + 0x0000af80 ) GSEventRunModal 0x000000018b1055c0 (UIKit + 0x000735c0 ) UIApplicationMain 0x0000000100bdd5d0 (Chrome -chrome_exe_main.mm:54 ) main 0x00000001814cc568 (libdyld.dylib + 0x00001568 ) start
,
Apr 6 2018
Although the original steps to reproduce requires #slim-navigation-manager feature tobe enabled, this crash appears in M65 stable version of the app as well. Currently trending at top#11 position, total of 6k+ crashes on M65. https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_iOS%27%20AND%20expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27-%5BCRWWebController%20webPageChangedWithContext%3A%5D%27
,
May 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ebe29d3d16d74fea153a8bfa01af0985a3d03de commit 4ebe29d3d16d74fea153a8bfa01af0985a3d03de Author: Danyao Wang <danyao@google.com> Date: Mon May 07 17:45:59 2018 [Nav Experiment] Allow back/forward navigation to WebUI URL. This is to handle an edge case where on the second back navigation to a WebUI URL from a web URL, |backForwardList.currentItem.URL| becomes the WebUI URL instead of the placeholder URL. This is incorrectly classified as a renderer-initiated app-specific load. The early exit causes no NavigationContext to be created, which leads to nullptr deferences in |webView:didCommitNavigation| and |webView:didFinishNavigation|. Also added nullptr checks for NavigationContext in general. Bug: 838585, 818813 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I1e2225adac2cc639e4f6a51bdcb4dece03d96648 Reviewed-on: https://chromium-review.googlesource.com/1044875 Commit-Queue: Danyao Wang <danyao@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#556489} [modify] https://crrev.com/4ebe29d3d16d74fea153a8bfa01af0985a3d03de/ios/chrome/browser/ui/webui/web_ui_egtest.mm [modify] https://crrev.com/4ebe29d3d16d74fea153a8bfa01af0985a3d03de/ios/web/web_state/ui/crw_web_controller.mm
,
May 7 2018
,
May 10 2018
Sorry for requesting merge late. This is the same fix for crbug.com/838585. However, we can't confirm that bug is fixed because it seems to depend on crbug.com/813018. I confirmed that this bug is fixed in M68 canary build (68.0.3426.0) using the steps in the original post.
,
May 10 2018
This bug requires manual review: Less than 15 days to go before AppStore submit on M67 Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 14 2018
If this bug is the same as crbug.com/838585 should we mark it as duplicate? Since both of the above bugs rely on crbug.com/813018, can we change priority on crbug.com/813018 to P1 and add the blocking bugs to it? If this bug relies on crbug.com/813018, I would like to wait until crbug.com/813018 is fixed and merged before merging this bug.
,
May 14 2018
The difference is that this bug is fully fixed with the patch in comment#6, but crbug.com/838585 is blocked on crbug.com/813018. My thinking is that crbug.com/813018 is a known bug for the last couple of releases and given the timeline, it's unlikely to be fixed for this release. This bug is a new one just for #slim-navigation-manager, and it appears to be fully fixed by the patch. That's why I think merging the patch is a net win.
,
May 15 2018
Verified the fix on M68.0.3431.0 canary iOS: 11.4, 10.3.3 Devices: iPhone, iPad No crashes seen after following the steps from original bug report.
,
May 15 2018
I was leaning towards not merging this into M67 since this issue was first seen in M65 (c#5) and we've had no major issues with this in M65 or M66 from what I can tell. Now that it's been verified on canary, it gives me slightly more confidence. Peter, Eugene: what do you think? Danyao: how confident are you in this fix and why should it be merged into 67?
,
May 15 2018
This bug only affects #slim-navigation-manager. It was noticed in M65 when we just started fishfood the feature. But the feature was disabled for the general public. I'll be starting a 1% experiment in M67, which means more people may run into this crash. So I think there's high value merging this for M67. I'm confident in this fix. Moreover, this fix only changes behavior for users who turns on #slim-navigation-manager. So the overall stability risk should be low.
,
May 15 2018
I looked at M66 (stable) the average daily collected crashes averages to ~80/day (don't know why it is trending down since early May though). I think it is borderline. If we have any doubt at all, I wouldn't mind if we just leave the fix in M68 and *not* cherrypick it for M67.
,
May 16 2018
Hi Danyao - I think it's best to not merge this into M67. It seems that with or without the fix, the bug will only affect some users who turn on #slim-navigation-manager. Also per #15 the number of crashes is pretty low at ~80/day.
,
May 18 2018
OK.
,
May 18 2018
One thing I'd like to point out: the 80/day number may not be representative because the total number of users using #slim-navigation-manager has also been low. I'm about to start a 1% experiment in M67 stable, and this will expose the crash to more users. That's why I wanted to merge this change to avoid having to delay #slim-navigation-manager rollout. That said, the crash only triggers around WebUI, which is around 8% of total navigations. It also requires the user to repeatedly navigation back and forward around the WebUI entry. Maybe the chance of actual users going to chrome:// URLs is even lower than internal testing. So I'm OK with not merging this.
,
May 22 2018
After speaking more about this with Danyao and Eugene, 80 crashes/day is a lot for TestFlight considering we have about 1,000 users on that channel. Also, since WKBasedNavigationManager feature is going to 1% experiment for 67, it's likely the crashes will increase. Danyao please confirm the fix in #6 is a part of the "Experiment with WKBasedNavigationManager in iOS Chrome" feature and completely behind a finch flag. If so, we will merge today.
,
May 22 2018
,
May 22 2018
,
May 22 2018
The fix in comment#6 is part of the "Experiment with WKBasedNavigationManager in iOS Chrome" feature. The new functionality added in the fix is completely behind a Finch flag.
,
May 22 2018
Thanks Danyao! Approved - Please merge asap.
,
May 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/615b8ba8a228b93c83676cc0e01f15d0242b9add commit 615b8ba8a228b93c83676cc0e01f15d0242b9add Author: Danyao Wang <danyao@google.com> Date: Tue May 22 18:46:10 2018 [Nav Experiment] Allow back/forward navigation to WebUI URL. Cherry-pick to refs/branch-heads/3396. This is to handle an edge case where on the second back navigation to a WebUI URL from a web URL, |backForwardList.currentItem.URL| becomes the WebUI URL instead of the placeholder URL. This is incorrectly classified as a renderer-initiated app-specific load. The early exit causes no NavigationContext to be created, which leads to nullptr deferences in |webView:didCommitNavigation| and |webView:didFinishNavigation|. Also added nullptr checks for NavigationContext in general. TBR=danyao@google.com Bug: 838585, 818813 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I1e2225adac2cc639e4f6a51bdcb4dece03d96648 Reviewed-on: https://chromium-review.googlesource.com/1044875 Commit-Queue: Danyao Wang <danyao@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#556489}(cherry picked from commit 4ebe29d3d16d74fea153a8bfa01af0985a3d03de) Reviewed-on: https://chromium-review.googlesource.com/1069372 Reviewed-by: Danyao Wang <danyao@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#679} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/615b8ba8a228b93c83676cc0e01f15d0242b9add/ios/chrome/browser/ui/webui/web_ui_egtest.mm [modify] https://crrev.com/615b8ba8a228b93c83676cc0e01f15d0242b9add/ios/web/web_state/ui/crw_web_controller.mm
,
May 22 2018
Merge merged. Removing RBS.
,
May 24 2018
Verified on M67.0.3369.59 beta iOS 11.4 iPhoneX. Looks good and no crashes seen. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by pkl@chromium.org
, Mar 5 2018Status: Assigned (was: Untriaged)