The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Created attachment 615011 [details] [diff] [review]
Factor fundamental traps into js::AbstractWrapper. v1

Attaching a patch. Flagging luke for review.
Attachment #615011 - Flags: review?(luke)
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.
Attachment #615011 - Flags: feedback?(bzbarsky)
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=7d9604bcf506

Comment 4

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

Nice. I like it.
Attachment #615011 - Flags: review?(luke) → review+
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
Attachment #615011 - Flags: feedback?(bzbarsky)
Target Milestone: --- → mozilla14
Looks good, but s/funamental/fundamental/?
(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?
Sure.
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"
https://hg.mozilla.org/mozilla-central/rev/1b19214a0a50
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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?
And yes, I'm running into this right this moment.
You need to log in before you can comment on or make changes to this bug.