745422 - Wrapper fundamental traps should be factored out into a separate base class

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

We need this to avoid duplicating js::Wrapper code in bug 726949.

We want to use the forwarding behavior of js::Wrapper, but have the derived traps implemented with fundamental traps (a la ProxyHandler) rather than implemented directly. This is easy enough to do.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Factor fundamental traps into js::AbstractWrapper. v1

Attaching a patch. Flagging luke for review.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 615011 [details] [diff] [review]
Factor fundamental traps into js::AbstractWrapper. v1

Flagging bz for feedback to make sure it meets his use case.

Comment 3

[Bobby Holley (:bholley)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=7d9604bcf506

Comment 4

[Andreas Gal :gal]

Comment on attachment 615011 [details] [diff] [review]
Factor fundamental traps into js::AbstractWrapper. v1

Nice. I like it.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 615011 [details] [diff] [review]
Factor fundamental traps into js::AbstractWrapper. v1

Given that we talked about it already, I'm going to skip the feedback? and land this so that it's ready for bz on monday.

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/1b19214a0a50

Comment 6

[Boris Zbarsky [:bzbarsky]]

Looks good, but s/funamental/fundamental/?

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> Looks good, but s/funamental/fundamental/?

Already landed it - can you piggyback that on your patch?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Sure.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 615011 [details] [diff] [review]
Factor fundamental traps into js::AbstractWrapper. v1

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

::: js/src/jswrapper.h
@@ +50,5 @@
>  namespace js {
>  
>  class DummyFrameGuard;
>  
> +/* Base class that just implements no-op forwarding methods for funamental

"funamental"

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/1b19214a0a50

Comment 11

[Boris Zbarsky [:bzbarsky]]

So wait.  We have code that assumes that isWrapper() is enough to call Wrapper::wrapperHandler and then examine the flags.  How is that supposed to work after this patch for things that are AbstractWrapper but not Wrapper?  Won't it just examine random memory?

Comment 12

[Boris Zbarsky [:bzbarsky]]

And yes, I'm running into this right this moment.