Bug 184369 – WebPage sometimes incorrectly rules out PDF as a mime type that can be showed

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

WebFrame sometimes incorrectly rule out PDF as a mime type that can be showed

[youenn fablet]

Created attachment 337393 [details]
Patch

[youenn fablet]

Created attachment 337394 [details]
WIP

[youenn fablet]

Created attachment 337396 [details]
WIP

[EWS Watchlist]

Comment on attachment 337396 [details]
WIP

Attachment 337396 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7231659

New failing tests:
userscripts/user-script-plugin-document.html
plugins/plugin-javascript-access.html
plugins/access-after-page-destroyed-2.html
plugins/embed-attributes-setting.html
http/tests/security/contentSecurityPolicy/cross-origin-plugin-document-allowed-in-child-window.html
fast/dom/collection-iterators.html
plugins/plugin-document-back-forward.html
http/tests/plugins/nounsupported-plugin.html
http/tests/security/contentSecurityPolicy/same-origin-plugin-document-blocked-in-child-window.html
plugins/iframe-plugin-bgcolor.html
http/tests/security/contentSecurityPolicy/1.1/plugintypes-affects-child.html
http/tests/plugins/supported-plugin-origin-specific-visibility.html
imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/navigator-pluginarray.html
plugins/access-after-page-destroyed.html
http/tests/plugins/plugin-javascript-access.html
fast/dom/Window/Plug-ins.html
plugins/no-mime-with-valid-extension.html

[EWS Watchlist]

Created attachment 337405 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 337415 [details]
Patch

[youenn fablet]

Created attachment 337416 [details]
Patch

[EWS Watchlist]

Comment on attachment 337416 [details]
Patch

Attachment 337416 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7234383

New failing tests:
plugins/navigator-plugins-disabled.html
http/tests/plugins/supported-plugin-origin-specific-visibility.html

[EWS Watchlist]

Created attachment 337419 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 337423 [details]
Patch

[EWS Watchlist]

Comment on attachment 337423 [details]
Patch

Attachment 337423 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7237224

New failing tests:
http/tests/plugins/supported-plugin-origin-specific-visibility.html

[EWS Watchlist]

Created attachment 337424 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 337428 [details]
Patch

[youenn fablet]

Created attachment 337432 [details]
Patch

[youenn fablet]

Comment on attachment 337432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337432&action=review

> Source/WebCore/plugins/PluginData.cpp:45
> +    fprintf(stderr, "url %s\n", m_url.string().utf8().data());

Will remove this fprintf in a follow-up or at landing time.

[Darin Adler]

Comment on attachment 337432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337432&action=review

Why no regression test?

> Source/WebCore/page/Page.cpp:554
> +    if (m_pluginData) {
> +        URL pageURL = mainFrame().document() ? mainFrame().document()->url() : URL { };
> +        if (!protocolHostAndPortAreEqual(pageURL, m_pluginData->url()))
> +            m_pluginData = nullptr;
> +    }

1) Ugly that this check is here, but the extraction of the URL is done in the PluginData constructor. Instead there should be a PluginData function to tell if it’s OK to keep using a PluginData. That way the code can be shared and/or kept in sync with the constructor. So the code here would be more like this:

    if (!m_pluginData || !m_pluginData->canBeReused())
        m_pluginData = PluginData::create(*this);

Or we could find a cleaner way for the PluginData to "fix itself". Creating a new object doesn’t seem necessary or relevant to me.

    if (m_pluginData)
        m_pluginData->updateURL();
    else
        m_pluginData = PluginData::create(*this);

2) It’s unclear to me why matching protocol, host, and port are sufficient, rather than matching the entire URL.

3) It is so inelegant to do this every time someone calls the pluginData function. Is there no better design?

> Source/WebKit/WebProcess/Plugins/WebPluginInfoProvider.h:49
> -    void getPluginInfo(WebCore::Page&, Vector<WebCore::PluginInfo>&, std::optional<Vector<WebCore::SupportedPluginName>>&) final;
> -    void getWebVisiblePluginInfo(WebCore::Page&, Vector<WebCore::PluginInfo>&) final;
> +    Vector<WebCore::PluginInfo> getPluginInfo(WebCore::Page&, std::optional<Vector<WebCore::SupportedPluginName>>&) final;
> +    Vector<WebCore::PluginInfo> getWebVisiblePluginInfo(WebCore::Page&, const WebCore::URL&) final;

WebKit coding style says that the reason these function names have the word "get" in them is that they use out arguments. When changing them to return vectors, the word "get" should be removed from their names.

[youenn fablet]

Thanks for the feedback. I think I'll split the refactoring and the modification change in two patches.

We now have to pass the URL since the page URL is used to match plug-in rules defined by WebKit applications.
The rules are matching URLS based on host and protocol information.

(In reply to Darin Adler from comment #16)
> Comment on attachment 337432 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337432&action=review
> 
> Why no regression test?
> 
> > Source/WebCore/page/Page.cpp:554
> > +    if (m_pluginData) {
> > +        URL pageURL = mainFrame().document() ? mainFrame().document()->url() : URL { };
> > +        if (!protocolHostAndPortAreEqual(pageURL, m_pluginData->url()))
> > +            m_pluginData = nullptr;
> > +    }
> 
> 1) Ugly that this check is here, but the extraction of the URL is done in
> the PluginData constructor. Instead there should be a PluginData function to
> tell if it’s OK to keep using a PluginData. That way the code can be shared
> and/or kept in sync with the constructor. So the code here would be more
> like this:
> 
>     if (!m_pluginData || !m_pluginData->canBeReused())
>         m_pluginData = PluginData::create(*this);
> 
> Or we could find a cleaner way for the PluginData to "fix itself". Creating
> a new object doesn’t seem necessary or relevant to me.
> 
>     if (m_pluginData)
>         m_pluginData->updateURL();
>     else
>         m_pluginData = PluginData::create(*this);
> 
> 2) It’s unclear to me why matching protocol, host, and port are sufficient,
> rather than matching the entire URL.
> 
> 3) It is so inelegant to do this every time someone calls the pluginData
> function. Is there no better design?
> 
> > Source/WebKit/WebProcess/Plugins/WebPluginInfoProvider.h:49
> > -    void getPluginInfo(WebCore::Page&, Vector<WebCore::PluginInfo>&, std::optional<Vector<WebCore::SupportedPluginName>>&) final;
> > -    void getWebVisiblePluginInfo(WebCore::Page&, Vector<WebCore::PluginInfo>&) final;
> > +    Vector<WebCore::PluginInfo> getPluginInfo(WebCore::Page&, std::optional<Vector<WebCore::SupportedPluginName>>&) final;
> > +    Vector<WebCore::PluginInfo> getWebVisiblePluginInfo(WebCore::Page&, const WebCore::URL&) final;
> 
> WebKit coding style says that the reason these function names have the word
> "get" in them is that they use out arguments. When changing them to return
> vectors, the word "get" should be removed from their names.

[youenn fablet]

Created attachment 337520 [details]
Patch

[youenn fablet]

Created attachment 338436 [details]
Patch

[Chris Dumez]

Comment on attachment 338436 [details]
Patch

Patch does not apply on Tip of Tree? I cannot find where m_cachedVisiblePlugins is defined.

[Chris Dumez]

Comment on attachment 338436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338436&action=review

> Source/WebCore/plugins/PluginData.cpp:125
> +    m_cachedVisiblePlugins.pageURL = url;

I would suggest:
m_cachedVisiblePlugins = { url, m_page.pluginInfoProvider().webVisiblePluginInfo(m_page, url) };

Also, why do we ignore potentially cached data if the domain matches?

[youenn fablet]

Created attachment 338454 [details]
Patch for landing

[youenn fablet]

Created attachment 338455 [details]
Patch for landing

[youenn fablet]

<rdar://problem/38534312>

[youenn fablet]

(In reply to Chris Dumez from comment #21)
> Comment on attachment 338436 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338436&action=review
> 
> > Source/WebCore/plugins/PluginData.cpp:125
> > +    m_cachedVisiblePlugins.pageURL = url;
> 
> I would suggest:
> m_cachedVisiblePlugins = { url,
> m_page.pluginInfoProvider().webVisiblePluginInfo(m_page, url) };
> 
> Also, why do we ignore potentially cached data if the domain matches?

Thanks for the review, updated both points.

[WebKit Commit Bot]

Comment on attachment 338455 [details]
Patch for landing

Clearing flags on attachment: 338455

Committed r230853: <https://trac.webkit.org/changeset/230853>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.