Make NPObjWrapper a proxy object

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

Posted patch PatchSplinter Review
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.
Attachment #8896772 - Flags: review?(wmccloskey)
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.
Attachment #8896772 - Flags: review?(wmccloskey) → review+
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.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09ece3c8484a
Make NPObjWrapper a JS proxy object. r=billm
https://hg.mozilla.org/mozilla-central/rev/09ece3c8484a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.