Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2156803002:
[HBD] Remove PluginCache and reload plugin list when origin changes (Closed)
DescriptionRemoves PluginCache and reload PluginData's plugin list whenever the origin changes. Use origin url to retrieve new plugin list.
This is needed because, after HTML5 by Default is implemented, the plugin list
will depend on the current web page url (settings url exceptions).
Here is more information about HTML5 by Default:
go/hbd-design-doc
BUG=626728
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/1c684eba8afc4c730b3ca2e68814e8412020552c
Cr-Commit-Position: refs/heads/master@{#415128}
Patch Set 1 #Patch Set 2 : Use m_refresh for plugin list init #Patch Set 3 : Set m_refresh to false after plugin list is retrieved #
Total comments: 2
Patch Set 4 : Reload plugin list and send message with an url whenever origin changes #
Total comments: 4
Patch Set 5 : Move stored origin to Page and fix red trybots #Patch Set 6 : Rename url parameters to main_frame_origin #
Total comments: 8
Patch Set 7 : Address issues pointed in comments #
Total comments: 25
Patch Set 8 : Changes per alexmos@'s comments #
Total comments: 12
Patch Set 9 : Changes per alexmos@'s comments + DOMImplementation::createDocument fix #
Total comments: 8
Patch Set 10 : Rebase #Patch Set 11 : Move stored SecurityOrigin to PluginData #
Total comments: 2
Patch Set 12 : Changes per alexmos@'s comments #
Total comments: 2
Patch Set 13 : Changes per alexmos@'s comments #
Total comments: 3
Patch Set 14 : Rebase #Patch Set 15 : Changes per dcheng@'s comment #Patch Set 16 : Changes per dcheng@'s comment #
Total comments: 2
Patch Set 17 : Changes per dcheng@'s comment #
Total comments: 5
Patch Set 18 : Transfered some changes from the dependant patchset to make it independent #Patch Set 19 : Changes per dcheng@'s comment #
Total comments: 2
Patch Set 20 : Rebase #Patch Set 21 : mainFrameOrigin documentation #
Total comments: 8
Patch Set 22 : m_refresh to s_refresh #Patch Set 23 : refresh method and s_refresh variable renaming + comment #Patch Set 24 : Remove static refresh variable #
Total comments: 2
Patch Set 25 : Use a local RefPtr to save origin before using get() #Patch Set 26 : Add comment #Messages
Total messages: 169 (96 generated)
The CQ bit was checked by trizzofo@google.com 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: This issue passed the CQ dry run.
Description was changed from ========== Remove PluginCache BUG=626728 ========== to ========== Removes PluginCache. This is needed because, after HTML5 by Default is implemented, the plugin list will depend on the current web page url (settings url exceptions). Here is more information about HTML5 by Default: https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss BUG=626728 ==========
trizzofo@google.com changed reviewers: + tommycli@chromium.org
tommycli, PTAL
https://codereview.chromium.org/2156803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right): https://codereview.chromium.org/2156803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/plugins/PluginData.cpp:84: Vector<PluginInfo> plugins; I think we can do something like: if (m_refresh) initPlugins(); ... use m_plugins below ... I think that captures the intent of the old code.
The CQ bit was checked by trizzofo@google.com 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: This issue passed the CQ dry run.
Description was changed from ========== Removes PluginCache. This is needed because, after HTML5 by Default is implemented, the plugin list will depend on the current web page url (settings url exceptions). Here is more information about HTML5 by Default: https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss BUG=626728 ========== to ========== Removes PluginCache. This is needed because, after HTML5 by Default is implemented, the plugin list will depend on the current web page url (settings url exceptions). Here is more information about HTML5 by Default: https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss BUG=626728 ==========
It may be good to get a Blink reviewer on this CL, since we will need their approval anyway.
On 2016/07/22 00:41:57, Lei Zhang wrote: > It may be good to get a Blink reviewer on this CL, since we will need their > approval anyway. Actually, this CL still needs some more patch sets. tommycli and I are trying to figure out a good way to solve the problems that HBD has with Plugin caches.
Description was changed from ========== Removes PluginCache. This is needed because, after HTML5 by Default is implemented, the plugin list will depend on the current web page url (settings url exceptions). Here is more information about HTML5 by Default: https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss BUG=626728 ========== to ========== Removes PluginCache. This is needed because, after HTML5 by Default is implemented, the plugin list will depend on the current web page url (settings url exceptions). Here is more information about HTML5 by Default: https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Removes PluginCache. This is needed because, after HTML5 by Default is implemented, the plugin list will depend on the current web page url (settings url exceptions). Here is more information about HTML5 by Default: https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Removes PluginCache and reload PluginData's plugin list whenever the origin changes. Use origin url to retrieve new plugin list. This is needed because, after HTML5 by Default is implemented, the plugin list will depend on the current web page url (settings url exceptions). Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by trizzofo@google.com 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
tommycli PTAL https://codereview.chromium.org/2156803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right): https://codereview.chromium.org/2156803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/plugins/PluginData.cpp:84: Vector<PluginInfo> plugins; On 2016/07/18 19:29:04, tommycli wrote: > I think we can do something like: > > if (m_refresh) > initPlugins(); > > ... use m_plugins below ... > > I think that captures the intent of the old code. Unfortunately we can't do that because getPluginMimeTypeFromExtension is static.
https://codereview.chromium.org/2156803002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_message_filter.h (right): https://codereview.chromium.org/2156803002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_message_filter.h:128: void OnGetPlugins(bool refresh, GURL url, IPC::Message* reply_msg); Since |url| is always the main frame origin, maybe we should call this: mainFrameOrigin or topLevelOrigin ... or something reasonable along those lines. https://codereview.chromium.org/2156803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/plugins/PluginData.h (right): https://codereview.chromium.org/2156803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/plugins/PluginData.h:76: WebSecurityOrigin m_origin; my suggestion: Don't save m_origin here. Save it on Page. Just discard the cached m_pluginData within Page when the origin changes.
The CQ bit was checked by trizzofo@google.com 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 checked by trizzofo@google.com 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...
https://codereview.chromium.org/2156803002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_message_filter.h (right): https://codereview.chromium.org/2156803002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_message_filter.h:128: void OnGetPlugins(bool refresh, GURL url, IPC::Message* reply_msg); On 2016/07/29 23:38:19, tommycli wrote: > Since |url| is always the main frame origin, maybe we should call this: > > mainFrameOrigin or topLevelOrigin ... or something reasonable along those lines. Done. https://codereview.chromium.org/2156803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/plugins/PluginData.h (right): https://codereview.chromium.org/2156803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/plugins/PluginData.h:76: WebSecurityOrigin m_origin; On 2016/07/29 23:38:19, tommycli wrote: > my suggestion: Don't save m_origin here. Save it on Page. Just discard the > cached m_pluginData within Page when the origin changes. Done.
some more comments for you https://codereview.chromium.org/2156803002/diff/100001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2156803002/diff/100001/content/common/frame_m... content/common/frame_messages.h:1229: GURL /* url */, this can be url::Origin /* main_frame_origin */ right? https://codereview.chromium.org/2156803002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2156803002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:250: RefPtr<SecurityOrigin> origin = context ? context->getSecurityOrigin() : SecurityOrigin::createUnique().get(); Add comment that context can be null in tests? https://codereview.chromium.org/2156803002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:255: m_origin = origin; Can we move this inside the conditional also? I think we only want to replace the stored origin pointer when we also replace the m_pluginData. https://codereview.chromium.org/2156803002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.h (right): https://codereview.chromium.org/2156803002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.h:69: void initPlugins(const WebSecurityOrigin*); Since this is only called in the constructor, can we just fold this method into the constructor?
The CQ bit was checked by trizzofo@google.com 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...
trizzofo@google.com changed reviewers: + alexmos@chromium.org, dcheng@chromium.org
dcheng, PTAL at blink changes. alexmos, PTAL at frame_host files. In this approach, a SecurityOrigin RefPtr is stored in Page and the PluginData is recreated whenever there's a navigation to another origin. Let me know what you think. https://codereview.chromium.org/2156803002/diff/100001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2156803002/diff/100001/content/common/frame_m... content/common/frame_messages.h:1229: GURL /* url */, On 2016/08/01 23:44:09, tommycli wrote: > this can be url::Origin /* main_frame_origin */ right? Done. https://codereview.chromium.org/2156803002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2156803002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:250: RefPtr<SecurityOrigin> origin = context ? context->getSecurityOrigin() : SecurityOrigin::createUnique().get(); On 2016/08/01 23:44:09, tommycli wrote: > Add comment that context can be null in tests? Done. https://codereview.chromium.org/2156803002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:255: m_origin = origin; On 2016/08/01 23:44:09, tommycli wrote: > Can we move this inside the conditional also? I think we only want to replace > the stored origin pointer when we also replace the m_pluginData. I thought about updating it every time because, even though the origin may stay the same, the SecurityOrigin instance changes with every navigation. So updating it would make sure that the RefPtr is always pointing to the most recent SecurityOrigin https://codereview.chromium.org/2156803002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.h (right): https://codereview.chromium.org/2156803002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.h:69: void initPlugins(const WebSecurityOrigin*); On 2016/08/01 23:44:09, tommycli wrote: > Since this is only called in the constructor, can we just fold this method into > the constructor? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm thanks!
https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.cc:508: filter->IsPluginAvailable( Looking at IsPluginAvailable implementations, they don't use the policy_url param that's populated here atm - is that going to come in another CL? https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.cc:510: GURL(main_frame_origin.Serialize()), &plugin)) { Is there a way to just use url::Origins in IsPluginAvailable as well? It'd be nice to minimize these kinds of origin conversions when possible, as they might lose information. In particular, a GURL doesn't track uniqueness: a unique origin can't access another unique origin, but a GURL created from one unique origin (serialized as "null") would be invalid and would be considered equal to a GURL created from another unique origin. So you'd need to be very careful in comparing them. https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_message_filter.h (right): https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.h:129: url::Origin main_frame_origin, const url::Origin&, here and below? https://codereview.chromium.org/2156803002/diff/120001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2156803002/diff/120001/content/common/frame_m... content/common/frame_messages.h:1226: // Used to get the list of plugins nit: update this comment to explain what the main_frame_origin is used for. https://codereview.chromium.org/2156803002/diff/120001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/2156803002/diff/120001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:814: url::Origin main_frame_origin = origin ? url::Origin(*origin) : url::Origin(); Is getPluginMimeTypeFromExtension the only reason why origin might be null? Maybe it's better to just pass a unique origin directly from there, which would avoid the conditional here and let you pass the origin as a const ref. https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:251: // whenever it's null, an unique SecurityOrigin is created instead. Just curious: for HBD, what set of plugins would be visible to a unique origin? https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.h (right): https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.h:263: RefPtr<SecurityOrigin> m_origin; I'll leave this to Blink reviewers, but storing m_origin on Page seems a little strange to me, as Page contains multiple Frames, each with its own origin, so to someone looking at Page.h it's unclear which origin this refers to, which might lead to misusing this later. IIUC, this is also not the same as main frame origin, but rather what the main frame origin was the last time you queried navigator.plugins. tommycli: I noticed you suggested moving the m_origin here from PluginData - any reasons for that, as to me it actually makes a little more sense to keep it localized inside PluginData? https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right): https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:31: PluginData::PluginData(const WebSecurityOrigin* origin) nit: empty line before this. https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:82: Platform::current()->getPluginList(PluginData::m_refresh, nullptr, &builder); Why doesn't this case need the origin? (Or rather, why is it ok to use the unique origin for this case?) https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:331: virtual void getPluginList(bool refresh, const WebSecurityOrigin* origin, WebPluginListBuilder*) {} I'd name this main_frame_origin here and in all related places as well, so it's clear which origin this refers to.
https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.h (right): https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.h:263: RefPtr<SecurityOrigin> m_origin; On 2016/08/02 22:06:28, alexmos wrote: > I'll leave this to Blink reviewers, but storing m_origin on Page seems a little > strange to me, as Page contains multiple Frames, each with its own origin, so to > someone looking at Page.h it's unclear which origin this refers to, which might > lead to misusing this later. IIUC, this is also not the same as main frame > origin, but rather what the main frame origin was the last time you queried > navigator.plugins. tommycli: I noticed you suggested moving the m_origin here > from PluginData - any reasons for that, as to me it actually makes a little more > sense to keep it localized inside PluginData? Thinking it over again, I think I was wrong. Maybe this should belong in PluginData. My latest thinking is a usage pattern such as: if (!m_pluginData || !currentOrigin.isEqual(m_pluginData->origin())) m_pluginData = new PluginData(currentOrigin) https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right): https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:82: Platform::current()->getPluginList(PluginData::m_refresh, nullptr, &builder); On 2016/08/02 22:06:28, alexmos wrote: > Why doesn't this case need the origin? (Or rather, why is it ok to use the > unique origin for this case?) Hi. This is used to fill-in the MIME types of unknown extensions, whereas everything else is used to inform how to render the page. It kind of sucks that it passes any origin at all, since the ideal behavior would be: "Ignore content settings, and just give me the whole list of plugins". But stepping back: Looking at the callsites, this only appears to be used as a last-resort when the extension doesn't match any well known extension. Since now the only PPAPI plugins are Flash, NaCl and Widevine, I would be surprised if this function ever does anything useful. I think if we deleted it right now, no one would notice. It's not like the NPAPI days when users had plugins for Unity, Quicktime, etc. There might be an edge case where we would want to add .swf, .pexe, and .nexe extensions to the well known mime types database, but I actually think no one would notice that either. tldr: Let's just delete this function and I don't think anyone will notice.
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by trizzofo@google.com 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...
https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.cc:508: filter->IsPluginAvailable( On 2016/08/02 22:06:27, alexmos wrote: > Looking at IsPluginAvailable implementations, they don't use the policy_url > param that's populated here atm - is that going to come in another CL? Yes, we are working on it: https://crrev.com/2208463002 https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.cc:510: GURL(main_frame_origin.Serialize()), &plugin)) { On 2016/08/02 22:06:27, alexmos wrote: > Is there a way to just use url::Origins in IsPluginAvailable as well? It'd be > nice to minimize these kinds of origin conversions when possible, as they might > lose information. In particular, a GURL doesn't track uniqueness: a unique > origin can't access another unique origin, but a GURL created from one unique > origin (serialized as "null") would be invalid and would be considered equal to > a GURL created from another unique origin. So you'd need to be very careful in > comparing them. The only reason why we're using unique origins is because it's empty, so, when it reaches isPluginAvailable(), the default content setting is used. For readability reasons, tommycli suggested changing it to a non-unique empty origin. If we change it, would it still make sense to use url::Origin instead of GURL inside isPluginAvailable()? https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_message_filter.h (right): https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.h:129: url::Origin main_frame_origin, On 2016/08/02 22:06:28, alexmos wrote: > const url::Origin&, here and below? Done. https://codereview.chromium.org/2156803002/diff/120001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2156803002/diff/120001/content/common/frame_m... content/common/frame_messages.h:1226: // Used to get the list of plugins On 2016/08/02 22:06:28, alexmos wrote: > nit: update this comment to explain what the main_frame_origin is used for. Done. https://codereview.chromium.org/2156803002/diff/120001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/2156803002/diff/120001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:814: url::Origin main_frame_origin = origin ? url::Origin(*origin) : url::Origin(); On 2016/08/02 22:06:28, alexmos wrote: > Is getPluginMimeTypeFromExtension the only reason why origin might be null? > Maybe it's better to just pass a unique origin directly from there, which would > avoid the conditional here and let you pass the origin as a const ref. Yes, getPluginMimeTypeFromExtension was the only reason. Changed it so it passes a const ref. Thanks for the suggestion! https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:251: // whenever it's null, an unique SecurityOrigin is created instead. On 2016/08/02 22:06:28, alexmos wrote: > Just curious: for HBD, what set of plugins would be visible to a unique origin? We want the default settings whenever the origin is empty. We are considering using a non-unique empty origin instead. Please take a look at my comment about isPluginAvailable() in render_frame_message_filter.cc. https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right): https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:31: PluginData::PluginData(const WebSecurityOrigin* origin) On 2016/08/02 22:06:28, alexmos wrote: > nit: empty line before this. Done. https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:331: virtual void getPluginList(bool refresh, const WebSecurityOrigin* origin, WebPluginListBuilder*) {} On 2016/08/02 22:06:28, alexmos wrote: > I'd name this main_frame_origin here and in all related places as well, so it's > clear which origin this refers to. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
I'm OOO but some drive-by thoughts. https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:251: // whenever it's null, an unique SecurityOrigin is created instead. Which tests? Can we fix the tests? Putting test-only logic in not test code makes me sad. https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.h (right): https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.h:263: RefPtr<SecurityOrigin> m_mainFrameOrigin; I would prefer we find a way to write this without caching the main frame origin like this. For one, it's not actually accurate since it only gets updated if pluginData is called.
https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:251: // whenever it's null, an unique SecurityOrigin is created instead. On 2016/08/03 14:34:58, dcheng (OOO Aug 1 - Aug 11) wrote: > Which tests? Can we fix the tests? Putting test-only logic in not test code > makes me sad. There are quite a few of them. You can see it in Patch Set 4 trybots. Apparently, LocalFrame::securityContext() returns nullptr when it's LocalDomWindow is null. Can that happen outside tests? https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.h (right): https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.h:263: RefPtr<SecurityOrigin> m_mainFrameOrigin; On 2016/08/03 14:34:58, dcheng (OOO Aug 1 - Aug 11) wrote: > I would prefer we find a way to write this without caching the main frame origin > like this. For one, it's not actually accurate since it only gets updated if > pluginData is called. We are considering storing it in PluginData. Take a look at tommycli's comment in Page.h in Patch Set 7: https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... Would that be a better approach?
https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:251: // whenever it's null, an unique SecurityOrigin is created instead. On 2016/08/03 19:11:18, trizzofo wrote: > On 2016/08/03 14:34:58, dcheng (OOO Aug 1 - Aug 11) wrote: > > Which tests? Can we fix the tests? Putting test-only logic in not test code > > makes me sad. > > There are quite a few of them. You can see it in Patch Set 4 trybots. > Apparently, LocalFrame::securityContext() returns nullptr when it's > LocalDomWindow is null. Can that happen outside tests? I agree it would be ideal to fix the tests. dcheng: Would a TODO be satisfactory? https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.h (right): https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.h:263: RefPtr<SecurityOrigin> m_mainFrameOrigin; On 2016/08/03 19:11:18, trizzofo wrote: > On 2016/08/03 14:34:58, dcheng (OOO Aug 1 - Aug 11) wrote: > > I would prefer we find a way to write this without caching the main frame > origin > > like this. For one, it's not actually accurate since it only gets updated if > > pluginData is called. > > We are considering storing it in PluginData. Take a look at tommycli's comment > in Page.h in Patch Set 7: > > https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... > > Would that be a better approach? If we put m_origin in PluginData, hopefully it's then clear that it's only the PluginData associated with that origin. I agree that it's confusing as-is. I suggested to Tomas to put m_origin in Page, so it was my bad suggestion.
Thanks, a few more comments below. Note that I'm OOO tomorrow and Friday, and back on 8-8. https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.cc:510: GURL(main_frame_origin.Serialize()), &plugin)) { On 2016/08/03 00:08:37, trizzofo wrote: > On 2016/08/02 22:06:27, alexmos wrote: > > Is there a way to just use url::Origins in IsPluginAvailable as well? It'd be > > nice to minimize these kinds of origin conversions when possible, as they > might > > lose information. In particular, a GURL doesn't track uniqueness: a unique > > origin can't access another unique origin, but a GURL created from one unique > > origin (serialized as "null") would be invalid and would be considered equal > to > > a GURL created from another unique origin. So you'd need to be very careful > in > > comparing them. > > The only reason why we're using unique origins is because it's empty, so, when > it reaches isPluginAvailable(), the default content setting is used. For > readability reasons, tommycli suggested changing it to a non-unique empty > origin. If we change it, would it still make sense to use url::Origin instead of > GURL inside isPluginAvailable()? Ah, looking at your https://crrev.com/2208463002, it looks like you'll end up passing this origin into content settings, which means you'll have to convert into a GURL anyway, at least until https://bugs.chromium.org/p/chromium/issues/detail?id=621724 is fixed so that content settings support url::Origins. Looking a bit more at isPluginAvailable, it seems that making it take url::Origins will touch a bunch more unrelated places, so I'm fine leaving this for a future CL, for whoever is going to work on 621724. What do you mean by a non-unique, empty origin? Looking at url::Origin's constructor, it will default to a unique origin unless the scheme/host/port are actually valid. (FWIW, I think it's fine to use a unique origin to get to default content settings - I think that's also done in some other content settings lookups.) https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.h (right): https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.h:263: RefPtr<SecurityOrigin> m_origin; On 2016/08/02 23:03:16, tommycli wrote: > On 2016/08/02 22:06:28, alexmos wrote: > > I'll leave this to Blink reviewers, but storing m_origin on Page seems a > little > > strange to me, as Page contains multiple Frames, each with its own origin, so > to > > someone looking at Page.h it's unclear which origin this refers to, which > might > > lead to misusing this later. IIUC, this is also not the same as main frame > > origin, but rather what the main frame origin was the last time you queried > > navigator.plugins. tommycli: I noticed you suggested moving the m_origin here > > from PluginData - any reasons for that, as to me it actually makes a little > more > > sense to keep it localized inside PluginData? > > Thinking it over again, I think I was wrong. Maybe this should belong in > PluginData. > > My latest thinking is a usage pattern such as: > > if (!m_pluginData || !currentOrigin.isEqual(m_pluginData->origin())) > m_pluginData = new PluginData(currentOrigin) Yes, I think something like that makes more sense; let's try it. https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right): https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:82: Platform::current()->getPluginList(PluginData::m_refresh, nullptr, &builder); On 2016/08/02 23:03:16, tommycli wrote: > On 2016/08/02 22:06:28, alexmos wrote: > > Why doesn't this case need the origin? (Or rather, why is it ok to use the > > unique origin for this case?) > > Hi. This is used to fill-in the MIME types of unknown extensions, whereas > everything else is used to inform how to render the page. > > It kind of sucks that it passes any origin at all, since the ideal behavior > would be: "Ignore content settings, and just give me the whole list of plugins". > > But stepping back: Looking at the callsites, this only appears to be used as a > last-resort when the extension doesn't match any well known extension. > > Since now the only PPAPI plugins are Flash, NaCl and Widevine, I would be > surprised if this function ever does anything useful. I think if we deleted it > right now, no one would notice. It's not like the NPAPI days when users had > plugins for Unity, Quicktime, etc. > > There might be an edge case where we would want to add .swf, .pexe, and .nexe > extensions to the well known mime types database, but I actually think no one > would notice that either. > > tldr: Let's just delete this function and I don't think anyone will notice. Sounds reasonable to me. https://codereview.chromium.org/2156803002/diff/160001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2156803002/diff/160001/content/common/frame_m... content/common/frame_messages.h:1227: // exceptions for Plugins Content Settings. nit: s/Plugins Content Settings/plugin content settings/ https://codereview.chromium.org/2156803002/diff/160001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/2156803002/diff/160001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:814: url::Origin main_frame_origin = url::Origin(mainFrameOrigin); Does "url::Origin main_frame_origin(mainFrameOrigin)" not work? Or just directly passing mainFrameOrigin into FrameHostMsg_GetPlugins constructor below, since there's a conversion operator defined for WebSecurityOrigin -> url::Origin. https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:251: // whenever it's null, an unique SecurityOrigin is created instead. On 2016/08/03 19:11:18, trizzofo wrote: > On 2016/08/03 14:34:58, dcheng (OOO Aug 1 - Aug 11) wrote: > > Which tests? Can we fix the tests? Putting test-only logic in not test code > > makes me sad. > > There are quite a few of them. You can see it in Patch Set 4 trybots. > Apparently, LocalFrame::securityContext() returns nullptr when it's > LocalDomWindow is null. Can that happen outside tests? LocalFrame::securityContext() just returns the LocalFrame's Document. Looks like all those tests fail with the same stack, during new (likely top-level) Document initialization, when we're deciding what kind of Document to create in DOMImplementation::createDocument: Received signal 11 SEGV_MAPERR 000000000008 #0 0x00000280d0d7 base::debug::(anonymous namespace)::StackDumpSignalHandler() #1 0x7f2b8d81acb0 <unknown> #2 0x00000586bcf9 blink::Page::pluginData() #3 0x00000526f3be blink::DOMImplementation::createDocument() #4 0x000005758b99 blink::LocalDOMWindow::createDocument() #5 0x000005758cac blink::LocalDOMWindow::installNewDocument() #6 0x000005821e86 blink::DocumentLoader::createWriterFor() #7 0x000005821d18 blink::DocumentLoader::ensureWriter() #8 0x000005820df8 blink::DocumentLoader::commitData() #9 0x000005820c53 blink::DocumentLoader::finishedLoading() ... Which makes sense - at this point the new Document isn't even created yet. Note that this happens in browser_tests as well, so it's not like it's caused by some unit test setup weirdness. Can you double-check that this path actually works? Maybe investigate one of the failing browser_tests? At the point in the crash, the new document's origin isn't initialized yet (we haven't decided what kind of document to create yet), and I'm not sure you want this to actually use a unique origin, rather than, say, the url or the owner()'s origin from the DocumentInit.
The CQ bit was checked by trizzofo@google.com 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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Patchset #9 (id:180001) has been deleted
The CQ bit was checked by trizzofo@google.com 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...
ptal https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.cc:510: GURL(main_frame_origin.Serialize()), &plugin)) { On 2016/08/04 05:32:02, alexmos (OOO until 8-8) wrote: > On 2016/08/03 00:08:37, trizzofo wrote: > > On 2016/08/02 22:06:27, alexmos wrote: > > > Is there a way to just use url::Origins in IsPluginAvailable as well? It'd > be > > > nice to minimize these kinds of origin conversions when possible, as they > > might > > > lose information. In particular, a GURL doesn't track uniqueness: a unique > > > origin can't access another unique origin, but a GURL created from one > unique > > > origin (serialized as "null") would be invalid and would be considered equal > > to > > > a GURL created from another unique origin. So you'd need to be very careful > > in > > > comparing them. > > > > The only reason why we're using unique origins is because it's empty, so, when > > it reaches isPluginAvailable(), the default content setting is used. For > > readability reasons, tommycli suggested changing it to a non-unique empty > > origin. If we change it, would it still make sense to use url::Origin instead > of > > GURL inside isPluginAvailable()? > > Ah, looking at your https://crrev.com/2208463002, it looks like you'll end up > passing this origin into content settings, which means you'll have to convert > into a GURL anyway, at least until > https://bugs.chromium.org/p/chromium/issues/detail?id=621724 is fixed so that > content settings support url::Origins. Looking a bit more at isPluginAvailable, > it seems that making it take url::Origins will touch a bunch more unrelated > places, so I'm fine leaving this for a future CL, for whoever is going to work > on 621724. > > What do you mean by a non-unique, empty origin? Looking at url::Origin's > constructor, it will default to a unique origin unless the scheme/host/port are > actually valid. (FWIW, I think it's fine to use a unique origin to get to > default content settings - I think that's also done in some other content > settings lookups.) Yes, I took a better look at url::Origin constructors and you're right. In that case, we should probably leave it as an unique url::Origin. https://codereview.chromium.org/2156803002/diff/160001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2156803002/diff/160001/content/common/frame_m... content/common/frame_messages.h:1227: // exceptions for Plugins Content Settings. On 2016/08/04 05:32:03, alexmos (OOO until 8-8) wrote: > nit: s/Plugins Content Settings/plugin content settings/ Done. https://codereview.chromium.org/2156803002/diff/160001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/2156803002/diff/160001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:814: url::Origin main_frame_origin = url::Origin(mainFrameOrigin); On 2016/08/04 05:32:03, alexmos (OOO until 8-8) wrote: > Does "url::Origin main_frame_origin(mainFrameOrigin)" not work? Or just > directly passing mainFrameOrigin into FrameHostMsg_GetPlugins constructor below, > since there's a conversion operator defined for WebSecurityOrigin -> > url::Origin. Done. Thanks! https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2156803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:251: // whenever it's null, an unique SecurityOrigin is created instead. On 2016/08/04 05:32:03, alexmos (OOO until 8-8) wrote: > On 2016/08/03 19:11:18, trizzofo wrote: > > On 2016/08/03 14:34:58, dcheng (OOO Aug 1 - Aug 11) wrote: > > > Which tests? Can we fix the tests? Putting test-only logic in not test code > > > makes me sad. > > > > There are quite a few of them. You can see it in Patch Set 4 trybots. > > Apparently, LocalFrame::securityContext() returns nullptr when it's > > LocalDomWindow is null. Can that happen outside tests? > > LocalFrame::securityContext() just returns the LocalFrame's Document. > > Looks like all those tests fail with the same stack, during new (likely > top-level) Document initialization, when we're deciding what kind of Document to > create in DOMImplementation::createDocument: > > Received signal 11 SEGV_MAPERR 000000000008 > #0 0x00000280d0d7 base::debug::(anonymous namespace)::StackDumpSignalHandler() > #1 0x7f2b8d81acb0 <unknown> > #2 0x00000586bcf9 blink::Page::pluginData() > #3 0x00000526f3be blink::DOMImplementation::createDocument() > #4 0x000005758b99 blink::LocalDOMWindow::createDocument() > #5 0x000005758cac blink::LocalDOMWindow::installNewDocument() > #6 0x000005821e86 blink::DocumentLoader::createWriterFor() > #7 0x000005821d18 blink::DocumentLoader::ensureWriter() > #8 0x000005820df8 blink::DocumentLoader::commitData() > #9 0x000005820c53 blink::DocumentLoader::finishedLoading() > ... > > Which makes sense - at this point the new Document isn't even created yet. Note > that this happens in browser_tests as well, so it's not like it's caused by some > unit test setup weirdness. > > Can you double-check that this path actually works? Maybe investigate one of > the failing browser_tests? At the point in the crash, the new document's origin > isn't initialized yet (we haven't decided what kind of document to create yet), > and I'm not sure you want this to actually use a unique origin, rather than, > say, the url or the owner()'s origin from the DocumentInit. Yes, you're right. I took a better look at DOMImplemetation::createDocument() and the PluginData is fetched if the document type is different from text/html and application/xhtml+xml. I managed to reproduce the crash locally by opening an image page. To solve this, we created a temporary PluginData instance based on DocumentInit information. Please take a look at patch set 9.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by trizzofo@google.com 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...
Patchset #11 (id:240001) has been deleted
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by trizzofo@google.com 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2156803002/diff/200001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2156803002/diff/200001/content/common/frame_m... content/common/frame_messages.h:1227: // exceptions for plugins content settings. nit: s/plugins/plugin/ https://codereview.chromium.org/2156803002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:230: pluginData = PluginData::create(init.owner() ? init.owner()->getSecurityOrigin() : SecurityOrigin::create(init.url())); Hmm, it looks like now, any time we go through this, we'll bypass the cached PluginData? So potentially we can end up doing more FrameHostMsg_GetPlugins lookups - how big of a problem is this? Could we avoid this by having the Page::pluginData() take the origin and pass the origin you calculate here to init.frame()->page()->pluginData()? Or maybe it's not that bad if it's just for top-level, non-html documents (like image or pdf). Another question is now that you don't go through LocalFrame::pluginData, this path will skip this check: if (!loader().allowPlugins(NotAboutToInstantiatePlugin)) return nullptr; is this ok?
Patchset #11 (id:260001) has been deleted
The CQ bit was checked by trizzofo@google.com 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...
Patchset #11 (id:280001) has been deleted
The CQ bit was checked by trizzofo@google.com to run a CQ dry run
https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.h (right): https://codereview.chromium.org/2156803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.h:263: RefPtr<SecurityOrigin> m_origin; On 2016/08/04 05:32:02, alexmos (OOO until 8-8) wrote: > On 2016/08/02 23:03:16, tommycli wrote: > > On 2016/08/02 22:06:28, alexmos wrote: > > > I'll leave this to Blink reviewers, but storing m_origin on Page seems a > > little > > > strange to me, as Page contains multiple Frames, each with its own origin, > so > > to > > > someone looking at Page.h it's unclear which origin this refers to, which > > might > > > lead to misusing this later. IIUC, this is also not the same as main frame > > > origin, but rather what the main frame origin was the last time you queried > > > navigator.plugins. tommycli: I noticed you suggested moving the m_origin > here > > > from PluginData - any reasons for that, as to me it actually makes a little > > more > > > sense to keep it localized inside PluginData? > > > > Thinking it over again, I think I was wrong. Maybe this should belong in > > PluginData. > > > > My latest thinking is a usage pattern such as: > > > > if (!m_pluginData || !currentOrigin.isEqual(m_pluginData->origin())) > > m_pluginData = new PluginData(currentOrigin) > > Yes, I think something like that makes more sense; let's try it. Done. https://codereview.chromium.org/2156803002/diff/200001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2156803002/diff/200001/content/common/frame_m... content/common/frame_messages.h:1227: // exceptions for plugins content settings. On 2016/08/05 23:49:06, alexmos (OOO until 8-8) wrote: > nit: s/plugins/plugin/ Done. https://codereview.chromium.org/2156803002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:230: pluginData = PluginData::create(init.owner() ? init.owner()->getSecurityOrigin() : SecurityOrigin::create(init.url())); On 2016/08/05 23:49:06, alexmos (OOO until 8-8) wrote: > Hmm, it looks like now, any time we go through this, we'll bypass the cached > PluginData? So potentially we can end up doing more FrameHostMsg_GetPlugins > lookups - how big of a problem is this? Could we avoid this by having the > Page::pluginData() take the origin and pass the origin you calculate here to > init.frame()->page()->pluginData()? Or maybe it's not that bad if it's just for > top-level, non-html documents (like image or pdf). > > Another question is now that you don't go through LocalFrame::pluginData, this > path will skip this check: > > if (!loader().allowPlugins(NotAboutToInstantiatePlugin)) > return nullptr; > > is this ok? Before this approach, I tried overloading LocalFrame::pluginData() and Page::pluginData() to take a SecurityOrigin but, in my opinion, creating a temp PluginData is more readable. I'm not sure about the implications of having one more lookup during document creation though. Maybe tommycli and dcheng(when he's back) can comment on that. If it's not ok to have an additional lookup, I can think of two other approaches: 1) Instead of overloading I can also create another method called pluginDataForOrigin() that takes the origin instead of fetching it from page. 2) Simply add the parameter to pluginData(), but that means that I'll have to touch more code, to fix all callers. Let me know what you think. Regarding your question about LocalFrame::pluginData()'s check, I think it's ok since it was redundant. The same check is being made at line 229.
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2156803002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:230: pluginData = PluginData::create(init.owner() ? init.owner()->getSecurityOrigin() : SecurityOrigin::create(init.url())); On 2016/08/06 01:30:46, trizzofo wrote: > On 2016/08/05 23:49:06, alexmos (OOO until 8-8) wrote: > > Hmm, it looks like now, any time we go through this, we'll bypass the cached > > PluginData? So potentially we can end up doing more FrameHostMsg_GetPlugins > > lookups - how big of a problem is this? Could we avoid this by having the > > Page::pluginData() take the origin and pass the origin you calculate here to > > init.frame()->page()->pluginData()? Or maybe it's not that bad if it's just > for > > top-level, non-html documents (like image or pdf). > > > > Another question is now that you don't go through LocalFrame::pluginData, this > > path will skip this check: > > > > if (!loader().allowPlugins(NotAboutToInstantiatePlugin)) > > return nullptr; > > > > is this ok? > > Before this approach, I tried overloading LocalFrame::pluginData() and > Page::pluginData() to take a SecurityOrigin but, in my opinion, creating a temp > PluginData is more readable. Agreed that overloading pluginData() isn't the best - but see below. > I'm not sure about the implications of having one > more lookup during document creation though. Maybe tommycli and dcheng(when he's > back) can comment on that. Sure, I'm curious if they have any thoughts. If it's just for top-level, non-html documents, this is probably ok. > > If it's not ok to have an additional lookup, I can think of two other > approaches: > > 1) Instead of overloading I can also create another method called > pluginDataForOrigin() that takes the origin instead of fetching it from page. > > 2) Simply add the parameter to pluginData(), but that means that I'll have to > touch more code, to fix all callers. There's only one caller to Page::pluginData(), which is LocalFrame::pluginData(). So it seems like it's not too much trouble to add the origin parameter to Page::pluginData and pass in the main frame origin from LocalFrame::pluginData (and here). > > Let me know what you think. > > Regarding your question about LocalFrame::pluginData()'s check, I think it's ok > since it was redundant. The same check is being made at line 229. Ah, didn't notice that. Thanks! https://codereview.chromium.org/2156803002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right): https://codereview.chromium.org/2156803002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:33: PluginData::PluginData(const RefPtr<SecurityOrigin> mainFrameOrigin) Why not PassRefPtr<SecurityOrigin> or SecurityOrigin*?
alexmos: I commented below. In general, I think making plugin data per-origin is going to have a bit of performance impact: A few extra IPCs. We could move the filtering logic to the renderer, or maintain a renderer-side cache to avoid this, but I recommended Tomas write the simplest, easiest to review code for now, and we could optimize once we've noticed we have an actual performance problem. My theory: The extra IPCs will be meaningless, and the perf benefits of not instantiating Flash will overwhelm the extra IPC costs. https://codereview.chromium.org/2156803002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:230: pluginData = PluginData::create(init.owner() ? init.owner()->getSecurityOrigin() : SecurityOrigin::create(init.url())); On 2016/08/09 01:40:15, alexmos wrote: > On 2016/08/06 01:30:46, trizzofo wrote: > > On 2016/08/05 23:49:06, alexmos (OOO until 8-8) wrote: > > > Hmm, it looks like now, any time we go through this, we'll bypass the cached > > > PluginData? So potentially we can end up doing more FrameHostMsg_GetPlugins > > > lookups - how big of a problem is this? Could we avoid this by having the > > > Page::pluginData() take the origin and pass the origin you calculate here to > > > init.frame()->page()->pluginData()? Or maybe it's not that bad if it's just > > for > > > top-level, non-html documents (like image or pdf). > > > > > > Another question is now that you don't go through LocalFrame::pluginData, > this > > > path will skip this check: > > > > > > if (!loader().allowPlugins(NotAboutToInstantiatePlugin)) > > > return nullptr; > > > > > > is this ok? > > > > Before this approach, I tried overloading LocalFrame::pluginData() and > > Page::pluginData() to take a SecurityOrigin but, in my opinion, creating a > temp > > PluginData is more readable. > Agreed that overloading pluginData() isn't the best - but see below. > > > I'm not sure about the implications of having one > > more lookup during document creation though. Maybe tommycli and dcheng(when > he's > > back) can comment on that. > > Sure, I'm curious if they have any thoughts. If it's just for top-level, > non-html documents, this is probably ok. > Those were my thoughts exactly. Since it was an extra IPC for PDF and exotic mime types, I thought it was fine. This data is cached on the browser process side, so it shouldn't hit the disk. I did suggest moving the PDF clause (and in fact all the plugin clauses) below the Image and Video clauses, but in a separate CL. > > > > If it's not ok to have an additional lookup, I can think of two other > > approaches: > > > > 1) Instead of overloading I can also create another method called > > pluginDataForOrigin() that takes the origin instead of fetching it from page. > > > > 2) Simply add the parameter to pluginData(), but that means that I'll have to > > touch more code, to fix all callers. > > There's only one caller to Page::pluginData(), which is > LocalFrame::pluginData(). So it seems like it's not too much trouble to add the > origin parameter to Page::pluginData and pass in the main frame origin from > LocalFrame::pluginData (and here). > > > > > Let me know what you think. > > > > Regarding your question about LocalFrame::pluginData()'s check, I think it's > ok > > since it was redundant. The same check is being made at line 229. > > Ah, didn't notice that. Thanks!
The CQ bit was checked by trizzofo@google.com 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 checked by trizzofo@google.com 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...
Patchset #12 (id:320001) has been deleted
Patchset #12 (id:340001) has been deleted
The CQ bit was checked by trizzofo@google.com 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...
https://codereview.chromium.org/2156803002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:230: pluginData = PluginData::create(init.owner() ? init.owner()->getSecurityOrigin() : SecurityOrigin::create(init.url())); On 2016/08/09 01:40:15, alexmos wrote: > On 2016/08/06 01:30:46, trizzofo wrote: > > On 2016/08/05 23:49:06, alexmos (OOO until 8-8) wrote: > > > Hmm, it looks like now, any time we go through this, we'll bypass the cached > > > PluginData? So potentially we can end up doing more FrameHostMsg_GetPlugins > > > lookups - how big of a problem is this? Could we avoid this by having the > > > Page::pluginData() take the origin and pass the origin you calculate here to > > > init.frame()->page()->pluginData()? Or maybe it's not that bad if it's just > > for > > > top-level, non-html documents (like image or pdf). > > > > > > Another question is now that you don't go through LocalFrame::pluginData, > this > > > path will skip this check: > > > > > > if (!loader().allowPlugins(NotAboutToInstantiatePlugin)) > > > return nullptr; > > > > > > is this ok? > > > > Before this approach, I tried overloading LocalFrame::pluginData() and > > Page::pluginData() to take a SecurityOrigin but, in my opinion, creating a > temp > > PluginData is more readable. > Agreed that overloading pluginData() isn't the best - but see below. > > > I'm not sure about the implications of having one > > more lookup during document creation though. Maybe tommycli and dcheng(when > he's > > back) can comment on that. > > Sure, I'm curious if they have any thoughts. If it's just for top-level, > non-html documents, this is probably ok. > > > > > If it's not ok to have an additional lookup, I can think of two other > > approaches: > > > > 1) Instead of overloading I can also create another method called > > pluginDataForOrigin() that takes the origin instead of fetching it from page. > > > > 2) Simply add the parameter to pluginData(), but that means that I'll have to > > touch more code, to fix all callers. > > There's only one caller to Page::pluginData(), which is > LocalFrame::pluginData(). So it seems like it's not too much trouble to add the > origin parameter to Page::pluginData and pass in the main frame origin from > LocalFrame::pluginData (and here). That's a good idea. I'm going to give it a try. > > > > > Let me know what you think. > > > > Regarding your question about LocalFrame::pluginData()'s check, I think it's > ok > > since it was redundant. The same check is being made at line 229. > > Ah, didn't notice that. Thanks! https://codereview.chromium.org/2156803002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right): https://codereview.chromium.org/2156803002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:33: PluginData::PluginData(const RefPtr<SecurityOrigin> mainFrameOrigin) On 2016/08/09 01:40:15, alexmos wrote: > Why not PassRefPtr<SecurityOrigin> or SecurityOrigin*? Done. Changed it to SecurityOrigin*.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the explanations, I think this is ready apart from one last issue with which frame's origin to pass below. https://codereview.chromium.org/2156803002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:230: pluginData = PluginData::create(init.owner() ? init.owner()->getSecurityOrigin() : SecurityOrigin::create(init.url())); > Those were my thoughts exactly. Since it was an extra IPC for PDF and exotic > mime types, I thought it was fine. This data is cached on the browser process > side, so it shouldn't hit the disk. > > I did suggest moving the PDF clause (and in fact all the plugin clauses) below > the Image and Video clauses, but in a separate CL. > Acknowledged and agreed. Either approach is fine with me. Looks like Tomas already changed Page::pluginData() to take the extra param to avoid this problem altogether. https://codereview.chromium.org/2156803002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2156803002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrame.cpp:850: return page()->pluginData(securityContext()->getSecurityOrigin()); This should be tree().top()->securityContext()->getSecurityOrigin(), or page()->mainFrame()->securityContext()->getSecurityOrigin(), right? This is what it used to resolve to in PS11 when you did the lookup inside Page::pluginData().
The CQ bit was checked by trizzofo@google.com 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...
https://codereview.chromium.org/2156803002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2156803002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrame.cpp:850: return page()->pluginData(securityContext()->getSecurityOrigin()); On 2016/08/10 00:10:29, alexmos wrote: > This should be tree().top()->securityContext()->getSecurityOrigin(), or > page()->mainFrame()->securityContext()->getSecurityOrigin(), right? This is > what it used to resolve to in PS11 when you did the lookup inside > Page::pluginData(). Yes, you're right. Fixed it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. Were you planning to add tests for this at some point? It'd be nice if this could be covered with a Blink unit test (though I don't know how hard it'd be to mock and verify the reloading part when the origin changes), or if you'll be adding browser tests in your later CLs, that's fine also.
The CQ bit was checked by trizzofo@google.com 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: This issue passed the CQ dry run.
After this CL, HBD needs 2 or 3 more CLs to work properly with the url exceptions. We're planning to add browser tests after that. I'll check if it's possible to add some Blink unit tests to cover this CL, but I think I'll leave it to a separate CL.
dcheng, ptal. Thanks!
dcheng, ptal. Thanks!
Part of me also wonders if we should just be plumbing this through WebFrameClient if we're going to be tying it to the frame origin like this. I don't think we have to do it in this CL, since it'd be rather involved (several things would have to move for this to work), but it's something to keep in mind for the future. https://codereview.chromium.org/2156803002/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/2156803002/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.cc:495: GURL(main_frame_origin.Serialize()), &plugin)) { Can you file a TODO to plumb this as a url::Origin directly? It shouldn't be necessary to go from Origin -> string -> GURL. Also, what happens here if the origin is unique?
Patchset #14 (id:400001) has been deleted
https://codereview.chromium.org/2156803002/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/2156803002/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.cc:495: GURL(main_frame_origin.Serialize()), &plugin)) { On 2016/08/18 21:54:04, dcheng wrote: > Can you file a TODO to plumb this as a url::Origin directly? It shouldn't be > necessary to go from Origin -> string -> GURL. > > Also, what happens here if the origin is unique? Done! When the origin is unique, the default content setting is used. We discussed it here: https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_...
The CQ bit was checked by trizzofo@google.com 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...
https://codereview.chromium.org/2156803002/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/2156803002/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.cc:495: GURL(main_frame_origin.Serialize()), &plugin)) { On 2016/08/18 22:23:44, trizzofo wrote: > On 2016/08/18 21:54:04, dcheng wrote: > > Can you file a TODO to plumb this as a url::Origin directly? It shouldn't be > > necessary to go from Origin -> string -> GURL. > > > > Also, what happens here if the origin is unique? > > Done! > > When the origin is unique, the default content setting is used. We discussed it > here: > https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... Right, but a unique Origin serializes to "null", which is not a valid GURL AFAIK. Do we handle that specially somewhere?
On 2016/08/18 22:26:37, dcheng wrote: > https://codereview.chromium.org/2156803002/diff/380001/content/browser/frame_... > File content/browser/frame_host/render_frame_message_filter.cc (right): > > https://codereview.chromium.org/2156803002/diff/380001/content/browser/frame_... > content/browser/frame_host/render_frame_message_filter.cc:495: > GURL(main_frame_origin.Serialize()), &plugin)) { > On 2016/08/18 22:23:44, trizzofo wrote: > > On 2016/08/18 21:54:04, dcheng wrote: > > > Can you file a TODO to plumb this as a url::Origin directly? It shouldn't be > > > necessary to go from Origin -> string -> GURL. > > > > > > Also, what happens here if the origin is unique? > > > > Done! > > > > When the origin is unique, the default content setting is used. We discussed > it > > here: > > > https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... > > Right, but a unique Origin serializes to "null", which is not a valid GURL > AFAIK. Do we handle that specially somewhere? We aren't handling it yet, I missed that. Thanks! I changed it to send an empty GURL when the url::Origin is unique. Passing an empty GURL assures that IsPluginAvailable() uses the default content setting.
On 2016/08/18 22:59:39, trizzofo wrote: > On 2016/08/18 22:26:37, dcheng wrote: > > > https://codereview.chromium.org/2156803002/diff/380001/content/browser/frame_... > > File content/browser/frame_host/render_frame_message_filter.cc (right): > > > > > https://codereview.chromium.org/2156803002/diff/380001/content/browser/frame_... > > content/browser/frame_host/render_frame_message_filter.cc:495: > > GURL(main_frame_origin.Serialize()), &plugin)) { > > On 2016/08/18 22:23:44, trizzofo wrote: > > > On 2016/08/18 21:54:04, dcheng wrote: > > > > Can you file a TODO to plumb this as a url::Origin directly? It shouldn't > be > > > > necessary to go from Origin -> string -> GURL. > > > > > > > > Also, what happens here if the origin is unique? > > > > > > Done! > > > > > > When the origin is unique, the default content setting is used. We discussed > > it > > > here: > > > > > > https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... > > > > Right, but a unique Origin serializes to "null", which is not a valid GURL > > AFAIK. Do we handle that specially somewhere? > > We aren't handling it yet, I missed that. Thanks! > I changed it to send an empty GURL when the url::Origin is unique. Passing an > empty GURL assures that IsPluginAvailable() uses the default content setting. I'm not sure there's actually a difference between GURL() and the GURL that a unique url::Origin would convert to -- both as invalid GURLs. But it's still probably better to be explicit about it. (And currently it's hard to tell that the default content setting will be used in this case, as the use of that GURL for lookup in IsPluginAvailable isn't implemented yet.)
On 2016/08/18 23:12:55, alexmos (OOO until 7-24) wrote: > On 2016/08/18 22:59:39, trizzofo wrote: > > On 2016/08/18 22:26:37, dcheng wrote: > > > > > > https://codereview.chromium.org/2156803002/diff/380001/content/browser/frame_... > > > File content/browser/frame_host/render_frame_message_filter.cc (right): > > > > > > > > > https://codereview.chromium.org/2156803002/diff/380001/content/browser/frame_... > > > content/browser/frame_host/render_frame_message_filter.cc:495: > > > GURL(main_frame_origin.Serialize()), &plugin)) { > > > On 2016/08/18 22:23:44, trizzofo wrote: > > > > On 2016/08/18 21:54:04, dcheng wrote: > > > > > Can you file a TODO to plumb this as a url::Origin directly? It > shouldn't > > be > > > > > necessary to go from Origin -> string -> GURL. > > > > > > > > > > Also, what happens here if the origin is unique? > > > > > > > > Done! > > > > > > > > When the origin is unique, the default content setting is used. We > discussed > > > it > > > > here: > > > > > > > > > > https://codereview.chromium.org/2156803002/diff/120001/content/browser/frame_... > > > > > > Right, but a unique Origin serializes to "null", which is not a valid GURL > > > AFAIK. Do we handle that specially somewhere? > > > > We aren't handling it yet, I missed that. Thanks! > > I changed it to send an empty GURL when the url::Origin is unique. Passing an > > empty GURL assures that IsPluginAvailable() uses the default content setting. > > I'm not sure there's actually a difference between GURL() and the GURL that a > unique url::Origin would convert to -- both as invalid GURLs. But it's still > probably better to be explicit about it. (And currently it's hard to tell that > the default content setting will be used in this case, as the use of that GURL > for lookup in IsPluginAvailable isn't implemented yet.) In the following CL (https://crrev.com/2208463002), I changed the GetDefaultContentSetting() call inside IsPluginAvailable() to a GetPluginContentSettings() call. That call uses the main frame origin to check for exceptions. While writing that CL, I had a conversation with bauerb@ about the consequences of passing an empty GURL to GetPluginContentSettings() and, apparently, an empty GURL can only be matched with a wildcard. That's why I believe that it will use the default content setting when an empty GURL is passed.
https://codereview.chromium.org/2156803002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:230: pluginData = init.frame()->page()->pluginData(init.owner() ? init.owner()->getSecurityOrigin() : SecurityOrigin::create(init.url()).get()); This is called when we create a new Document in LocalDOMWindow::createDocument. This means that an iframe that points to a .swf would be using its parent's origin to do the plugin data check rather than the top frame's origin. Is that desired?
The CQ bit was checked by trizzofo@google.com 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...
https://codereview.chromium.org/2156803002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:230: pluginData = init.frame()->page()->pluginData(init.owner() ? init.owner()->getSecurityOrigin() : SecurityOrigin::create(init.url()).get()); On 2016/08/19 04:30:03, dcheng wrote: > This is called when we create a new Document in LocalDOMWindow::createDocument. > This means that an iframe that points to a .swf would be using its parent's > origin to do the plugin data check rather than the top frame's origin. Is that > desired? No, that's not desired. I changed it to get the main frame origin instead. ptal. Thanks!
trizzofo@google.com changed reviewers: + esprehn@chromium.org, piman@chromium.org
esprehn and piman, ptal. Thanks!
https://codereview.chromium.org/2156803002/diff/480001/content/browser/frame_... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/2156803002/diff/480001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.cc:455: const url::Origin& main_frame_origin, This origin comes from an untrusted source. Do we need to validate that this is a valid one for the calling renderer?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2156803002/diff/480001/content/browser/frame_... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/2156803002/diff/480001/content/browser/frame_... content/browser/frame_host/render_frame_message_filter.cc:455: const url::Origin& main_frame_origin, On 2016/08/19 21:59:22, piman wrote: > This origin comes from an untrusted source. Do we need to validate that this is > a valid one for the calling renderer? Since OnGetPluginInfo() (that effectively determine if Flash should run or not) doesn't validate the URLs, I believe that it should be fine if we don't validate it here as well. The only thing we are determining here is if Flash is advertised or not and I think that it isn't that critical. Let me know if you have any additional thoughts.
On 2016/08/20 00:00:41, trizzofo wrote: > https://codereview.chromium.org/2156803002/diff/480001/content/browser/frame_... > File content/browser/frame_host/render_frame_message_filter.cc (right): > > https://codereview.chromium.org/2156803002/diff/480001/content/browser/frame_... > content/browser/frame_host/render_frame_message_filter.cc:455: const > url::Origin& main_frame_origin, > On 2016/08/19 21:59:22, piman wrote: > > This origin comes from an untrusted source. Do we need to validate that this > is > > a valid one for the calling renderer? > > Since OnGetPluginInfo() (that effectively determine if Flash should run or not) > doesn't validate the URLs, I believe that it should be fine if we don't validate > it here as well. The only thing we are determining here is if Flash is > advertised or not and I think that it isn't that critical. Let me know if you > have any additional thoughts. I think we ought to if this is a security feature. Otherwise a compromised renderer can instantiate a plugin it was not authorized to. I'll let dcheng comment on the security aspect (in particular relative to OOP iframes), LGTM for the rest.
tommycli@chromium.org changed reviewers: + bauerb@chromium.org
On 2016/08/20 00:21:31, piman wrote: > On 2016/08/20 00:00:41, trizzofo wrote: > > > https://codereview.chromium.org/2156803002/diff/480001/content/browser/frame_... > > File content/browser/frame_host/render_frame_message_filter.cc (right): > > > > > https://codereview.chromium.org/2156803002/diff/480001/content/browser/frame_... > > content/browser/frame_host/render_frame_message_filter.cc:455: const > > url::Origin& main_frame_origin, > > On 2016/08/19 21:59:22, piman wrote: > > > This origin comes from an untrusted source. Do we need to validate that this > > is > > > a valid one for the calling renderer? > > > > Since OnGetPluginInfo() (that effectively determine if Flash should run or > not) > > doesn't validate the URLs, I believe that it should be fine if we don't > validate > > it here as well. The only thing we are determining here is if Flash is > > advertised or not and I think that it isn't that critical. Let me know if you > > have any additional thoughts. > > I think we ought to if this is a security feature. Otherwise a compromised > renderer can instantiate a plugin it was not authorized to. > I'll let dcheng comment on the security aspect (in particular relative to OOP > iframes), LGTM for the rest. +bauerb: As far as I'm aware, there is a separate AuthorizePlugin[1] flow so that a compromised renderer still can't instantiate the plugin. GetPlugins only gets the list of plugins on navigator.plugins() anyways, AFAIK there's a second check in GetPluginInfo (that isn't being modified) when the plugin is actually instantiated. I'll let Bernard comment authoritatively. [1] https://cs.chromium.org/chromium/src/chrome/browser/plugins/chrome_plugin_ser...
On 2016/08/20 00:26:05, tommycli wrote: > On 2016/08/20 00:21:31, piman wrote: > > On 2016/08/20 00:00:41, trizzofo wrote: > > > > > > https://codereview.chromium.org/2156803002/diff/480001/content/browser/frame_... > > > File content/browser/frame_host/render_frame_message_filter.cc (right): > > > > > > > > > https://codereview.chromium.org/2156803002/diff/480001/content/browser/frame_... > > > content/browser/frame_host/render_frame_message_filter.cc:455: const > > > url::Origin& main_frame_origin, > > > On 2016/08/19 21:59:22, piman wrote: > > > > This origin comes from an untrusted source. Do we need to validate that > this > > > is > > > > a valid one for the calling renderer? > > > > > > Since OnGetPluginInfo() (that effectively determine if Flash should run or > > not) > > > doesn't validate the URLs, I believe that it should be fine if we don't > > validate > > > it here as well. The only thing we are determining here is if Flash is > > > advertised or not and I think that it isn't that critical. Let me know if > you > > > have any additional thoughts. > > > > I think we ought to if this is a security feature. Otherwise a compromised > > renderer can instantiate a plugin it was not authorized to. > > I'll let dcheng comment on the security aspect (in particular relative to OOP > > iframes), LGTM for the rest. > > +bauerb: > > As far as I'm aware, there is a separate AuthorizePlugin[1] flow so that a > compromised renderer still can't instantiate the plugin. GetPlugins only gets > the list of plugins on navigator.plugins() anyways, AFAIK there's a second check > in GetPluginInfo (that isn't being modified) when the plugin is actually > instantiated. > > I'll let Bernard comment authoritatively. > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/plugins/chrome_plugin_ser... That's correct. PluginServiceFilter::CanLoadPlugin() checks whether the renderer process is allowed to connect to a given plugin. If a compromised renderer lies here, it will just get back a different list of plugins, but can't automatically load them. (That being said, plugin authorization is based on content settings, which are based on the URL that's trying to load a plugin… which is coming from the renderer. That has always been the case though, and with PlzNavigate we should probably eventually be able to verify any renderer's origin.)
On 2016/08/20 00:44:19, Bernhard Bauer wrote: > On 2016/08/20 00:26:05, tommycli wrote: > > On 2016/08/20 00:21:31, piman wrote: > > > On 2016/08/20 00:00:41, trizzofo wrote: > > > > > > > > > > https://codereview.chromium.org/2156803002/diff/480001/content/browser/frame_... > > > > File content/browser/frame_host/render_frame_message_filter.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2156803002/diff/480001/content/browser/frame_... > > > > content/browser/frame_host/render_frame_message_filter.cc:455: const > > > > url::Origin& main_frame_origin, > > > > On 2016/08/19 21:59:22, piman wrote: > > > > > This origin comes from an untrusted source. Do we need to validate that > > this > > > > is > > > > > a valid one for the calling renderer? > > > > > > > > Since OnGetPluginInfo() (that effectively determine if Flash should run or > > > not) > > > > doesn't validate the URLs, I believe that it should be fine if we don't > > > validate > > > > it here as well. The only thing we are determining here is if Flash is > > > > advertised or not and I think that it isn't that critical. Let me know if > > you > > > > have any additional thoughts. > > > > > > I think we ought to if this is a security feature. Otherwise a compromised > > > renderer can instantiate a plugin it was not authorized to. > > > I'll let dcheng comment on the security aspect (in particular relative to > OOP > > > iframes), LGTM for the rest. > > > > +bauerb: > > > > As far as I'm aware, there is a separate AuthorizePlugin[1] flow so that a > > compromised renderer still can't instantiate the plugin. GetPlugins only gets > > the list of plugins on navigator.plugins() anyways, AFAIK there's a second > check > > in GetPluginInfo (that isn't being modified) when the plugin is actually > > instantiated. > > > > I'll let Bernard comment authoritatively. > > > > [1] > > > https://cs.chromium.org/chromium/src/chrome/browser/plugins/chrome_plugin_ser... > > That's correct. PluginServiceFilter::CanLoadPlugin() checks whether the renderer > process is allowed to connect to a given plugin. If a compromised renderer lies > here, it will just get back a different list of plugins, but can't automatically > load them. > > (That being said, plugin authorization is based on content settings, which are > based on the URL that's trying to load a plugin… which is coming from the > renderer. That has always been the case though, and with PlzNavigate we should > probably eventually be able to verify any renderer's origin.) Ok, LGTM then, thanks for the explanation.
dcheng, ptal. esprehn, ptal at third_party/WebKit/Source/platform/ files. Thanks!
https://codereview.chromium.org/2156803002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:231: pluginData = init.frame()->page()->pluginData(mainFrameSecurityContext ? mainFrameSecurityContext->getSecurityOrigin() : SecurityOrigin::create(init.url()).get()); Hm, so when we create the document for the main frame, mainFrameSecurityContext will always be null (since the document will have just been cleared in installNewDocument). So this always ends up going through the SecurityOrigin::create(init.url()) path. This juat happens to (mostly) match the behavior of Document::initSecurityContext, but there's no actual guarantee, so it feels fragile. - A top-level frame can never be sandboxed (I think?) so we never need to create a unique origin. - A top-level frame can have an owner (opener), so the origin of the initial empty document would actually be the origin of its opener. But perhaps it's OK to just treat this as unique? - Otherwise, we follow the same path as setting the security origin for a main frame... I wonder if it would it be better to be explicit about the main frame thing here? For subframes, the main frame security context must always be already initialized. And for the main frame, we should just use an origin derived from the document URL (even if the URL is something like about:blank).
https://codereview.chromium.org/2156803002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:231: pluginData = init.frame()->page()->pluginData(mainFrameSecurityContext ? mainFrameSecurityContext->getSecurityOrigin() : SecurityOrigin::create(init.url()).get()); On 2016/08/23 05:51:55, dcheng wrote: > Hm, so when we create the document for the main frame, mainFrameSecurityContext > will always be null (since the document will have just been cleared in > installNewDocument). So this always ends up going through the > SecurityOrigin::create(init.url()) path. This juat happens to (mostly) match the > behavior of Document::initSecurityContext, but there's no actual guarantee, so > it feels fragile. > > - A top-level frame can never be sandboxed (I think?) so we never need to create > a unique origin. > - A top-level frame can have an owner (opener), so the origin of the initial > empty document would actually be the origin of its opener. But perhaps it's OK > to just treat this as unique? > - Otherwise, we follow the same path as setting the security origin for a main > frame... > > I wonder if it would it be better to be explicit about the main frame thing > here? > > For subframes, the main frame security context must always be already > initialized. And for the main frame, we should just use an origin derived from > the document URL (even if the URL is something like about:blank). For the purposes of retrieving the plugin data, we always want the origin of the new main frame. So if the document is at foo.com/bar.swf, we always want foo.com to be the origin. It's probably misleading to even call it a security origin, it's more like: the domain used for Site Settings. Actually part of your comment confused me. Why does Document set its own security origin to that of the opener? I think I don't understand security origins that well.
On 2016/08/23 17:31:03, tommycli wrote: > https://codereview.chromium.org/2156803002/diff/480001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): > > https://codereview.chromium.org/2156803002/diff/480001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/DOMImplementation.cpp:231: pluginData = > init.frame()->page()->pluginData(mainFrameSecurityContext ? > mainFrameSecurityContext->getSecurityOrigin() : > SecurityOrigin::create(init.url()).get()); > On 2016/08/23 05:51:55, dcheng wrote: > > Hm, so when we create the document for the main frame, > mainFrameSecurityContext > > will always be null (since the document will have just been cleared in > > installNewDocument). So this always ends up going through the > > SecurityOrigin::create(init.url()) path. This juat happens to (mostly) match > the > > behavior of Document::initSecurityContext, but there's no actual guarantee, so > > it feels fragile. > > > > - A top-level frame can never be sandboxed (I think?) so we never need to > create > > a unique origin. > > - A top-level frame can have an owner (opener), so the origin of the initial > > empty document would actually be the origin of its opener. But perhaps it's OK > > to just treat this as unique? > > - Otherwise, we follow the same path as setting the security origin for a main > > frame... > > > > I wonder if it would it be better to be explicit about the main frame thing > > here? > > > > For subframes, the main frame security context must always be already > > initialized. And for the main frame, we should just use an origin derived from > > the document URL (even if the URL is something like about:blank). > > For the purposes of retrieving the plugin data, we always want the origin of the > new main frame. So if the document is at foo.com/bar.swf, we always want http://foo.com > to be the origin. > > It's probably misleading to even call it a security origin, it's more like: the > domain used for Site Settings. > > Actually part of your comment confused me. Why does Document set its own > security origin to that of the opener? I think I don't understand security > origins that well. esprehn: ping third_party/WebKit/Source/platform/ thanks!
The CQ bit was checked by trizzofo@google.com 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...
https://codereview.chromium.org/2156803002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:231: pluginData = init.frame()->page()->pluginData(mainFrameSecurityContext ? mainFrameSecurityContext->getSecurityOrigin() : SecurityOrigin::create(init.url()).get()); On 2016/08/23 05:51:55, dcheng wrote: > Hm, so when we create the document for the main frame, mainFrameSecurityContext > will always be null (since the document will have just been cleared in > installNewDocument). So this always ends up going through the > SecurityOrigin::create(init.url()) path. This juat happens to (mostly) match the > behavior of Document::initSecurityContext, but there's no actual guarantee, so > it feels fragile. > > - A top-level frame can never be sandboxed (I think?) so we never need to create > a unique origin. > - A top-level frame can have an owner (opener), so the origin of the initial > empty document would actually be the origin of its opener. But perhaps it's OK > to just treat this as unique? > - Otherwise, we follow the same path as setting the security origin for a main > frame... > > I wonder if it would it be better to be explicit about the main frame thing > here? > > For subframes, the main frame security context must always be already > initialized. And for the main frame, we should just use an origin derived from > the document URL (even if the URL is something like about:blank). I changed it to use Frame's IsMainFrame() method. Hopefully it's more explicit. ptal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/08/23 17:31:03, tommycli wrote: > https://codereview.chromium.org/2156803002/diff/480001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): > > https://codereview.chromium.org/2156803002/diff/480001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/DOMImplementation.cpp:231: pluginData = > init.frame()->page()->pluginData(mainFrameSecurityContext ? > mainFrameSecurityContext->getSecurityOrigin() : > SecurityOrigin::create(init.url()).get()); > On 2016/08/23 05:51:55, dcheng wrote: > > Hm, so when we create the document for the main frame, > mainFrameSecurityContext > > will always be null (since the document will have just been cleared in > > installNewDocument). So this always ends up going through the > > SecurityOrigin::create(init.url()) path. This juat happens to (mostly) match > the > > behavior of Document::initSecurityContext, but there's no actual guarantee, so > > it feels fragile. > > > > - A top-level frame can never be sandboxed (I think?) so we never need to > create > > a unique origin. > > - A top-level frame can have an owner (opener), so the origin of the initial > > empty document would actually be the origin of its opener. But perhaps it's OK > > to just treat this as unique? > > - Otherwise, we follow the same path as setting the security origin for a main > > frame... > > > > I wonder if it would it be better to be explicit about the main frame thing > > here? > > > > For subframes, the main frame security context must always be already > > initialized. And for the main frame, we should just use an origin derived from > > the document URL (even if the URL is something like about:blank). > > For the purposes of retrieving the plugin data, we always want the origin of the > new main frame. So if the document is at foo.com/bar.swf, we always want http://foo.com > to be the origin. > > It's probably misleading to even call it a security origin, it's more like: the > domain used for Site Settings. > > Actually part of your comment confused me. Why does Document set its own > security origin to that of the opener? I think I don't understand security > origins that well. It's complicated: basically if you create an <iframe>, it starts on the initial empty document. In that case, the initial empty document inherits the security origin from the parent frame. When you create a new window with window.open(), a similar thing happens: it starts on the initial empty document. However, instead of inheriting from the parent frame (since there is none), it inherits it from the *opener* frame.
LGTM with nits https://codereview.chromium.org/2156803002/diff/520001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2156803002/diff/520001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:331: virtual void getPluginList(bool refresh, const WebSecurityOrigin& mainFrameOrigin, WebPluginListBuilder*) {} Nit: Let's document the |mainFrameOrigin| parameter.
https://codereview.chromium.org/2156803002/diff/520001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2156803002/diff/520001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:331: virtual void getPluginList(bool refresh, const WebSecurityOrigin& mainFrameOrigin, WebPluginListBuilder*) {} On 2016/08/24 03:33:13, dcheng wrote: > Nit: Let's document the |mainFrameOrigin| parameter. Done.
On 2016/08/24 23:34:13, trizzofo wrote: > https://codereview.chromium.org/2156803002/diff/520001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/Platform.h (right): > > https://codereview.chromium.org/2156803002/diff/520001/third_party/WebKit/pub... > third_party/WebKit/public/platform/Platform.h:331: virtual void > getPluginList(bool refresh, const WebSecurityOrigin& mainFrameOrigin, > WebPluginListBuilder*) {} > On 2016/08/24 03:33:13, dcheng wrote: > > Nit: Let's document the |mainFrameOrigin| parameter. > > Done. esprehn: ping, thanks!
Description was changed from ========== Removes PluginCache and reload PluginData's plugin list whenever the origin changes. Use origin url to retrieve new plugin list. This is needed because, after HTML5 by Default is implemented, the plugin list will depend on the current web page url (settings url exceptions). Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Removes PluginCache and reload PluginData's plugin list whenever the origin changes. Use origin url to retrieve new plugin list. This is needed because, after HTML5 by Default is implemented, the plugin list will depend on the current web page url (settings url exceptions). Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
I don't see how this can work, you're reading a static variable and looking at the main frame origin, but there's lots of main frames in any given process https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right): https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:37: Platform::current()->getPluginList(m_refresh, WebSecurityOrigin(m_mainFrameOrigin), &builder); I don't understand how this code can work, you're using a static variable m_refresh here, but looking at the main frame origin, but the static is shared per process. This can't work https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:38: m_refresh = false; What about all of the other frames? https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.h (right): https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.h:66: static bool m_refresh; s_ for static
https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right): https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:37: Platform::current()->getPluginList(m_refresh, WebSecurityOrigin(m_mainFrameOrigin), &builder); On 2016/08/26 04:46:03, esprehn wrote: > I don't understand how this code can work, you're using a static variable > m_refresh here, but looking at the main frame origin, but the static is shared > per process. This can't work It's confusing, but PluginData::refresh() is actually used to refresh the plugin list in the browser side. It ends up calling PluginServiceImpl::RefreshPlugin() here: https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... Looking at the current code, as far as I can understand, the desired effect is that, after PluginData::refresh() is called, the plugin list on the browser side is updated and all new instances of PluginData in the process that called PluginData::refresh() have the updated list of plugins. Since PluginCache is per-process, I believe that a static boolean will have the same effect. The only behavioral difference I noticed in my code is that the browser process plugin list is updated only when a new PluginData is instantiated, but the current code sends an IPC to the browser process and update it right away. https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:38: m_refresh = false; On 2016/08/26 04:46:03, esprehn wrote: > What about all of the other frames? New instances of PluginData will have the updated list of plugins. https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.h (right): https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.h:66: static bool m_refresh; On 2016/08/26 04:46:03, esprehn wrote: > s_ for static Done.
https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right): https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:37: Platform::current()->getPluginList(m_refresh, WebSecurityOrigin(m_mainFrameOrigin), &builder); On 2016/08/26 20:28:00, trizzofo wrote: > On 2016/08/26 04:46:03, esprehn wrote: > > I don't understand how this code can work, you're using a static variable > > m_refresh here, but looking at the main frame origin, but the static is shared > > per process. This can't work > > It's confusing, but PluginData::refresh() is actually used to refresh the plugin > list in the browser side. It ends up calling PluginServiceImpl::RefreshPlugin() > here: > > https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... > > Looking at the current code, as far as I can understand, the desired effect is > that, after PluginData::refresh() is called, the plugin list on the browser side > is updated and all new instances of PluginData in the process that called > PluginData::refresh() have the updated list of plugins. > > Since PluginCache is per-process, I believe that a static boolean will have the > same effect. The only behavioral difference I noticed in my code is that the > browser process plugin list is updated only when a new PluginData is > instantiated, but the current code sends an IPC to the browser process and > update it right away. Yes - after refresh() is called, all existing PluginData instances must re-read the list of plugins, so we indeed want a static variable. refresh() could probably be renamed rereadPluginListFromDisk() and s_refresh could be renamed s_needsRereadFromDisk. Would that be clearer, esprehn?
https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right): https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/plugins/PluginData.cpp:37: Platform::current()->getPluginList(m_refresh, WebSecurityOrigin(m_mainFrameOrigin), &builder); On 2016/08/26 20:34:16, tommycli wrote: > On 2016/08/26 20:28:00, trizzofo wrote: > > On 2016/08/26 04:46:03, esprehn wrote: > > > I don't understand how this code can work, you're using a static variable > > > m_refresh here, but looking at the main frame origin, but the static is > shared > > > per process. This can't work > > > > It's confusing, but PluginData::refresh() is actually used to refresh the > plugin > > list in the browser side. It ends up calling > PluginServiceImpl::RefreshPlugin() > > here: > > > > > https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... > > > > Looking at the current code, as far as I can understand, the desired effect is > > that, after PluginData::refresh() is called, the plugin list on the browser > side > > is updated and all new instances of PluginData in the process that called > > PluginData::refresh() have the updated list of plugins. > > > > Since PluginCache is per-process, I believe that a static boolean will have > the > > same effect. The only behavioral difference I noticed in my code is that the > > browser process plugin list is updated only when a new PluginData is > > instantiated, but the current code sends an IPC to the browser process and > > update it right away. > > Yes - after refresh() is called, all existing PluginData instances must re-read > the list of plugins, so we indeed want a static variable. > > refresh() could probably be renamed rereadPluginListFromDisk() and s_refresh > could be renamed s_needsRereadFromDisk. Would that be clearer, esprehn? Actually, when PluginData::refresh() is called, all existing PluginData instances remain untouched. The only effect is that new instances will have the updated list provided by the browser process.
Patchset #23 (id:600001) has been deleted
On 2016/08/26 at 22:19:41, trizzofo wrote: > https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... > ... > > > > > > Since PluginCache is per-process, I believe that a static boolean will have > > the > > > same effect. The only behavioral difference I noticed in my code is that the > > > browser process plugin list is updated only when a new PluginData is > > > instantiated, but the current code sends an IPC to the browser process and > > > update it right away. > > > > Yes - after refresh() is called, all existing PluginData instances must re-read > > the list of plugins, so we indeed want a static variable. > > > > refresh() could probably be renamed rereadPluginListFromDisk() and s_refresh > > could be renamed s_needsRereadFromDisk. Would that be clearer, esprehn? > > Actually, when PluginData::refresh() is called, all existing PluginData instances remain untouched. The only effect is that new instances will have the updated list provided by the browser process. That doesn't make sense though, why send m_mainFrameOrigin in the IPC for state that gets stored globally? That means all the various main frames are racing to send the IPC with their origin.
On 2016/08/26 23:31:09, esprehn wrote: > On 2016/08/26 at 22:19:41, trizzofo wrote: > > > https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... > > ... > > > > > > > > Since PluginCache is per-process, I believe that a static boolean will > have > > > the > > > > same effect. The only behavioral difference I noticed in my code is that > the > > > > browser process plugin list is updated only when a new PluginData is > > > > instantiated, but the current code sends an IPC to the browser process and > > > > update it right away. > > > > > > Yes - after refresh() is called, all existing PluginData instances must > re-read > > > the list of plugins, so we indeed want a static variable. > > > > > > refresh() could probably be renamed rereadPluginListFromDisk() and s_refresh > > > could be renamed s_needsRereadFromDisk. Would that be clearer, esprehn? > > > > Actually, when PluginData::refresh() is called, all existing PluginData > instances remain untouched. The only effect is that new instances will have the > updated list provided by the browser process. > > That doesn't make sense though, why send m_mainFrameOrigin in the IPC for state > that gets stored globally? That means all the various main frames are racing to > send the IPC with their origin. Yeah, the race is totally fine. The origin that's sent with that IPC doesn't actually matter for the purposes of the browser-side plugin list cache. On the browser side, the logic looks something like (in pseudocode): if (!cache || ipc.invalidateCache) readPluginListFromCache(); pluginListForOrigin = filter(cache, plugin => isAvailable(plugin, origin)); Send(pluginListForOrigin) So technically we should break it apart into two separate IPCs: an invalidate cache IPC, and a getPluginList IPC. If you would like that I would request we do that in a separate CL since Tomas' internship is ending soon.
On 2016/08/26 23:48:00, tommycli wrote: > On 2016/08/26 23:31:09, esprehn wrote: > > On 2016/08/26 at 22:19:41, trizzofo wrote: > > > > > > https://codereview.chromium.org/2156803002/diff/560001/third_party/WebKit/Sou... > > > ... > > > > > > > > > > Since PluginCache is per-process, I believe that a static boolean will > > have > > > > the > > > > > same effect. The only behavioral difference I noticed in my code is that > > the > > > > > browser process plugin list is updated only when a new PluginData is > > > > > instantiated, but the current code sends an IPC to the browser process > and > > > > > update it right away. > > > > > > > > Yes - after refresh() is called, all existing PluginData instances must > > re-read > > > > the list of plugins, so we indeed want a static variable. > > > > > > > > refresh() could probably be renamed rereadPluginListFromDisk() and > s_refresh > > > > could be renamed s_needsRereadFromDisk. Would that be clearer, esprehn? > > > > > > Actually, when PluginData::refresh() is called, all existing PluginData > > instances remain untouched. The only effect is that new instances will have > the > > updated list provided by the browser process. > > > > That doesn't make sense though, why send m_mainFrameOrigin in the IPC for > state > > that gets stored globally? That means all the various main frames are racing > to > > send the IPC with their origin. > > Yeah, the race is totally fine. The origin that's sent with that IPC doesn't > actually matter for the purposes of the browser-side plugin list cache. > > On the browser side, the logic looks something like (in pseudocode): > > if (!cache || ipc.invalidateCache) > readPluginListFromCache(); > > pluginListForOrigin = filter(cache, plugin => isAvailable(plugin, origin)); > Send(pluginListForOrigin) > > So technically we should break it apart into two separate IPCs: an invalidate > cache IPC, and a getPluginList IPC. If you would like that I would request we do > that in a separate CL since Tomas' internship is ending soon. Correction, a more accurate pseudocode: if (!cache || ipc.invalidateCache) cache = readPluginListFromDisk(); pluginListForOrigin = filter(cache, plugin => isAvailable(plugin, origin)); Send(pluginListForOrigin)
Can we just send an empty security origin then? Sending a racy origin is super scary to me, I don't want anyone to depend on that.
On 2016/08/27 00:01:00, esprehn wrote: > Can we just send an empty security origin then? Sending a racy origin is super > scary to me, I don't want anyone to depend on that. Yes we can definitely do that. When refresh() is called, we could just send an IPC with the refresh flag on, an empty origin, and get rid of the s_refresh static variable.
On 2016/08/27 00:02:41, tommycli wrote: > On 2016/08/27 00:01:00, esprehn wrote: > > Can we just send an empty security origin then? Sending a racy origin is super > > scary to me, I don't want anyone to depend on that. > > Yes we can definitely do that. When refresh() is called, we could just send an > IPC with the refresh flag on, an empty origin, and get rid of the s_refresh > static variable. And later we can refactor it into a separate IPC, since that seems to be the natural direction it's going.
On 2016/08/27 00:03:18, tommycli wrote: > On 2016/08/27 00:02:41, tommycli wrote: > > On 2016/08/27 00:01:00, esprehn wrote: > > > Can we just send an empty security origin then? Sending a racy origin is > super > > > scary to me, I don't want anyone to depend on that. > > > > Yes we can definitely do that. When refresh() is called, we could just send an > > IPC with the refresh flag on, an empty origin, and get rid of the s_refresh > > static variable. > > And later we can refactor it into a separate IPC, since that seems to be the > natural direction it's going. Done.
On 2016/08/28 00:07:29, trizzofo wrote: > On 2016/08/27 00:03:18, tommycli wrote: > > On 2016/08/27 00:02:41, tommycli wrote: > > > On 2016/08/27 00:01:00, esprehn wrote: > > > > Can we just send an empty security origin then? Sending a racy origin is > > super > > > > scary to me, I don't want anyone to depend on that. > > > > > > Yes we can definitely do that. When refresh() is called, we could just send > an > > > IPC with the refresh flag on, an empty origin, and get rid of the s_refresh > > > static variable. > > > > And later we can refactor it into a separate IPC, since that seems to be the > > natural direction it's going. > > Done. esprehn, ptal. Thanks!
https://codereview.chromium.org/2156803002/diff/640001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/640001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:230: pluginData = init.frame()->page()->pluginData(init.frame()->isMainFrame() ? SecurityOrigin::create(init.url()).get() : init.frame()->tree().top()->securityContext()->getSecurityOrigin()); why not always call tree().top() ? Calling .get() on the PassRefPtr here is bad, the object is left floating. You want to save a local variable if you want to do that.
This is really close, just need to do something about that .get() on the PassRefPtr :)
https://codereview.chromium.org/2156803002/diff/640001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/2156803002/diff/640001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMImplementation.cpp:230: pluginData = init.frame()->page()->pluginData(init.frame()->isMainFrame() ? SecurityOrigin::create(init.url()).get() : init.frame()->tree().top()->securityContext()->getSecurityOrigin()); On 2016/08/29 20:54:56, esprehn wrote: > why not always call tree().top() ? When the document is being created for the main frame, init.frame()->tree().top->securityContext() returns nullptr, because the document is cleared in installNewDocument() (createDocument() caller). So we need to get the origin directly from init.url(). > Calling .get() on the PassRefPtr here is bad, the object is left floating. > > You want to save a local variable if you want to do that. Done.
On 2016/08/29 at 21:32:32, trizzofo wrote: > https://codereview.chromium.org/2156803002/diff/640001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/DOMImplementation.cpp (right): > > https://codereview.chromium.org/2156803002/diff/640001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/DOMImplementation.cpp:230: pluginData = init.frame()->page()->pluginData(init.frame()->isMainFrame() ? SecurityOrigin::create(init.url()).get() : init.frame()->tree().top()->securityContext()->getSecurityOrigin()); > On 2016/08/29 20:54:56, esprehn wrote: > > why not always call tree().top() ? > > When the document is being created for the main frame, init.frame()->tree().top->securityContext() returns nullptr, because the document is cleared in installNewDocument() (createDocument() caller). So we need to get the origin directly from init.url(). > That needs a comment explaining what's up then. We should probably fix it. lgtm if you add the comment.
The CQ bit was checked by trizzofo@google.com 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: This issue passed the CQ dry run.
The CQ bit was checked by trizzofo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, alexmos@chromium.org, piman@chromium.org, dcheng@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2156803002/#ps680001 (title: "Add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #26 (id:680001)
Message was sent while issue was closed.
Description was changed from ========== Removes PluginCache and reload PluginData's plugin list whenever the origin changes. Use origin url to retrieve new plugin list. This is needed because, after HTML5 by Default is implemented, the plugin list will depend on the current web page url (settings url exceptions). Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Removes PluginCache and reload PluginData's plugin list whenever the origin changes. Use origin url to retrieve new plugin list. This is needed because, after HTML5 by Default is implemented, the plugin list will depend on the current web page url (settings url exceptions). Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/1c684eba8afc4c730b3ca2e68814e8412020552c Cr-Commit-Position: refs/heads/master@{#415128} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/1c684eba8afc4c730b3ca2e68814e8412020552c Cr-Commit-Position: refs/heads/master@{#415128} |
