1389949 - Make NPObjWrapper a proxy object

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

We want to remove the getProperty and setProperty ClassOps, see bug 1389510. There are only a few uses left, one of them is NPObjWrapper.

NPObjWrapper acts like a proxy (forwarding property gets/sets to the underlying plugin object) and so we should just make it an actual proxy object.

I'm happy with how this turned out, most of the class hooks map well to the proxy traps and we don't need that many code changes. All tests pass and I tested the Flash plugin on a number of websites, including the YouTube flash player, and everything works fine.

Bill, flagging you as reviewer because you're familiar with the JS API and have reviewed plugin code before, but feel free to redirect.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8896772 [details] [diff] [review]
Patch

Review of attachment 8896772 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +172,5 @@
> +
> +  bool defineProperty(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id,
> +                      JS::Handle<JS::PropertyDescriptor> desc,
> +                      JS::ObjectOpResult& result) const override {
> +    ::JS_ReportErrorASCII(cx, "Trying to add unsupported property on NPObject!");

This seems like a difference from the old scheme. Previously, wouldn't Object.defineProperty have succeeded if the property was one that the plugin supported and it was just modifying the value?

Maybe that's fine. I have no idea how this stuff is used in practice.

Comment 2

[Jan de Mooij [:jandem]]

Thanks for the review!

(In reply to Bill McCloskey (:billm) from comment #1)
> This seems like a difference from the old scheme. Previously, wouldn't
> Object.defineProperty have succeeded if the property was one that the plugin
> supported and it was just modifying the value?

Looking at the code, I don't think we call the setProperty hook when *defining* a property. We do call addProperty (which would succeed in this case). My best guess is we would then just define a native data property on the object, bypassing setProperty, pretty weird.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09ece3c8484a
Make NPObjWrapper a JS proxy object. r=billm

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/09ece3c8484a