Fix navigator.plugins and navigator.mimeTypes for OOPIFs.

Fix navigator.plugins and navigator.mimeTypes for OOPIFs.

Previously, Page::pluginData() returned nullptr when the Page's main
frame was a RemoteFrame.  This caused navigator.plugins and
navigator.mimeTypes to both return empty arrays from an OOPIF.

Page::pluginData() needed the main frame to run the
FrameLoader::allowPlugins check on it.  This CL moves this check to
happen on the actual LocalFrame that needs to access
Page::pluginData().

BUG=612200, 607981
Committed: https://crrev.com/fb92f3e718335521cf454e53651235fbf30c10e2
Cr-Commit-Position: refs/heads/master@{#394942}

[alexmos, 2016-05-19 18:21:55]

alexmos@chromium.org changed reviewers:
+ japhet@chromium.org

[alexmos, 2016-05-19 18:21:56]

Nate, can you please take a look?

https://codereview.chromium.org/1990343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/plugins/navigator-plugins-in-cross-origin-frame.html
(right):

https://codereview.chromium.org/1990343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/plugins/navigator-plugins-in-cross-origin-frame.html:16:
log("FAIL: mismatch in plugin data.  The frame's data is " + event.data + " but
the parent's data is " + pluginDataAsString());
I didn't know this, but apparently layout tests have fake plugin data defined,
which makes it handy here.  Without this CL, this test fails with
--site-per-process like this:

-PASS
+FAIL: mismatch in plugin data. The frame's data is
{"pluginNames":[],"mimeTypes":[]} but the parent's data is
{"pluginNames":["Blink Deprecated Test Plugin","Blink Test
Plugin"],"mimeTypes":["application/x-blink-deprecated-test-plugin","application/x-blink-test-plugin"]}

https://codereview.chromium.org/1990343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.cpp (left):

https://codereview.chromium.org/1990343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.cpp:262: ||
!deprecatedLocalMainFrame()->loader().allowPlugins(NotAboutToInstantiatePlugin))
AFAICT, the places which called this didn't rely on allowPlugins being called on
the main frame: several places also call allowPlugins directly on the frame, and
the client calls like ContentSettingsObserver::allowPlugins don't care about the
frame.  (ContentSettingsObserver::didNotAllowPlugins actually sends a message
via the frame to the browser, but the handler is
TabSpecificContentSettings::OnContentBlockedWithDetail and doesn't use the
frame)

[Nate Chapin, 2016-05-19 19:44:41]

LGTM with a cleanup question/suggestion

https://codereview.chromium.org/1990343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.cpp (left):

https://codereview.chromium.org/1990343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.cpp:266: return m_pluginData.get();
Semi-tangential: What are the rules on what goes on FrameHost vs. Page? Should
we move m_pluginData to FrameHost while we're here? Also, it's not entirely
clear to me, but it's possible that we could make PluginData per-process rather
than per-page?

[dcheng, 2016-05-19 20:04:37]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-05-19 20:04:38]

https://codereview.chromium.org/1990343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.cpp (left):

https://codereview.chromium.org/1990343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.cpp:266: return m_pluginData.get();
On 2016/05/19 at 19:44:40, Nate Chapin wrote:
> Semi-tangential: What are the rules on what goes on FrameHost vs. Page? Should
we move m_pluginData to FrameHost while we're here? Also, it's not entirely
clear to me, but it's possible that we could make PluginData per-process rather
than per-page?

At one point, I believe the plan for FrameHost was to move things shared by
Frames but agnostic to Frame relationships (e.g. Settings) to FrameHost while
things that are shared by Frames but care about Frame relationships (e.g.
FocusController) would move to Page. I don't think there's anyone actively
working on this, and the split is kind of confusing, so maybe we should just
merge the two for now.

[alexmos, 2016-05-19 21:37:15]

Description was changed from

==========
Fix navigator.plugins and navigator.mimeTypes for OOPIFs.

Previously, Page::pluginData() returned nullptr when the Page's main
frame was a RemoteFrame.  This caused navigator.plugins and
navigator.mimeTypes to both return empty arrays from an OOPIF.

Page::pluginData() needed the main frame to run the
FrameLoader::allowPlugins check on it.  This CL moves this check to
happen on the actual LocalFrame that needs to access
Page::pluginData().

BUG=612200
==========

to

==========
Fix navigator.plugins and navigator.mimeTypes for OOPIFs.

Previously, Page::pluginData() returned nullptr when the Page's main
frame was a RemoteFrame.  This caused navigator.plugins and
navigator.mimeTypes to both return empty arrays from an OOPIF.

Page::pluginData() needed the main frame to run the
FrameLoader::allowPlugins check on it.  This CL moves this check to
happen on the actual LocalFrame that needs to access
Page::pluginData().

BUG=612200, 607981
==========

[alexmos, 2016-05-19 22:23:27]

Thanks!

Thanks to lukasza@, I realized that this also fixes another layout test with
--site-per-process, so I've tweaked the expectations to re-enable it.

(The test is http/tests/plugins/cross-frame-object-access.html, and it
apparently failed due to FrameLoaderClientImpl::getObjectContentType returning
the wrong thing due to not getting valid pluginData)

https://codereview.chromium.org/1990343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.cpp (left):

https://codereview.chromium.org/1990343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.cpp:266: return m_pluginData.get();
On 2016/05/19 20:04:37, dcheng wrote:
> On 2016/05/19 at 19:44:40, Nate Chapin wrote:
> > Semi-tangential: What are the rules on what goes on FrameHost vs. Page?
Should
> we move m_pluginData to FrameHost while we're here? Also, it's not entirely
> clear to me, but it's possible that we could make PluginData per-process
rather
> than per-page?
> 
> At one point, I believe the plan for FrameHost was to move things shared by
> Frames but agnostic to Frame relationships (e.g. Settings) to FrameHost while
> things that are shared by Frames but care about Frame relationships (e.g.
> FocusController) would move to Page. I don't think there's anyone actively
> working on this, and the split is kind of confusing, so maybe we should just
> merge the two for now.

Yeah, I think the best cleanup for this is actually to make pluginData
per-process rather than per-page.  Ultimately, the data comes from pluginCache()
in PluginData.cpp, where the data is already per-process as a
DEFINE_STATIC_LOCAL: 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
AFAICT, nothing in PluginData.cpp depends on Page, even though it's passed in
(maybe it used to?), so perhaps it's just possible to retrieve the necessary
data from that cache directly, without involving Page at all.  There'd be a few
more things to figure out (like refresh), so if it's ok, I'm going to leave this
for a followup CL.

[alexmos, 2016-05-19 22:26:13]

alexmos@chromium.org changed reviewers:
+ haraken@chromium.org

[alexmos, 2016-05-19 22:26:13]

haraken@: can you please review Source/modules for OWNERS?

[Nate Chapin, 2016-05-19 22:26:20]

On 2016/05/19 22:23:27, alexmos wrote:
> Thanks!
> 
> Thanks to lukasza@, I realized that this also fixes another layout test with
> --site-per-process, so I've tweaked the expectations to re-enable it.
> 
> (The test is http/tests/plugins/cross-frame-object-access.html, and it
> apparently failed due to FrameLoaderClientImpl::getObjectContentType returning
> the wrong thing due to not getting valid pluginData)
> 
>
https://codereview.chromium.org/1990343002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/page/Page.cpp (left):
> 
>
https://codereview.chromium.org/1990343002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/Page.cpp:266: return m_pluginData.get();
> On 2016/05/19 20:04:37, dcheng wrote:
> > On 2016/05/19 at 19:44:40, Nate Chapin wrote:
> > > Semi-tangential: What are the rules on what goes on FrameHost vs. Page?
> Should
> > we move m_pluginData to FrameHost while we're here? Also, it's not entirely
> > clear to me, but it's possible that we could make PluginData per-process
> rather
> > than per-page?
> > 
> > At one point, I believe the plan for FrameHost was to move things shared by
> > Frames but agnostic to Frame relationships (e.g. Settings) to FrameHost
while
> > things that are shared by Frames but care about Frame relationships (e.g.
> > FocusController) would move to Page. I don't think there's anyone actively
> > working on this, and the split is kind of confusing, so maybe we should just
> > merge the two for now.
> 
> Yeah, I think the best cleanup for this is actually to make pluginData
> per-process rather than per-page.  Ultimately, the data comes from
pluginCache()
> in PluginData.cpp, where the data is already per-process as a
> DEFINE_STATIC_LOCAL: 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> AFAICT, nothing in PluginData.cpp depends on Page, even though it's passed in
> (maybe it used to?), so perhaps it's just possible to retrieve the necessary
> data from that cache directly, without involving Page at all.  There'd be a
few
> more things to figure out (like refresh), so if it's ok, I'm going to leave
this
> for a followup CL.

Works for me, thanks.

[haraken, 2016-05-19 23:38:19]

modules/ LGTM

[alexmos, 2016-05-20 00:00:12]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-05-20 00:00:13]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1990343002/#ps60001
(title: "Undo accidental blank line")

[commit-bot: I haz the power, 2016-05-20 00:02:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1990343002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1990343002/60001

[commit-bot: I haz the power, 2016-05-20 01:04:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-20 01:05:20]

Description was changed from

==========
Fix navigator.plugins and navigator.mimeTypes for OOPIFs.

Previously, Page::pluginData() returned nullptr when the Page's main
frame was a RemoteFrame.  This caused navigator.plugins and
navigator.mimeTypes to both return empty arrays from an OOPIF.

Page::pluginData() needed the main frame to run the
FrameLoader::allowPlugins check on it.  This CL moves this check to
happen on the actual LocalFrame that needs to access
Page::pluginData().

BUG=612200, 607981
==========

to

==========
Fix navigator.plugins and navigator.mimeTypes for OOPIFs.

Previously, Page::pluginData() returned nullptr when the Page's main
frame was a RemoteFrame.  This caused navigator.plugins and
navigator.mimeTypes to both return empty arrays from an OOPIF.

Page::pluginData() needed the main frame to run the
FrameLoader::allowPlugins check on it.  This CL moves this check to
happen on the actual LocalFrame that needs to access
Page::pluginData().

BUG=612200, 607981

Committed: https://crrev.com/fb92f3e718335521cf454e53651235fbf30c10e2
Cr-Commit-Position: refs/heads/master@{#394942}
==========

[commit-bot: I haz the power, 2016-05-20 01:05:21]

Patchset 4 (id:??) landed as
https://crrev.com/fb92f3e718335521cf454e53651235fbf30c10e2
Cr-Commit-Position: refs/heads/master@{#394942}