Bug 58282 - Core Animation plugin layers are incorrectly released
Summary: Core Animation plugin layers are incorrectly released
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.6
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on: 61948
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-11 15:55 PDT by Stuart Morgan
Modified: 2022-06-22 22:18 PDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Stuart Morgan 2011-04-11 15:55:09 PDT
WebKit's Core Animation drawing model has the following code (in WebNetscapePluginView.mm):

            // The plug-in gives us a retained layer.
            _pluginLayer.adoptNS((CALayer *)value);

meaning that the plugin will be release'd later. This is a clear violation of the CA spec as accepted: https://wiki.mozilla.org/NPAPI:CoreAnimationDrawingModel

I understand that in the pre-confirmed-spec implementation this code was correct, but Safari's implementation should match the spec. Matching the spec would mean that plugins not written to spec would leak, and those written to spec would be correct. The current behavior means that plugins not written to spec are okay, but those written to spec crash. That's clearly worse.


I found this bug because a plugin developer argued that their leaky code was correct when I pointed it out to them; they were building their plugin so that it wouldn't crash in Safari, rather than to match the spec. That means that Safari's incorrect implementation is almost certainly leading to the creation of plugins that leak when run in Chromium or Firefox. (The best case scenario as it stands now is that plugins do UA detection, which will probably mean niche browsers will get a broken behavior).

Please fix this, and reach out to plugin vendors using CA to get them to match the spec to avoid the resulting leaks.
Comment 1 Simon Fraser (smfr) 2011-04-11 15:59:40 PDT
Or maybe we just change the spec.
Comment 2 Richard Bateman 2011-04-11 16:00:36 PDT
We are currently working around this in FireBreath (http://www.firebreath.org) but it would sure be nice if we could just implement it to spec and it would work.
Comment 3 Richard Bateman 2011-04-11 16:02:54 PDT
(In reply to comment #1)
> Or maybe we just change the spec.

I find it interesting that the email addresses of the people on the spec are @apple.com, and it's a spec that has been "Accepted" and published for quite some time -- enough time that two other major browsers have adopted it and added support.

I really, *really* hope that you were joking. It's bad enough trying to support all the weird differences in implementations between major browsers without those browsers doing it on purpose...
Comment 4 Stuart Morgan 2011-04-11 16:12:56 PDT
(In reply to comment #1)
> Or maybe we just change the spec.

Can you guarantee that nobody implemented a plugin that does UA checks, and does the right thing in browsers other than Safari? I'm not excited about the idea of releasing a version of Chrome where plugins that were built to spec suddenly start crashing, and I'm guessing the same would be true of Firefox.


You and Kevin were both involved in the discussion on plugin-futures about making that change to the spec; Kevin explicitly agreed that we should make the change, and you didn't object when Josh gave the new language that was going in. You even made the change to the sample code on the wiki IIRC. It's not like this is something that slipped through unnoticed by accident.

If you had argued then that your having an implementation already shipped meant the change shouldn't be made, that would be one thing (although it would be nice if pre-spec implementations used some made-up constant to prevent that kind of thing), but now that there are several other shipped implementations that *do* match the spec it's a bit late for that.
Comment 5 Simon Fraser (smfr) 2011-06-01 18:10:04 PDT
This is apparently fixed in WebKit2.
Comment 6 Stuart Morgan 2011-06-13 14:54:14 PDT
I want to capture here what I've learned from discussions I've had with various plugin vendors since filing this, to make it clear that this is *very* hard for plugins to work around correctly:
- If a plugin follow the spec as written, every single WebKit.framework-based app will crash if it loads that plugin
- If a plugin vendor ignores the spec, it will leak CA layer in every compliant browser.
- The seemingly logic approach of special-casing browsers that do the wrong thing by doing a UA-check to do the retain is, as far as I can tell, impossible to get right. It's impossible to enumerate the apps that are wrong because every WebKit.framework-based app is wrong, and they could have basically any UA.

That leaves whitelisting browsers that *aren't* broken, which is horrible. It means anyone who writes a browser that implements the spec has to get every CA plugin to special-case them, or satisfy the checks done by every plugin (which of course a browser developer won't even know because most plugins are closed-source).

And even if we accept that, it's still not clear what check a plugin vendor should actually do. If the plugin looks for "WebKit", it leaks in WebKit2 and in anything Chromium based. So it has to look for WebKit, but not WebKit2, and not Chrome. But any number of apps could be Chromium-based (and at least a couple of other browsers are), and they may not have Chrome in their UA, so then they leak too. And meanwhile any app without WebKit in its UA that uses WebKit.framework will crash.

I considered suggesting to plugin vendors that they look for WebKit.framework being loaded in the app--but that doesn't work because there are plugins that themselves load WebKit.framework, and most apps don't have OOP plugins.


Apple could make this whole mess go away by shipping a fixed version of WebKit.framework in 10.6.x. Then all a plugin vendor would have to do is check the OS version (or maybe the WebKit version) and not retain for 10.6.x+. The only downside to this is that until plugin vendors updated, things would leak in WebKit.framework apps. That seems a whole lot better than WebKit.framework apps maybe leaking, maybe crashing, and maybe being okay, indefinitely, based on what each individual plugin vendor checks for. Especially given that the number of plugins using CA is still quite small, and evangelizing all of them to add an OS-version-based check would be quite easy.
Comment 7 Stuart Morgan 2012-07-07 03:27:49 PDT
(In reply to comment #6)
> - The seemingly logic approach of special-casing browsers that do the wrong thing by doing a UA-check to do the retain is, as far as I can tell, impossible to get right. It's impossible to enumerate the apps that are wrong because every WebKit.framework-based app is wrong, and they could have basically any UA.

I had a thought today; I'm not sure why this had never occurred to me before: instead of looking at the UA, a plugin vendor can look for support for the Invalidating Core Animation model. Since any non-Apple implementation of a CA plugin host would want to support ICA for performance reasons, plugin vendors could reasonably assume that a browser that doesn't support ICA must be WebKit.framework-based, and only then violate the spec by leaking the layer.

That solves the enumeration problem, and means that it's not fragile to users doing UA spoofing.
Comment 8 Ahmad Saleem 2022-06-22 16:27:40 PDT
NPAPI support is removed from Safari 14 onward and it is not supported in Webkit Builds like WebkitGTK as well. I think this can be marked as "RESOLVED WONTFIX". Thanks!
Comment 9 Ryosuke Niwa 2022-06-22 22:18:31 PDT
Yup, won't fix.