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:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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+
(Assignee)

Comment 2

2 years ago
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

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09ece3c8484a
Make NPObjWrapper a JS proxy object. r=billm

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/09ece3c8484a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.