1.60 KB, patch
Benjamin Smedberg: review+
|Details | Diff | Splinter Review|
1.07 KB, patch
dmajor (offline): review+
|Details | Diff | Splinter Review|
Looking through the crash stats while protected mode was turned off on beta, I discovered a crash signature which was close to 20% of Flash crashes, mozilla::plugins::child::_getproperty These crashes appear to all be caused by the realnetworks video downloader which is hooking into Flash somehow. See e.g. https://crash-stats.mozilla.com/report/index/c18008fa-7720-41b0-b1ed-379f52150202 I think we should straight-up block this DLL, since it's destined to always be a stability problem. Chad, can you confirm my decision here? dmajor, please verify that our DLL blocklist works in the plugin-container process and prepare the necessary patch for blocking these two DLLs.
This could break a real use case that some people may want. Has there been any outreach to RealNetworks for a fix from their side? These crashes are in the latest version (126.96.36.199 from October 2014). Do you want to block all versions or just the latest? Either way has a potential downside. From the technical side, plugin-container.exe does not currently use the blocklist. I tried hooking up a delayload mozglue but it messed up the fragile DLL balance of bug 1023941. I haven't yet figured out why.
relnote-firefox: --- → ?
I don't care about the RealNetworks code working. Hitching a ride on the Flash plugin is not a good way to accomplish that use-case and there are Firefox extensions that solve the same problems without the hooking. Plus youtube is switching away from Flash in the near future anyway.
So, plugin-container.exe contains a nontrivial amount of code and inevitably a bunch of it calls the malloc family. Linking to mozglue would violate the constraint at https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/WindowsCrtPatch.h#31-32. Without a pressing need to install the blocklist super early, I'm not inclined to spend time dealing with the library relationships. I propose that we let xul.dll do the initialize call from XRE_InitChildProcess.
Created attachment 8565269 [details] [diff] [review] Part 1: Use the DLL blocklist in plugin-container processes I tested that our asserts are still happy on XPSP2.
Attachment #8565269 - Flags: review?(benjamin)
Created attachment 8565270 [details] [diff] [review] Part 2 - Block rndlnpshimswf.dll and rndlmainbrowserrecordplugin.dll I checked that these DLLs don't get loaded, and that the Real addon gracefully handles the failed loads.
Created attachment 8565272 [details] [diff] [review] Part 2 - Block rndlnpshimswf.dll and rndlmainbrowserrecordplugin.dll I checked that these DLLs don't get loaded, and that the Real addon gracefully handles the failed loads.
Attachment #8565269 - Flags: review?(benjamin) → review+
Comment on attachment 8565272 [details] [diff] [review] Part 2 - Block rndlnpshimswf.dll and rndlmainbrowserrecordplugin.dll I think we should block all versions.
Attachment #8565272 - Flags: review?(benjamin) → review-
And just to avoid friction, r+ with ALL_VERSIONS.
Created attachment 8565757 [details] [diff] [review] Part 2 - Block rndlnpshimswf.dll and rndlmainbrowserrecordplugin.dll as landed, with ALL_VERSIONS
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
We've never release noted blocking DLLs before - is there something special about these two (very long-named) that makes us want to change that? Should I be noting that the Real addon will be blocked from this version forward (which is also something we don't typically put in release notes).
The RealDownloader will be blocked. Somebody may be deliberately using it, and may be surprised when it stops working. (Or maybe not, I don't know.) If these things are normally not mentioned, then ok.
Ok, good to know. As addon blocking is not something we release note, I'm removing the flag here. NI? to Tyler so that UA has awareness that this may come up in feedback.
relnote-firefox: ? → ---
Flags: needinfo?(benjamin) → needinfo?(tdowner)
You need to log in before you can comment on or make changes to this bug.