Closed Bug 778201 Opened 7 years ago Closed 3 years ago

nspluginhost calls synchronous nsIProtocolProxyService::Resolve

Categories

(Core :: Plug-ins, defect)

16 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: mcmanus, Assigned: jld)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
Depends on: 778181
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
Blocks: 781732
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.
No longer blocks: 769764, 781732
Attached patch fix v0.2 (obsolete) — Splinter Review
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
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).
See Also: → 1325242
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
Flags: needinfo?(benjamin)
Resolution: WONTFIX → ---
100% confident this is not used by Flash. Rip it out in 53+ (we still need this for ESR52).
Flags: needinfo?(benjamin)
Attachment #667598 - Attachment is obsolete: true
Assignee: jaas → jld
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 jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30185f9bdac5
Remove nsPluginHost::FindProxyByURL and nsProtocolProxyService::DeprecatedBlockingResolve. r=bsmedberg,mcmanus
https://hg.mozilla.org/mozilla-central/rev/30185f9bdac5
Status: REOPENED → RESOLVED
Closed: 7 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.