[HBD] Remove PluginCache and reload plugin list when origin changes

third_party/WebKit/Source/core/dom/DOMImplementation.cpp

Index: third_party/WebKit/Source/core/dom/DOMImplementation.cpp
diff --git a/third_party/WebKit/Source/core/dom/DOMImplementation.cpp b/third_party/WebKit/Source/core/dom/DOMImplementation.cpp
index 5c7975e2c94c5fb777ecb1d96de93c7e7abc6cf0..92accd6ce8a5a3d7fad819a4870762eba8c30b0c 100644
--- a/third_party/WebKit/Source/core/dom/DOMImplementation.cpp
+++ b/third_party/WebKit/Source/core/dom/DOMImplementation.cpp
@@ -225,9 +225,9 @@ Document* DOMImplementation::createDocument(const String& type, const DocumentIn
     if (type == "application/xhtml+xml")
         return XMLDocument::createXHTML(init);
 
-    PluginData* pluginData = 0;
+    RefPtr<PluginData> pluginData;
     if (init.frame() && init.frame()->page() && init.frame()->loader().allowPlugins(NotAboutToInstantiatePlugin))
-        pluginData = init.frame()->pluginData();
+        pluginData = PluginData::create(init.owner() ? init.owner()->getSecurityOrigin() : SecurityOrigin::create(init.url()));

[alexmos, 2016/08/05 23:49:06]

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?

[trizzofo, 2016/08/06 01:30:46]

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.

[alexmos, 2016/08/09 01:40:15]

Agreed that overloading pluginData() isn't the best - but see below.
Sure, I'm curious if they have any thoughts.  If it's just for top-level,
non-html documents, this is probably ok.
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).
Ah, didn't notice that.  Thanks!

[tommycli, 2016/08/09 15:54:24]

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.

[trizzofo, 2016/08/09 21:24:24]

That's a good idea. I'm going to give it a try.

[alexmos, 2016/08/10 00:10:29]

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.

 
     // PDF is one image type for which a plugin can override built-in support.
     // We do not want QuickTime to take over all image types, obviously.