Renderer crash when activating fullscreen Flash from OOPIFs |
|||||||
Issue descriptionWith --site-per-process and a Flash-enabled chrome: (1) Go to http://csreis.github.io/tests/cross-site-iframe-simple.html (2) From devtools, navigate the frame to a page that has a fullscreenable Flash object: frames[0].location.href="http://permadi.com/tutorial/flash9FullScreen/index.html" (3) Click on the fullscreen icon in the Flash object. Note: there is another bug with Flash visibility/painting in OOPIF, issue 593520 . Until that's fixed, the Flash object in the repro won't be visible but you can still interact with it and click on the fullscreen icon. First do the repro steps without --site-per-process to see where the icon is (upper right), and then click in the same location with --site-per-process. This crashes the renderer with the following stack: Received signal 11 SEGV_MAPERR 000000000280 #0 0x7f3aaae5184e base::debug::StackTrace::StackTrace() #1 0x7f3aaae5138f base::debug::(anonymous namespace)::StackDumpSignalHandler() #2 0x7f3a982d8340 <unknown> #3 0x7f3aa20a89ac WTF::RefPtr<>::operator->() #4 0x7f3aa20a9e05 WTF::CString::buffer() #5 0x7f3aa20aa734 blink::WebString::WebString() #6 0x7f3a9e397f8d blink::WebURL::WebURL() #7 0x7f3a9cf79137 blink::WebDocument::url() #8 0x7f3aa60fad66 content::RenderFrameImpl::CreatePepperFullscreenContainer() #9 0x7f3aa63fd0a6 content::PepperPluginInstanceImpl::FlashSetFullscreen() #10 0x7f3aafa63a72 PepperFlashFullscreenHost::OnSetFullscreen() #11 0x7f3aafa63b43 _ZN5ppapi4host20DispatchResourceCallI25PepperFlashFullscreenHostMS2_FiPNS0_18HostMessageContextEbEbEEiPT_T0_S4_RSt5tupleIJT1_EE #12 0x7f3aafa639b5 PepperFlashFullscreenHost::OnResourceMessageReceived() #13 0x7f3a9f4825da ppapi::host::ResourceMessageHandler::RunMessageHandlerAndReply() #14 0x7f3a9f47f8cc ppapi::host::ResourceHost::HandleMessage() #15 0x7f3a9f4740be ppapi::host::PpapiHost::HandleResourceCall() #16 0x7f3a9f472a52 ppapi::host::PpapiHost::OnHostMsgResourceSyncCall() #17 0x7f3a9f47990a _ZN4base20DispatchToMethodImplIPN5ppapi4host9PpapiHostEMS3_FvRKNS1_5proxy25ResourceMessageCallParamsERKN3IPC7MessageEPSA_EJS6_SA_EJRSA_EJLm0ELm1EEJLm0EEEEvRKT_T0_RKSt5tupleIJDpT1_EEPSL_IJDpT2_EENS_13IndexSequenceIJXspT3_EEEENSV_IJXspT4_EEEE #18 0x7f3a9f478ea8 _ZN4base16DispatchToMethodIPN5ppapi4host9PpapiHostEMS3_FvRKNS1_5proxy25ResourceMessageCallParamsERKN3IPC7MessageEPSA_EJS6_SA_EJRSA_EEEvRKT_T0_RKSt5tupleIJDpT1_EEPSL_IJDpT2_EE #19 0x7f3a9f474c11 _ZN3IPC8MessageTI34PpapiHostMsg_ResourceSyncCall_MetaSt5tupleIJN5ppapi5proxy25ResourceMessageCallParamsENS_7MessageEEES2_IJNS4_26ResourceMessageReplyParamsES6_EEE18DispatchDelayReplyINS3_4host9PpapiHostEvMSD_FvRKS5_RKS6_PS6_EEEbPSG_PT_PT0_T1_ #20 0x7f3a9f471f86 ppapi::host::PpapiHost::OnMessageReceived() #21 0x7f3aa01eef11 ppapi::proxy::HostDispatcher::OnMessageReceived() #22 0x7f3aa33d9fb3 IPC::ChannelProxy::Context::OnDispatchMessage() #23 0x7f3aa340090b IPC::SyncChannel::ReceivedSyncMsgQueue::DispatchMessages() #24 0x7f3aa33fdfa1 IPC::SyncChannel::SyncContext::DispatchMessages() #25 0x7f3aa33ffb2d IPC::SyncChannel::OnWaitableEventSignaled() Looks like we're trying to reference the main frame's URL to be used in RenderWidgetFullscreenPepper::GetURLForGraphicsContext3D(), so we'll need to fix this for remote main frames. Not sure what other work there is beyond this, but hopefully not much - piman@ mentioned Flash fullscreen code should be pretty frame-agnostic. I've tried this on Mac and Windows canaries. Note that there's also a non-OOPIF Flash fullscreen bug, which might get in the way of fixing this - see issue 590446 .
,
May 10 2016
,
May 11 2016
I've looked at this issue again with lfg@ fix for Flash animation. Overall, getting this to work is not too bad. The main issues are: 1. RenderFrameImpl::CreatePepperFullscreenContainer crashes because it tries to use main frame’s document URL, incorrectly assuming that the main frame is local. 2. Mismatched opener routing ID is used when sending ViewHostMsg_ShowFullscreenWidget and ViewHostMsg_CreateFullscreenWidget. These pass in the subframe widget's routing ID from the renderer, but RenderWidgetHelper::OnCreateFullscreenWidgetOnUI tries to use them to resolve a RenderViewHostImpl, can't find it, and aborts. 3. RenderViewHostImpl::OnShowFullscreenWidget does nothing if is_active_ is false. Similarly to RenderViewHostImpl::OnShowWidget, this can be legitimately called from a subframe for a swapped-out RVH, so this check seems incorrect. 4. WebContentsImpl tracks the fullscreen RenderWidgetHostView using a single routing id, fullscreen_widget_routing_id_. The current fullscreen RenderWidgetHostView is then looked up elsewhere using WebContentsImpl::GetFullscreenRenderWidgetHostView, which couples that routing ID with the main frame’s process ID. For OOPIFs, this is incorrect and needs to use the subframe’s process instead.
,
May 11 2016
Great. I'll mark this as blocking launch, since it seems worth having.
,
May 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/899e06f316b1e9353b59a28e88f80796ad1b3d3b commit 899e06f316b1e9353b59a28e88f80796ad1b3d3b Author: alexmos <alexmos@chromium.org> Date: Fri May 13 23:08:09 2016 Remove unused routing ID param from Did(Show|Destroy)FullscreenWidget. Looks like this was introduced in https://chromiumcodereview.appspot.com/12026029, where the routing IDs were used in web_contents_video_capture_device.cc coupled with the process ID from WebContents (i.e., main frame's process), but this has since been removed, and the routing IDs aren't currently used in any observers. With out-of-process iframes, this routing ID should really be coupled with process ID to be meaningful, but since nobody is using it, just remove the parameter. BUG= 593522 Review-Url: https://codereview.chromium.org/1980633002 Cr-Commit-Position: refs/heads/master@{#393677} [modify] https://crrev.com/899e06f316b1e9353b59a28e88f80796ad1b3d3b/chrome/browser/extensions/api/tab_capture/offscreen_tab.cc [modify] https://crrev.com/899e06f316b1e9353b59a28e88f80796ad1b3d3b/chrome/browser/extensions/api/tab_capture/offscreen_tab.h [modify] https://crrev.com/899e06f316b1e9353b59a28e88f80796ad1b3d3b/chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc [modify] https://crrev.com/899e06f316b1e9353b59a28e88f80796ad1b3d3b/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm [modify] https://crrev.com/899e06f316b1e9353b59a28e88f80796ad1b3d3b/content/browser/media/capture/web_contents_tracker.cc [modify] https://crrev.com/899e06f316b1e9353b59a28e88f80796ad1b3d3b/content/browser/media/capture/web_contents_tracker.h [modify] https://crrev.com/899e06f316b1e9353b59a28e88f80796ad1b3d3b/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/899e06f316b1e9353b59a28e88f80796ad1b3d3b/content/public/browser/web_contents_observer.h [modify] https://crrev.com/899e06f316b1e9353b59a28e88f80796ad1b3d3b/ui/views/controls/webview/webview.cc [modify] https://crrev.com/899e06f316b1e9353b59a28e88f80796ad1b3d3b/ui/views/controls/webview/webview.h
,
May 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c9e76ecdcb771c7fcd975afeaeb2516af6220ab9 commit c9e76ecdcb771c7fcd975afeaeb2516af6220ab9 Author: alexmos <alexmos@chromium.org> Date: Mon May 16 22:59:37 2016 Fix Fullscreen Flash for out-of-process iframes. This CL: - fixes routing ID mismatch when sending ViewHostMsg_ShowFullscreenWidget - fixes main frame URL access in CreatePepperFullscreenContainer - allows swapped-out RVH to process OnShowFullscreenWidget - fixes WebContentsImpl to track the fullscreen RenderWidgetHostView using a {process_id, routing_id} pair, rather than just the widget routing_id. See https://crbug.com/593522#c3 for a slightly more detailed summary. BUG= 593522 Review-Url: https://codereview.chromium.org/1973813002 Cr-Commit-Position: refs/heads/master@{#393965} [modify] https://crrev.com/c9e76ecdcb771c7fcd975afeaeb2516af6220ab9/content/browser/renderer_host/render_view_host_impl.cc [modify] https://crrev.com/c9e76ecdcb771c7fcd975afeaeb2516af6220ab9/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/c9e76ecdcb771c7fcd975afeaeb2516af6220ab9/content/browser/web_contents/web_contents_impl.h [modify] https://crrev.com/c9e76ecdcb771c7fcd975afeaeb2516af6220ab9/content/renderer/render_frame_impl.cc
,
May 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c2a8cec37a56e92d29d91b936734cd1e5ee28734 commit c2a8cec37a56e92d29d91b936734cd1e5ee28734 Author: alexmos <alexmos@chromium.org> Date: Mon May 23 22:19:53 2016 Track pending WebContents and widgets by (process_id, routing_id) pair. Currently, WebContentsImpl tracks pending widgets (widgets that are created but not yet shown) in the pending_widget_views_ map, which is keyed by the new widget's routing_id only. Similarly, pending WebContents are tracked in pending_contents_, also keyed by a single routing_id. This is prone to a race when two OOPIFs in different processes (but part of the same WebContents) create new widgets, and the two widgets happen to use the same routing ID, colliding in one of these maps. This CL changes both of these maps to be keyed by (process_id, routing_id). BUG= 612276 , 593522 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/1986643002 Cr-Commit-Position: refs/heads/master@{#395435} [modify] https://crrev.com/c2a8cec37a56e92d29d91b936734cd1e5ee28734/content/browser/browser_plugin/browser_plugin_guest.cc [modify] https://crrev.com/c2a8cec37a56e92d29d91b936734cd1e5ee28734/content/browser/frame_host/interstitial_page_impl.cc [modify] https://crrev.com/c2a8cec37a56e92d29d91b936734cd1e5ee28734/content/browser/frame_host/interstitial_page_impl.h [modify] https://crrev.com/c2a8cec37a56e92d29d91b936734cd1e5ee28734/content/browser/renderer_host/render_view_host_delegate.h [modify] https://crrev.com/c2a8cec37a56e92d29d91b936734cd1e5ee28734/content/browser/renderer_host/render_view_host_impl.cc [modify] https://crrev.com/c2a8cec37a56e92d29d91b936734cd1e5ee28734/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/c2a8cec37a56e92d29d91b936734cd1e5ee28734/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/c2a8cec37a56e92d29d91b936734cd1e5ee28734/content/browser/web_contents/web_contents_impl.h [modify] https://crrev.com/c2a8cec37a56e92d29d91b936734cd1e5ee28734/content/browser/web_contents/web_contents_impl_unittest.cc [modify] https://crrev.com/c2a8cec37a56e92d29d91b936734cd1e5ee28734/content/test/data/site_isolation/page-with-select.html [modify] https://crrev.com/c2a8cec37a56e92d29d91b936734cd1e5ee28734/content/test/test_web_contents.cc [modify] https://crrev.com/c2a8cec37a56e92d29d91b936734cd1e5ee28734/content/test/test_web_contents.h
,
May 25 2016
At this point, the actual fullscreen Flash functionality is fixed, so I'm removing the blocking launch label. I've started working on a test for this in a follow-up CL, which should be the only remaining piece of this work. I'll keep this bug open until I finish that.
,
May 25 2016
Copying over a few comments about the test: I got a variant of FlashFullscreenInteractiveBrowserTest.FullscreenWithinTab_EscapeKeyExitsFullscreen to work in a subframe with --site-per-process, and this involves several other fixes other than writing the actual test: - navigator.plugins and navigator.mimeTypes didn't work for OOPIF. This is now fixed - see issue 612200 . - simulated plugin events (PepperPluginInstanceImpl::SimulateInputEvent) need to be fixed for OOPIFs. - adding plumbing and fixing a few assumptions in ppapi_test.cc to allow running in a subframe.
,
Jul 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d93138eb45d464a042ff85086cb19e5d74d6af1f commit d93138eb45d464a042ff85086cb19e5d74d6af1f Author: alexmos <alexmos@chromium.org> Date: Wed Jul 13 15:38:47 2016 Add a test for Flash Fullscreen from a cross-site subframe. The underlying issues with Flash fullscreen and OOPIFs were already fixed by https://codereview.chromium.org/1973813002, and this follows up with a test. Note that the test doesn't actually turn on --site-per-process so that we get coverage both with OOPIFs (on Site Isolation FYI bots) and without (on regular bots). BUG= 593522 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2123093005 Cr-Commit-Position: refs/heads/master@{#405163} [modify] https://crrev.com/d93138eb45d464a042ff85086cb19e5d74d6af1f/chrome/browser/ui/exclusive_access/flash_fullscreen_interactive_browsertest.cc [modify] https://crrev.com/d93138eb45d464a042ff85086cb19e5d74d6af1f/chrome/interactive_ui_tests.isolate [modify] https://crrev.com/d93138eb45d464a042ff85086cb19e5d74d6af1f/chrome/test/BUILD.gn [modify] https://crrev.com/d93138eb45d464a042ff85086cb19e5d74d6af1f/content/renderer/pepper/pepper_plugin_instance_impl.cc [modify] https://crrev.com/d93138eb45d464a042ff85086cb19e5d74d6af1f/ppapi/tests/test_case.html
,
Jul 13 2016
,
Jul 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d93138eb45d464a042ff85086cb19e5d74d6af1f commit d93138eb45d464a042ff85086cb19e5d74d6af1f Author: alexmos <alexmos@chromium.org> Date: Wed Jul 13 15:38:47 2016 Add a test for Flash Fullscreen from a cross-site subframe. The underlying issues with Flash fullscreen and OOPIFs were already fixed by https://codereview.chromium.org/1973813002, and this follows up with a test. Note that the test doesn't actually turn on --site-per-process so that we get coverage both with OOPIFs (on Site Isolation FYI bots) and without (on regular bots). BUG= 593522 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2123093005 Cr-Commit-Position: refs/heads/master@{#405163} [modify] https://crrev.com/d93138eb45d464a042ff85086cb19e5d74d6af1f/chrome/browser/ui/exclusive_access/flash_fullscreen_interactive_browsertest.cc [modify] https://crrev.com/d93138eb45d464a042ff85086cb19e5d74d6af1f/chrome/interactive_ui_tests.isolate [modify] https://crrev.com/d93138eb45d464a042ff85086cb19e5d74d6af1f/chrome/test/BUILD.gn [modify] https://crrev.com/d93138eb45d464a042ff85086cb19e5d74d6af1f/content/renderer/pepper/pepper_plugin_instance_impl.cc [modify] https://crrev.com/d93138eb45d464a042ff85086cb19e5d74d6af1f/ppapi/tests/test_case.html |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by alex...@chromium.org
, Mar 10 2016