778201 - nspluginhost calls synchronous nsIProtocolProxyService::Resolve

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Patrick McManus [:mcmanus]]

In order to resolve 769764 (blocking dns in PAC evaluation) and its goal of removing the synchronous dns api all together (766973), I need to remove the synchronous interface of nsIProtocolProxyService::Resolve()

Most of the users of that are in netwerk (http, websockets, and ftp) and I can fix those as part of 769764, but the remaining use is in the plugin code.

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#764

josh - would you be interested in fixing this to use the AsyncResolve() method already available on nsIProtocolProxyService?

Comment 1

[Josh Aas]

I'll figure this out myself or find someone to do it. This might be tricky because this is part of a synchronous NPAPI call (NPN_GetValue->NPNURLVProxy).

Comment 2

[Josh Aas]

Status here is that we believe Java is the only popular plugin making use of this API. We will need to propose an async alternative and implement it to fix this bug.

Comment 3

[Patrick McManus [:mcmanus]]

in the interim, I can implement a nsIProtocolProxyService2::DeprecatedBlockingResolve() so that this bug does not create dependencies for removing nsIProtocolProxyService::resolve() or nsIDNSService::Resolve..

during that time java will of course run the risk of continuing to block the main thread.

Comment 4

[Josh Aas]

Attachment: fix v0.2

I started an implementation but I don't plan to finish it. Backing up my work here.

Since I started coding this I've been thinking about whether or not this is really worth it.

Patrick implemented async off-main-thread proxy info lookups recently, and he left the blocking plugin lookup working by doing an async lookup while blocking the main thread. This matches our previous behavior. Since the actual work is not occurring on the blocked main thread, we were able to go ahead and disable things like blocking DNS queries on the main thread.

The Java plugin is the only plugin we know of that might make the sync proxy info lookup. This is a limited edge case, and I no longer believe it is worth the time and energy to ship a new API in Gecko and wait for them to update. Nobody else was interested on the plugin-futures mailing list either. I think we should just continue our general efforts to reduce dependencies on plugins like Java, and reduce unnecessary plugin instantiations via features like click-to-play.

I am going to WONTFIX this bug.

Comment 5

[Patrick McManus [:mcmanus]]

I think the argument for doing this is that a footgun available to addons, which blocks the main thread, could be removed.

Comment 6

[Josh Aas]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> I think the argument for doing this is that a footgun available to addons,
> which blocks the main thread, could be removed.

What do you mean available to addons? Without doing this the footgun will remain available to NPAPI plugins, but we can avoid allowing addons to use the footgun quite easily (noscript in the idl, or just take it out of an idl).

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

In light of the current state of / plans for NPAPI (bug 1269807): How sure are we that this isn't used by Flash, and (if so) can we just remove or stub out FindProxyForURL?

In addition to being the only remaining caller of nsProtocolProxyService::DeprecatedBlockingResolve, this is also one of two places where the proxy service is instantiated in a content process, which is causing some problems with sandboxing.  This method could be remoted, but if it's known to be effectively dead code now then it makes more sense to just remove it.

Comment 8

[Benjamin Smedberg]

100% confident this is not used by Flash. Rip it out in 53+ (we still need this for ESR52).

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 778201 - Remove nsPluginHost::FindProxyByURL and nsProtocolProxyService::DeprecatedBlockingResolve.

Review commit: https://reviewboard.mozilla.org/r/109344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109344/

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8833117 [details]
Bug 778201 - Remove nsPluginHost::FindProxyByURL and nsProtocolProxyService::DeprecatedBlockingResolve.

https://reviewboard.mozilla.org/r/109344/#review110520

woot!

Comment 11

[Benjamin Smedberg]

Comment on attachment 8833117 [details]
Bug 778201 - Remove nsPluginHost::FindProxyByURL and nsProtocolProxyService::DeprecatedBlockingResolve.

https://reviewboard.mozilla.org/r/109344/#review110596

::: dom/plugins/base/nsPluginHost.cpp:597
(Diff revision 1)
> - * This method conforms to the return code specified in
> - * http://developer.netscape.com/docs/manuals/proxy/adminnt/autoconf.htm#1020923
> - * with the exception that multiple values are not implemented.
>   */
>  
>  nsresult nsPluginHost::FindProxyForURL(const char* url, char* *result)

It would be better to go further, remove this function entirely and have the sole caller return an eager error: http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/dom/plugins/base/nsNPAPIPlugin.cpp#2465

r=me with that change

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8833117 [details]
Bug 778201 - Remove nsPluginHost::FindProxyByURL and nsProtocolProxyService::DeprecatedBlockingResolve.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109344/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30185f9bdac5
Remove nsPluginHost::FindProxyByURL and nsProtocolProxyService::DeprecatedBlockingResolve. r=bsmedberg,mcmanus

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/30185f9bdac5