WebViewInternalAddContentScriptsFunction might target a wrong renderer process |
|||
Issue description
Today WebViewInternalAddContentScriptsFunction::Run calls WebViewContentScriptManager::AddContentScripts with the *main frame* process's id as an argument.
We should double-check if
sender_web_contents->GetMainFrame()->GetProcess()->GetID()
should be replaced with
render_frame_host()->GetProcess()->GetID()
The proposed change above assumes that the process id is meant to be associated with the frame (and its routing id). I am not sure about this assumption, because:
1) the name of the parameter is |embedder_process_id|
2) |embedder_process_id| is paired with |view_instance_id| in WebViewContentScriptManager::AddContentScripts. |view_instance_id| seems to be "the instance ID of the guest <webview> process" (declared in extensions/common/api/web_view_internal.json)
Note - a similar problem is present in WebViewInternalRemoveContentScriptsFunction::Run - it calls WebViewContentScriptManager::RemoveContentScripts with the *main frame* process's id as an argument.
,
Sep 27 2017
The tryjob looks like a good idea. I think lfg might have a strong idea of what the right approach is for this class.
,
Sep 27 2017
WebViewContentScriptManager seems to be built with the assumption that the <webview>'s embedder is in a single process (i.e. not OOPIF-safe). However, I don't think there's a way to embed a <webview> inside an OOPIF, since OOPIFs shouldn't occur in platform apps, so this is likely not a bug. That said, I think your change should be fine and we should land it, as it's the right thing to do.
,
Sep 27 2017
this sgtm as long as lfg@'s happy, and I think the patch shouldn't cause any problems. Łukasz, okay if we assign this to you, since you already have the fix up? :)
,
Sep 27 2017
The tryjobs look green so far...
,
Oct 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dd3a06f94961076c2d22dc31bd17138be165eabb commit dd3a06f94961076c2d22dc31bd17138be165eabb Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Wed Oct 04 00:33:40 2017 Use a renderer process of render_frame_host (instead of GetMainFrame). Bug: 769385 Change-Id: Ibdc12ab30f48b89c6af8e29a546bf933de02b5c9 Reviewed-on: https://chromium-review.googlesource.com/688103 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Lucas Gadani <lfg@chromium.org> Cr-Commit-Position: refs/heads/master@{#506248} [modify] https://crrev.com/dd3a06f94961076c2d22dc31bd17138be165eabb/extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
,
Oct 4 2017
I think this can be marked as fixed, right? |
|||
►
Sign in to add a comment |
|||
Comment 1 by lukasza@chromium.org
, Sep 27 2017