NavigateToInaccessibleResourceFromChromeURL fails |
||||||
Issue descriptionflakiness dashboard shows that it is flaky on win, chromos and mac https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=ExtensionBrowserTest.NavigateToInaccessibleResourceFromChromeURL alexmos@: could you triage this?
,
Apr 24 2018
Issue 836284 has been merged into this issue.
,
Apr 24 2018
https://logs.chromium.org/v/?s=chromium%2Fbb%2Ftryserver.chromium.chromiumos%2Flinux-chromeos-rel%2F108473%2F%2B%2Frecipes%2Fsteps%2Fbrowser_tests__with_patch_%2F0%2Flogs%2FExtensionBrowserTest.NavigateToInaccessibleResourceFromChromeURL%2F0 [ RUN ] ExtensionBrowserTest.NavigateToInaccessibleResourceFromChromeURL [17687:17768:0424/071126.504576:ERROR:account_manager.cc(37)] Failed to read tokens file [17687:17687:0424/071126.507739:WARNING:user_policy_manager_factory_chromeos.cc(198)] No policy loaded for known non-enterprise user [17687:17687:0424/071126.516264:WARNING:user_session_manager.cc(1086)] Attempting to save user password for non enterprise user. [17687:17687:0424/071126.759916:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type. [17687:17687:0424/071126.760237:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type. [17687:17687:0424/071126.761907:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type. [17687:17687:0424/071126.762783:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type. [17687:17687:0424/071127.760094:ERROR:bad_message.cc(25)] Terminating renderer for bad IPC message, reason 181 [17687:17768:0424/071127.926996:WARNING:simple_synchronous_entry.cc(1255)] Could not open platform files for entry. ../../chrome/browser/extensions/window_open_apitest.cc:376: Failure Expected equality of these values: start_urls[i] Which is: chrome-search://local-ntp/local-ntp.html tab->GetMainFrame()->GetLastCommittedURL() Which is: Stack trace: #0 0x000001f45bec testing::internal::UnitTestImpl::CurrentOsStackTraceExceptTop() #1 0x000001f455c9 testing::internal::AssertHelper::operator=() #2 0x000001310d25 ExtensionBrowserTest_NavigateToInaccessibleResourceFromChromeURL_Test::RunTestOnMainThread()
,
Apr 24 2018
The flakiness dashboard shows that this test started flaking around Apr 23, so it might be related to --site-per-process becoming enabled on ToT in r552589. It's interesting that it affects everything except Linux. The log from #3 shows that we're killing the renderer when trying to navigate to the local NTP URL, with reason 181 being WEBUI_SEND_FROM_UNAUTHORIZED_PROCESS. Looking at the code, it looks like happens when the page tries to use chrome.send(), and either the corresponding process doesn't have WebUI bindings or sender_rfh->GetLastCommittedURL() isn't acceptable for WebUI, leading to a kill in WebUIImpl::OnWebUISend(). The sequence of navigations in the test is: chrome://history -> chrome-extension://id/test.html -> chrome-search://local-ntp/local-ntp.html (the kill probably happens while waiting for this navigation to complete). I'm not sure how the kill is possible, I'll try to find some time to see if I can repro locally. If anyone else is able to help out, please feel free to grab this.
,
Apr 24 2018
,
Apr 24 2018
Confirmed locally that this test becomes flaky when running with --site-per-process.
The source of flakiness is that chrome://history is periodically calls chrome.send(), and sometimes that IPC gets queued up right after we commit a cross-process navigation to a chrome-extension:// URL in the test, and chrome://history becomes pending delete.
Processing WebUIImpl::OnWebUISend() from a pending delete RFH triggers a kill, because
(1) we clear the old RFH's last committed URL when committing a cross-process navigation:
void FrameTreeNode::ResetForNewProcess() {
current_frame_host()->SetLastCommittedUrl(GURL());
...
}
(2) This check in OnWebUISend fails for the empty last committed URL:
WebUIControllerFactoryRegistry::GetInstance()->IsURLAcceptableForWebUI(
web_contents_->GetBrowserContext(), source_url)
The OnWebUISend actually intends to ignore the IPC from inactive RFHs:
// Ignore IPCs from swapped-out frames. See also https://crbug.com/780920.
if (!sender->IsCurrent())
return;
but that is incorrectly done after checking for the renderer kill. Just changing the order and performing this check first should fix this test. I'll put together a CL for that.
Separately, I don't think it's necessary to clear the last committed URL in (1). I think that might date to the days where we kept the last committed URL on FrameTreeNode, but now that it's on RFH, it should be safe (and potentially useful) to keep the last committed URL around even in pending delete state.
,
Apr 24 2018
+1 on the last paragraph of comment 6. Having the last committed URL (and origin) for a RenderFrameHost that is pending deletion will be useful. We can indeed use them for security checks while processing IPCs sent during running unload handler.
,
Apr 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7c7368fd7988bca8dbe664ad475f3cc8abaa29da commit 7c7368fd7988bca8dbe664ad475f3cc8abaa29da Author: Alex Moshchuk <alexmos@chromium.org> Date: Wed Apr 25 00:49:25 2018 Fix flakiness in NavigateToInaccessibleResourceFromChromeURL. There was a race in this test where chrome.send() from chrome://history might have occasionally been processed on a pending delete RenderFrameHost, right after a cross-process navigation. This revealed a real bug in WebUIImpl::OnWebUISend(), which incorrectly killed the renderer before checking whether the RFH is active. Fix this by checking whether the RFH is active and dropping the IPC if not prior to checking WebUI bindings. Change-Id: I672e8457b7a4a167ad31e5afa4b2059b2c92b97a Bug: 836211 Tbr: rdevlin.cronin@chromium.org Reviewed-on: https://chromium-review.googlesource.com/1026803 Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Cr-Commit-Position: refs/heads/master@{#553399} [modify] https://crrev.com/7c7368fd7988bca8dbe664ad475f3cc8abaa29da/chrome/browser/extensions/window_open_apitest.cc [modify] https://crrev.com/7c7368fd7988bca8dbe664ad475f3cc8abaa29da/content/browser/webui/web_ui_impl.cc
,
Apr 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/02c5156d9d1f657bf58888f0f3d1a3fe318def5f commit 02c5156d9d1f657bf58888f0f3d1a3fe318def5f Author: Alex Moshchuk <alexmos@chromium.org> Date: Wed Apr 25 22:26:04 2018 Don't clear last committed URL on pending delete RenderFrameHosts Now that last committed URLs are tracked on RFH instead of FrameTreeNode, there's no harm in keeping this URL around after a RFH becomes pending deletion. Keeping it might be useful for security checks on such RFHs, and not keeping it might actually lead to correctness issues when last committed URLs are checked for pending delete RFHs. Bug: 836211 Change-Id: I2090732bd1f1583430e2eafcf732965f43e00c44 Reviewed-on: https://chromium-review.googlesource.com/1026971 Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#553793} [modify] https://crrev.com/02c5156d9d1f657bf58888f0f3d1a3fe318def5f/content/browser/frame_host/frame_tree_node.cc [modify] https://crrev.com/02c5156d9d1f657bf58888f0f3d1a3fe318def5f/content/browser/frame_host/frame_tree_node_blame_context_unittest.cc [modify] https://crrev.com/02c5156d9d1f657bf58888f0f3d1a3fe318def5f/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/02c5156d9d1f657bf58888f0f3d1a3fe318def5f/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/02c5156d9d1f657bf58888f0f3d1a3fe318def5f/content/browser/site_per_process_browsertest.cc
,
May 2 2018
Flakiness dashboard confirms that this test is no longer flaky. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by bugdroid1@chromium.org
, Apr 24 2018