It's been been way more trouble than it's worth. This is the first part of bug 800915.

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.

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.

There are no actual dependencies on IndirectProxyHandler, despite the fact that it roughly mimics its behavior.

We also take the opportunity to update the now-obsolete comment about the wrapper hierarchy.

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+

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+

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+

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+

Comment on attachment 674723 [details] [diff] [review] Part 5 - Remove IndirectProxyHandler. v3 Review of attachment 674723 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. r+

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+

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+

Looks green: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1413ce44a30f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4a812a19c267 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/779d380a77f8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/da2e3ebd1c26 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7488590fc194 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/44d874f51c28 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a2b481bb4d1

https://hg.mozilla.org/mozilla-central/rev/1413ce44a30f https://hg.mozilla.org/mozilla-central/rev/4a812a19c267 https://hg.mozilla.org/mozilla-central/rev/779d380a77f8 https://hg.mozilla.org/mozilla-central/rev/da2e3ebd1c26 https://hg.mozilla.org/mozilla-central/rev/7488590fc194 https://hg.mozilla.org/mozilla-central/rev/44d874f51c28 https://hg.mozilla.org/mozilla-central/rev/8a2b481bb4d1

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/

(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

Yes, I bisected and it looks like Part 3 here is the culprit. Filed bug 807623.