Closed Bug 778201 Opened 7 years ago Closed 3 years ago
nspluginhost calls synchronous ns
IProtocol Proxy Service::Resolve
Bug 778201 - Remove nsPluginHost::FindProxyByURL and nsProtocolProxyService::DeprecatedBlockingResolve.
59 bytes, text/x-review-board-request
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?
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).
Assignee: nobody → joshmoz
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.
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.
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.
I think the argument for doing this is that a footgun available to addons, which blocks the main thread, could be removed.
(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).
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
100% confident this is not used by Flash. Rip it out in 53+ (we still need this for ESR52).
Attachment #667598 - Attachment is obsolete: true
Comment on attachment 8833117 [details] Bug 778201 - Remove nsPluginHost::FindProxyByURL and nsProtocolProxyService::DeprecatedBlockingResolve. https://reviewboard.mozilla.org/r/109344/#review110520 woot!
Attachment #8833117 - Flags: review?(mcmanus) → review+
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
Attachment #8833117 - Flags: review?(benjamin) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/30185f9bdac5 Remove nsPluginHost::FindProxyByURL and nsProtocolProxyService::DeprecatedBlockingResolve. r=bsmedberg,mcmanus
You need to log in before you can comment on or make changes to this bug.