Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2465813002:
Fix content script injection for cancelled loads with PlzNavigate. (Closed)
|
Created:
4 years, 1 month ago by jam Modified:
4 years, 1 month ago CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, Charlie Reis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix content script injection for cancelled loads with PlzNavigate.
With PlzNavigate, the RenderFrame doesn't get OnFailedNavigation messages for requests that don't commit (204 & 205) or ones that turn into downloads. Content scripts code depend on getting this so that they can mark the initial about:blank as idle instead of begin state, otherwise content script injection never completes. The fix is to watch the OnStop message, since in these cases the navigation is always renderer initiated and the browser always sends that message to let it know that the loading stopped.
This fixes
ExecuteScriptApiTest.FrameWithHttp204
with PlzNavigate.
BUG=504347
Committed: https://crrev.com/e24fafa52a1c44cb650444151493883906c75f50
Cr-Commit-Position: refs/heads/master@{#429031}
Patch Set 1 #
Total comments: 6
Patch Set 2 : simpler approach #Messages
Total messages: 30 (19 generated)
Description was changed from ========== Ensure that content scripts aren't injected into frames that haven't had any successful commits. Without PlzNavigate, a RenderFrame that receives a 204 knows this failure state, which is propagated to RenderFrameObservers. This is how the bug fix in r372556 works. With PlzNavigate, 204 & 205s don't reach the renderer (which might not even exist), so also ensure we don't try to inject content scripts in them. This fixes ExecuteScriptApiTest.FrameWithHttp204 with PlzNavigate. BUG=504347 ========== to ========== Ensure that content scripts aren't injected into frames that haven't had any successful commits. Without PlzNavigate, a RenderFrame that receives a 204 knows this failure state, which is propagated to RenderFrameObservers. This is how the bug fix in r372556 works. With PlzNavigate, 204 & 205s don't reach the renderer (which might not even exist), so also ensure we don't try to inject content scripts in them. This fixes ExecuteScriptApiTest.FrameWithHttp204 with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by jam@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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Ensure that content scripts aren't injected into frames that haven't had any successful commits. Without PlzNavigate, a RenderFrame that receives a 204 knows this failure state, which is propagated to RenderFrameObservers. This is how the bug fix in r372556 works. With PlzNavigate, 204 & 205s don't reach the renderer (which might not even exist), so also ensure we don't try to inject content scripts in them. This fixes ExecuteScriptApiTest.FrameWithHttp204 with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix content script injection for cancelled loads with PlzNavigate. With PlzNavigate, the RenderFrame doesn't get notified about 204 & 205 responses. This is fine for all the features except for content script injection. To fix this, have the extensions code monitor these errors and inform the renderer side. This fixes ExecuteScriptApiTest.FrameWithHttp204 with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
jam@chromium.org changed reviewers: + creis@chromium.org, rdevlin.cronin@chromium.org
Charlie: content Devlin: the rest BTW I tried a few approaches to do this fix in the browser side only. However it meant that we would need to duplicate logic in the renderer there, particularly with regards to different times that css/scripts are injected and logic related to about:blank URLs. I also couldn't convince myself that there were no race conditions with navigation messages that could lead to this bug (hang) happening again.
Interesting. What is it that content scripts need to do when a 204 occurs? Would we need to do this for downloads as well? https://codereview.chromium.org/2465813002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2465813002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:503: DCHECK_EQ(frame_tree_node_, render_frame_host->frame_tree_node()); We should probably have these first two DCHECKs in AbortCommit, since a different RFH shouldn't be interfering with an unrelated NavigationHandle. https://codereview.chromium.org/2465813002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2465813002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:257: // Called when a 204 or 205 response is received, which abort the commit. From NavigationHandle's perspective, how is a 204 or 205 different from a navigation that turns out to be a download (which also doesn't commit)? Should we be including downloads in this? https://codereview.chromium.org/2465813002/diff/20001/extensions/common/exten... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2465813002/diff/20001/extensions/common/exten... extensions/common/extension_messages.h:625: IPC_MESSAGE_ROUTED0(ExtensionMsg_Set204Or205Error) Again, I'm wondering if this should be a more general "navigation did not commit" message, depending on if the download case matters.
https://codereview.chromium.org/2465813002/diff/20001/extensions/common/exten... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2465813002/diff/20001/extensions/common/exten... extensions/common/extension_messages.h:625: IPC_MESSAGE_ROUTED0(ExtensionMsg_Set204Or205Error) On 2016/10/31 17:05:28, Charlie Reis wrote: > Again, I'm wondering if this should be a more general "navigation did not > commit" message, depending on if the download case matters. If this *does* stay here (I'll trust y'all on that), the "Error" language seems odd to me, since 204/205s are considered successes, I thought. Maybe just s/Error/Status? https://codereview.chromium.org/2465813002/diff/20001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/2465813002/diff/20001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:239: DidFailProvisionalLoad(blink::WebURLError()); In a non-plznavigate world, would a 204/205 cause DidFailProvisionalLoad?
https://codereview.chromium.org/2465813002/diff/20001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/2465813002/diff/20001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:239: DidFailProvisionalLoad(blink::WebURLError()); On 2016/10/31 17:24:00, Devlin wrote: > In a non-plznavigate world, would a 204/205 cause DidFailProvisionalLoad? For more context, I'm asking because of the case of content scripts that want to inject on every page, and try injecting on a page with a 204 status. I think we even have a test for this (ExecuteScriptApiTest.FrameWithHttp204), but it's disabled for plz navigate. I just wanna make sure we're not going to make this harder on ourselves down the road. :)
The CQ bit was checked by jam@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Fix content script injection for cancelled loads with PlzNavigate. With PlzNavigate, the RenderFrame doesn't get notified about 204 & 205 responses. This is fine for all the features except for content script injection. To fix this, have the extensions code monitor these errors and inform the renderer side. This fixes ExecuteScriptApiTest.FrameWithHttp204 with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix content script injection for cancelled loads with PlzNavigate. With PlzNavigate, the RenderFrame doesn't get OnFailedNavigation messages for requests that don't commit (204 & 205) or ones that turn into downloads. Content scripts code depend on getting this so that they can mark the initial about:blank as idle instead of begin state, otherwise content script injection never completes. The fix is to watch the OnStop message, since in these cases the navigation is always renderer initiated and the browser always sends that message to let it know that the loading stopped. This fixes ExecuteScriptApiTest.FrameWithHttp204 with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
jam@chromium.org changed reviewers: - creis@chromium.org
(moving Charlie to CC now with latest change, feel free to comment though) Devlin: ptal, here's a simpler approach. Based on Charlie's comments about downloads, I looked into a different approach that would handle it (and possibly any other navigation that also didn't end up sending commit/fail IPCs). It turns out that in the case we care about here (initial about:blank where Blink only sends start but not end WebFrameClient notifications), since these are by definition renderer initiated navigations, the browser will always send a Stop IPC message when they don't commit. So watch that instead.
extensions lgtm assuming the test passes. On that note, should we be removing it from the filter here? testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter
Description was changed from ========== Fix content script injection for cancelled loads with PlzNavigate. With PlzNavigate, the RenderFrame doesn't get OnFailedNavigation messages for requests that don't commit (204 & 205) or ones that turn into downloads. Content scripts code depend on getting this so that they can mark the initial about:blank as idle instead of begin state, otherwise content script injection never completes. The fix is to watch the OnStop message, since in these cases the navigation is always renderer initiated and the browser always sends that message to let it know that the loading stopped. This fixes ExecuteScriptApiTest.FrameWithHttp204 with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix content script injection for cancelled loads with PlzNavigate. With PlzNavigate, the RenderFrame doesn't get OnFailedNavigation messages for requests that don't commit (204 & 205) or ones that turn into downloads. Content scripts code depend on getting this so that they can mark the initial about:blank as idle instead of begin state, otherwise content script injection never completes. The fix is to watch the OnStop message, since in these cases the navigation is always renderer initiated and the browser always sends that message to let it know that the loading stopped. This fixes ExecuteScriptApiTest.FrameWithHttp204 with PlzNavigate. BUG=504347 ==========
The CQ bit was checked by jam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/01 16:27:20, Devlin wrote: > extensions lgtm assuming the test passes. On that note, should we be removing > it from the filter here? > testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter yes, although I do that change separately. Due to a workarounds for other bugs, the recipes won't try failures without the patch if it has changes in testing/buildbot/. So I don't like putting those changes with the fix because it sometimes makes the change uncommittable :)
creis@chromium.org changed reviewers: + creis@chromium.org
Sounds reasonable, and I like that it requires less plumbing. LGTM.
Message was sent while issue was closed.
Description was changed from ========== Fix content script injection for cancelled loads with PlzNavigate. With PlzNavigate, the RenderFrame doesn't get OnFailedNavigation messages for requests that don't commit (204 & 205) or ones that turn into downloads. Content scripts code depend on getting this so that they can mark the initial about:blank as idle instead of begin state, otherwise content script injection never completes. The fix is to watch the OnStop message, since in these cases the navigation is always renderer initiated and the browser always sends that message to let it know that the loading stopped. This fixes ExecuteScriptApiTest.FrameWithHttp204 with PlzNavigate. BUG=504347 ========== to ========== Fix content script injection for cancelled loads with PlzNavigate. With PlzNavigate, the RenderFrame doesn't get OnFailedNavigation messages for requests that don't commit (204 & 205) or ones that turn into downloads. Content scripts code depend on getting this so that they can mark the initial about:blank as idle instead of begin state, otherwise content script injection never completes. The fix is to watch the OnStop message, since in these cases the navigation is always renderer initiated and the browser always sends that message to let it know that the loading stopped. This fixes ExecuteScriptApiTest.FrameWithHttp204 with PlzNavigate. BUG=504347 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix content script injection for cancelled loads with PlzNavigate. With PlzNavigate, the RenderFrame doesn't get OnFailedNavigation messages for requests that don't commit (204 & 205) or ones that turn into downloads. Content scripts code depend on getting this so that they can mark the initial about:blank as idle instead of begin state, otherwise content script injection never completes. The fix is to watch the OnStop message, since in these cases the navigation is always renderer initiated and the browser always sends that message to let it know that the loading stopped. This fixes ExecuteScriptApiTest.FrameWithHttp204 with PlzNavigate. BUG=504347 ========== to ========== Fix content script injection for cancelled loads with PlzNavigate. With PlzNavigate, the RenderFrame doesn't get OnFailedNavigation messages for requests that don't commit (204 & 205) or ones that turn into downloads. Content scripts code depend on getting this so that they can mark the initial about:blank as idle instead of begin state, otherwise content script injection never completes. The fix is to watch the OnStop message, since in these cases the navigation is always renderer initiated and the browser always sends that message to let it know that the loading stopped. This fixes ExecuteScriptApiTest.FrameWithHttp204 with PlzNavigate. BUG=504347 Committed: https://crrev.com/e24fafa52a1c44cb650444151493883906c75f50 Cr-Commit-Position: refs/heads/master@{#429031} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e24fafa52a1c44cb650444151493883906c75f50 Cr-Commit-Position: refs/heads/master@{#429031} |
