2 tests in MimeHandlerViewCrossProcessTest are flaky on Win 7 dbg |
||||
Issue descriptionSeeing flakes on the Win 7 dbg bot in MimeHandlerViewCrossProcessTest.NavigationRaceFromCrossProcessRenderer/1 and MimeHandlerViewCrossProcessTest.EmbedWithInitialCrossOriginFrame/1. I will disable the tests. Example failures of NavigationRaceFromCrossProcessRenderer/1: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win7%20Tests%20%28dbg%29%281%29/72448 https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win7%20Tests%20%28dbg%29%281%29/72436 Example failure of EmbedWithInitialCrossOriginFrame/1: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win7%20Tests%20%28dbg%29%281%29/72437
,
Oct 23
Issue 898029 has been merged into this issue.
,
Oct 23
,
Oct 24
I see a few remaining issues not properly addressed in CL 1240635 (support for proxies in attach WebContents). Context: The FrameNavigationHelper fires a navigation to about:blank and waits for the first finished navigation to about:blank. Then immediately attaches WebContentses (we currently need a frame same process with its parent for attach to happen). Meanwhile, to avoid interference from other navigations for the same FrameTreeNode, throttles any forthcoming navigation which is not to about:blank. There is a couple of problems here: 1- First off, to attach right after DidFinishNavigation does not seem safe. This will end up destroying the navigation_handle->GetRenderFrameHost() (after calling AttachToOuterWebContentsFrame). An observer such as ContentPasswordManagerDriverFactory is relying on NavigationHandle->GetRenderFrameHost() not been deleted when a navigation commits (note that RFH has private dtor so I think causing a deletion through non-navigation path is possibly a bug). 2- We could have multiple navigations to about:blank which might have started already so we commit multiple times to about:blank for the same frame perhaps. This is just a guess but I believe is possible because the navigations here are from cross-origin to same-origin about:blank and do not seem to be happening synchronously. The scenario I am thinking is when we set 'object.data = "pdf"' and then repeatedly call object.contentDocument.defaultView.location.href = "about:blank". So many about:blank navigations are queued up. I am working on this design to see if the issue can be resolved without changes to the content API AttachToOuterWebContentsFrame. But I think the comment above the method declaration should point out that the |outer_render_frame_host| will be destroyed soon (synchronously?).
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dd9a414abfcc248c94dca646d85741cf7c7d956e commit dd9a414abfcc248c94dca646d85741cf7c7d956e Author: Ehsan Karamad <ekaramad@chromium.org> Date: Tue Dec 04 20:38:20 2018 WebContents should attach after navigations commit To accommodate attaching MimeHandlerViewGuest though a proxy, the FrameTreeNode is first navigated to about:blank. The current logic in FrameNavigationHelper waits for the first commit to about:blank and then continues the attach process. This current behavior has some problems which this CL tries to resolve. 1- The helper class listens to WebContentsObserver::DidFinishNavigation to find out when the navigation in the plugin frame commits; then immediately attaches to the outer WebContents. This is incorrect as attaching will synchronously destroy the RenderFrameHost which causes issues with other observers of the navigations in that frame (NavigationHandle::GetRenderFrameHost() becomes invalid. 2- There could be many queued up navigations to about:blank and some of them will commit after attach which causes the same problem as 1). To address issue 1), this CL will post task the attach to give all the observers the chance to observe the committed navigation. To address issue 2 ) new APIs are introduced to content layer which help to reset a frame's state (navigation requests and loading state) prior to attaching. The CL will also enable a test which was disabled due to crashes caused by the mentioned problems above. Bug: 897971, 659750 Change-Id: Ie26a7ca644dcbdb1b18e5a659119ea2f46719c0a Reviewed-on: https://chromium-review.googlesource.com/c/1298354 Commit-Queue: Ehsan Karamad <ekaramad@chromium.org> Reviewed-by: James MacLean <wjmaclean@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#613683} [modify] https://crrev.com/dd9a414abfcc248c94dca646d85741cf7c7d956e/chrome/test/data/extensions/api_test/mime_handler_view/test_page.html [modify] https://crrev.com/dd9a414abfcc248c94dca646d85741cf7c7d956e/content/browser/frame_host/navigation_controller_impl.cc [modify] https://crrev.com/dd9a414abfcc248c94dca646d85741cf7c7d956e/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/dd9a414abfcc248c94dca646d85741cf7c7d956e/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/dd9a414abfcc248c94dca646d85741cf7c7d956e/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/dd9a414abfcc248c94dca646d85741cf7c7d956e/content/browser/web_contents/web_contents_impl.h [modify] https://crrev.com/dd9a414abfcc248c94dca646d85741cf7c7d956e/content/public/browser/render_frame_host.h [modify] https://crrev.com/dd9a414abfcc248c94dca646d85741cf7c7d956e/content/public/browser/web_contents.h [modify] https://crrev.com/dd9a414abfcc248c94dca646d85741cf7c7d956e/extensions/browser/guest_view/extensions_guest_view_message_filter.cc [modify] https://crrev.com/dd9a414abfcc248c94dca646d85741cf7c7d956e/extensions/browser/guest_view/extensions_guest_view_message_filter.h [modify] https://crrev.com/dd9a414abfcc248c94dca646d85741cf7c7d956e/extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc |
||||
►
Sign in to add a comment |
||||
Comment 1 by bugdroid1@chromium.org
, Oct 23