Issue metadata
Sign in to add a comment
|
DCHECK violation: PlzNavigate crashes on Safe Browsing help links |
||||||||||||||||||||||||
Issue descriptionI noticed this while investigating a Safe Browsing issue, but realized this is caused by PlzNavigate. I can reliably repro on both Chrome and WebView. I've only tested on Android, but I suspect this is cross-platform. Steps: 1) Go to http://testsafebrowsing.appspot.com 2) Click on link #1 (#2 & #4 also work) 3) Click on "privacy policy" (or "learn more," "automatically report," etc.) 4) Observe a crash (if DCHECK is enabled) Using --disable-browser-side-navigation fixes the issue. The crash stack: 0x00000000000bfa10: base::debug::(anonymous namespace)::DebugBreak() at [chromium] //src/base/debug/debugger_posix.cc:228 (inlined by) base::debug::BreakDebugger() at [chromium] //src/base/debug/debugger_posix.cc:258 0x00000000000e4f68: logging::LogMessage::~LogMessage() at [chromium] //src/base/logging.cc:791 0x0000000000b69e8c: content::NavigationRequest::CommitNavigation() at [chromium] //src/content/browser/frame_host/navigation_request.cc:1106 (discriminator 16) 0x0000000000b6af3c: content::NavigationRequest::OnWillProcessResponseChecksComplete(content::NavigationThrottle::ThrottleCheckResult) at [chromium] //src/content/browser/frame_host/navigation_request.cc:1085 --- Here's the DCHECK [1] (request_params_.has_user_gesture = true, begin_params_.has_user_gesture = false). This crashes 10/10 on WebView. Chrome is a little different (the link opens in a new foreground tab). If you go back to the interstitial tab, you will probably see an ANR (app not responding) or crash. I chatted with Nasko today, and he thinks it's suspicious that we're hardcoding `false` here [2]. CC'ing some interstitial people (FYI) and some PlzNavigate people. [1] https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_request.cc?q=commitnavigation+f:request.cc&sq=package:chromium&l=1106 [2] https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_request.cc?type=cs&q=createbrowserinitiated+f:navigation_request.cc&sq=package:chromium&l=272
,
Oct 12 2017
,
Oct 12 2017
,
Oct 12 2017
If behavior changes based on platform, maybe this is an interstitial bug? If desktop really doesn't have user gesture for this action, Android probably shouldn't either... As I recall, both Desktop and Android use the same code (OpenInNewForegroundTab) for this action.
,
Oct 12 2017
Correction: that's OpenUrlInNewForegroundTab [1] [1] https://cs.chromium.org/chromium/src/components/security_interstitials/content/security_interstitial_controller_client.cc?type=cs&sq=package:chromium&l=85
,
Oct 16 2017
Waiting for Android test keys to be able to trigger this from a custom build.
,
Oct 20 2017
Received the keys, starting investigation.
,
Oct 23 2017
After talking to clamy@, it looks like the addition of https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_entry_impl.cc?sq=package:chromium&l=731 was not matched in content::BeginNavigationParams. Verified that using the same assignment in both constructors fix the issue. However this should not be part of both BeginNavigationParams and BeginNavigationParams but of CommonNavigationParams instead. Working on changing this.
,
Oct 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/14a71ad2bc721ff84f3a852e138eda2cb2fb1641 commit 14a71ad2bc721ff84f3a852e138eda2cb2fb1641 Author: Arthur Hemery <ahemery@chromium.org> Date: Fri Oct 27 10:54:39 2017 Navigation: Moving has_user_gesture to CommonNavigationParams has_user_gesture is now only in CommonNavigationParams instead of being part of both BeginNavigationParams and RequestNavigationParams. This also solved a problem where has_user_gesture was not set in the same way in begin and requests params on Android leading to a DCHECK failure under specific circumstances. Bug: 773755 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I4d22176f5376000e16a1b3bc491f969308290e83 Reviewed-on: https://chromium-review.googlesource.com/735331 Reviewed-by: Camille Lamy <clamy@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Arthur Hemery <ahemery@chromium.org> Cr-Commit-Position: refs/heads/master@{#512144} [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/browser/browser_side_navigation_browsertest.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/browser/frame_host/navigation_entry_impl.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/browser/frame_host/navigation_request.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/browser/frame_host/navigator_impl.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/browser/frame_host/navigator_impl_unittest.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/browser/loader/navigation_url_loader_network_service_unittest.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/browser/loader/navigation_url_loader_unittest.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/browser/loader/resource_dispatcher_host_impl.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/browser/loader/resource_dispatcher_host_unittest.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/common/frame_messages.h [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/common/navigation_params.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/common/navigation_params.h [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/public/test/navigation_simulator.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/public/test/render_view_test.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/renderer/render_frame_impl.cc [modify] https://crrev.com/14a71ad2bc721ff84f3a852e138eda2cb2fb1641/content/test/test_render_frame_host.cc
,
Oct 27 2017
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by jialiul@chromium.org
, Oct 11 2017