Remove IndirectProxyHandler and simplify proxies

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

Assignee

Description

7 years ago
It's been been way more trouble than it's worth.

This is the first part of bug 800915.
Assignee

Comment 1

7 years ago
Let's just bite the bullet and do this here. It will let us get rid of
IndirectProxyHandler, which has really overcomplicated our proxy hierarchy.
Attachment #674716 - Flags: review?(ejpbruel)
Assignee

Comment 2

7 years ago
I don't ever remember them being moved, but this seems wrong. As long as we
continue to support function proxies, there's no reason that the call logic
should be limited only proxies with targets. In particular, this issue breaks
the SpecialPowers wrapper after making ScriptedIndirectProxyHandler inherit
BaseProxyHandler.
Attachment #674719 - Flags: review?(ejpbruel)
Assignee

Comment 3

7 years ago
There are no actual dependencies on IndirectProxyHandler, despite the fact that
it roughly mimics its behavior.
Attachment #674721 - Flags: review?(ejpbruel)
Assignee

Comment 4

7 years ago
We also take the opportunity to update the now-obsolete comment about the
wrapper hierarchy.
Attachment #674722 - Flags: review?
Assignee

Comment 5

7 years ago
Attachment #674723 - Flags: review?(ejpbruel)
Assignee

Updated

7 years ago
Attachment #674722 - Flags: review? → review?(ejpbruel)
Comment on attachment 674716 [details] [diff] [review]
Part 1 - Manually grab the BaseProxyHandler derived traps in SandboxProxyHandler. v1

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

::: js/xpconnect/wrappers/XrayWrapper.h
@@ +116,5 @@
> +    virtual bool set(JSContext *cx, JSObject *proxy, JSObject *receiver,
> +                     jsid id, bool strict, JS::Value *vp);
> +    virtual bool keys(JSContext *cx, JSObject *proxy, JS::AutoIdVector &props);
> +    virtual bool iterate(JSContext *cx, JSObject *proxy, unsigned flags,
> +                         JS::Value *vp);

Add MOZ_OVERRIDE to all those?
Comment on attachment 674716 [details] [diff] [review]
Part 1 - Manually grab the BaseProxyHandler derived traps in SandboxProxyHandler. v1

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

What Ms2ger said, and there's a typo in your comment:

110	    // We just forward the derived traps to the BaseProxyHandler versions which
111	    // implement them in terms of the funamental traps.

Otherwise, r+
Attachment #674716 - Flags: review?(ejpbruel) → review+
Comment on attachment 674719 [details] [diff] [review]
Part 2 - Restore BaseProxyHandler call/construct traps. v1

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

I don't remember these traps ever being on BaseProxyHandler, but putting them here won't break anything, and seems perfectly legit to me. r+
Attachment #674719 - Flags: review?(ejpbruel) → review+
Comment on attachment 674721 [details] [diff] [review]
Part 3 - Make ScriptedIndirectProxyHandler inherit BaseProxyHandler directly. v1

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

I remember us talking about this on irc. Looks good to me. r+
Attachment #674721 - Flags: review?(ejpbruel) → review+
Comment on attachment 674722 [details] [diff] [review]
Part 4 - Remove IndirectWrapper. v1

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

I don't feel particularly strong about this, but in my mind, its technically possible to 'unwrap' a DirectProxyHandler as well, using GetProxyTargetObject(proxy). If there's a distinction, it would be nice if the comment explained that. Other than that, I have no issues with this patch. r+
Attachment #674722 - Flags: review?(ejpbruel) → review+
Comment on attachment 674723 [details] [diff] [review]
Part 5 - Remove IndirectProxyHandler. v3

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

LGTM. r+
Attachment #674723 - Flags: review?(ejpbruel) → review+
Comment on attachment 674724 [details] [diff] [review]
Part 6 - Merge DirectWrapper and Wrapper. v1

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

All of this is pretty straightforward. I can't find anything wrong with it. r+
Attachment #674724 - Flags: review?(ejpbruel) → review+
Comment on attachment 674727 [details] [diff] [review]
Part 7 - Remove toBaseProxyHandler and toWrapper. v1

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

I'm glad we're getting rid of these. Thanks for doing this, Bobby. r+
Attachment #674727 - Flags: review?(ejpbruel) → review+
Since this landed, the panel presented by the tos;dr add-on[1] is showing up completely blank in Nightly builds. Was such a change in behavior expected?

[1] https://addons.mozilla.org/en-US/firefox/addon/terms-of-service-didnt-read/
Assignee

Comment 20

7 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #19)
> Since this landed, the panel presented by the tos;dr add-on[1] is showing up
> completely blank in Nightly builds. Was such a change in behavior expected?

Nope. I'm pretty surprised actually, since this should be just a simple refactor. File a bug? I assume you bisected to this revision? A reduced testcase would definitely be helpful. :P
Depends on: 807623
Yes, I bisected and it looks like Part 3 here is the culprit. Filed bug 807623.

Updated

7 years ago
Depends on: 807644
You need to log in before you can comment on or make changes to this bug.