566818 - JS_Becomes not needed yet, should not be added even to jsproxy.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #546590 +++

The bug 546590 has added JS_Becomes as a generic function to change object's internal class. But its usage could be too dangerous at least with current API, Thus the engine should not exp[ose it and rather just inline the corresponding code in the implementation of Proxy.fix with big and clear comments about the safeness of its usage.

Comment 1

[Andreas Gal :gal]

memset is significantly more dangerous than becomes, yet we have no bug on file to remove it. Sure, becomes needs to be used with great care. Object::swap which becomes delegates to comes with this disclaimer:

/*                                                                               * Use this method with extreme caution. It trades the guts of two objects and updates
 * scope ownership. This operation is not thread-safe, just as fast array to slow array.
 * transitions are inherently not thread-safe. Don't perform a swap operation on objects
 * shared across threads or, or bad things will happen. You have been warned.
 */

JSAPI is for adults and we can't make it child proof if we want it to be powerful.

Comment 2

[Brendan Eich [:brendan]]

The becomes operations is critical for JS_THREADSAFE elimination.

I don't think this bug should block so many other bugs, some already fixed. Just the harmony:proxies bug would be enough, really.

/be

Comment 3

[Igor Bukanov]

Sorry for the dependency spam.

(In reply to comment #1)
> memset is significantly more dangerous than becomes, yet we have no bug on file
> to remove it.

The problem with JS_becomes is that it is hard to detect its obuses and analyze all the consequences. That is, it is extremely non-local as bugs its usage introduces could be spread through all the code. Just consider that its introduction without the later gal's fix for the proxy has would have added hazards in jsobj.cpp and jsarray.cpp. This is sort of similar to the initial introduction of scripted getters and setters. It took years too remove all the GC and other hazards associated with them. 

I do not want this story to repeat itself with the new proxy code. So IMO we should fix internal/public API first and only than add anything like JS_Becomes.

Comment 4

[Igor Bukanov]

(In reply to comment #2)
> The becomes operations is critical for JS_THREADSAFE elimination.

Surely, but lets consider the generalization in due time. Currently we only need this for proxy and to support Proxy->Ordinary transition. I doubt that we would need exactly the same functionality for JS_THREADSAFE especially with not yet clear interaction with the request model.

Comment 5

[Brendan Eich [:brendan]]

The thread-safe native-becomes-MT-proxy plan relies on the request model, see

http://groups.google.com/group/mozilla.dev.tech.js-engine/msg/af16c4039f402ceb

It is happening very soon.

Igor makes a good point about getters and setters. We should add to jsapi.h only after more private or friend API development. Not making JS_Becomes public API, instead js::Becomes or something like that in jsobj.h, seems fine. Gal?

/be

Comment 6

[Andreas Gal :gal]

obj->swap(obj2) is the internal API.

I don't really buy the omg its too dangerous for embedders argument. Everything in JSAPI can explode in your face. Its C/C++ after all. I can think of only 2 reasons not to expose JS_Becomes right away:

1. The API/arguments might change.
2. We might not want to expose this functionality after all.

I think neither argument holds here. The API is dead simple and won't change, and I do think we want this functionality very badly.

The reason I am exposing the public API is so we can use JS_Becomes as a choke point to pick and chose which operations we allow. We can for example limit JS_Becomes to native objects, and exclude certain classes.

While it can be a dangerous API call, memset is literally more dangerous, and you can implement your own JS_Becomes in 2 lines using memcpy.

So executive summery if you don't want to expose JS_Becomes, I don't really care, I use the internal obj->swap API anyway, but "omg it can be used wrong" is not a valid/strong argument imo.

Comment 7

[Robert Sayre]

(In reply to comment #6)
> 
> So executive summery if you don't want to expose JS_Becomes, I don't really
> care, I use the internal obj->swap API anyway, but "omg it can be used wrong"
> is not a valid/strong argument imo.

I have another argument. We don't seem willing or able to turn off API and script-exposed features that are useful in some way, no matter how costly or buggy they end up being over time. Just this week we have people spending time on the interaction of proxies with js_MarkSharpObjects. Let me tell you, neither of these things were at the top of my list when I wrote the Q2 goals. To refresh your memory, they are something like "faster, faster, faster, es5, debugger!"

As far as I can see, the proxy patch basically crash-landed on the TM tree (why?). I see no rush to add this stuff to the public API or expose it to general scripts at this point. Don't get me wrong, it's a cool feature, but we can use it internally first to shake out the bugs.

There's an old joke in the IETF: "we can't change it now, there are already 10 users!"

Comment 8

[Brendan Eich [:brendan]]

Proxies are more than a cool feature, they're the objectops make-over we've been in need of for years -- they're the path forward to faster DOM, MT proxies for no JS_THREADSAFE, and more self-hosting all over the place.

Not everything can be predicted to fit on a quarterly goals. Also I've said it before, if your quarterly goals are *only* "be like JSC" then we can dump TM and try to front the JS API to JSC, or rewrite all our JS API based code to use JSC's API, and then go to the beach. But that violates our mission as well as our users in many and deep ways.

/be

Comment 9

[Robert Sayre]

(In reply to comment #8)
> Proxies are more than a cool feature

Yes, they're great. I don't want to back them out or not do work on them.

[✔] they're the objectops make-over we've been in need of for years
[✔] they're the path forward to faster DOM
[✔] MT proxies for no JS_THREADSAFE

I'll get 100% behind all three points for this argument. To accomplish those goals, it seems to me that we don't need public API or general availability of Proxy in scripts for shipping browsers (nightlies can do whatever). We might decide it's ready to turn on for everyone, but I don't see what the rush is.

> 
> if your quarterly goals are *only* "be like JSC" 

Fortunately, that is not a fair description of the goals we have at the moment.

Comment 10

[Brendan Eich [:brendan]]

Comment 7 is also wrong in claiming "[w]e don't seem willing or able to turn off API and script-exposed features that are useful in some way." Indeed the bug referenced by "Just this week we have people spending time on the interaction of proxies with js_MarkSharpObjects" (bug 566141) is a pre-existing bug, nothing particular to do with Proxies.

We turned off __parent__ and __count__, we're turning off the 2nd argument to eval (*after* we talk to DigitalMe maintainers and other addon owners whose addons we host -- no "not my problem" or "take that, losers!" attitude toward our platform users, that is crap-tastic and too often cover for mediocrity elsewhere).

We turned off old getter/setter syntax (broke downrev Weave clients).

We may well turn off sharp variables, at least in Firefox, which being debated in bug 566700.

We're turning off stuff that won't be standardized, or that ends up standardized differently. This will include JS1.7 features in due course.

But we do *not* just yank stuff without fixing what we also host (our own apps, and extensions on addons.m.o). We don't provide zero recourse if there's a legitimate use case in our view (not all use-cases are legit -- __parent__ for all objects is not and never was, it was censored for Call objects). Yeah, "our view" may not be unitary, so I'm gonna call some shots in case of controversy.

If this is all too complicated and we can't compete on perf, I've yet to see technical evidence why. Instead, I see misdirection. Our extensions cost us, but they cost more beyond any intrinsic technical costs by being attractive "fight nuisances". The temptation to fight instead of doing work on the *almost entirely unrelated* performance and standard compat goals that you listed is a character flaw. Resist it.

Security attack surface is a problem, but it is too easy a proof. It strikes at much in Gecko. Self-hosting, removing dead wood after standardizing (or proto-standardizing, e.g. harmony:proxies) better limbs of the tree, and in any event not breaking backward compatibility solely to "reduce attack surface", can lead us to a better near-term future.

Especially with Proxies, without which it's all C++ attack surface until some big indefinite-future rewrite.

/be

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #9)
> (In reply to comment #8)
> > if your quarterly goals are *only* "be like JSC" 
> 
> Fortunately, that is not a fair description of the goals we have at the moment.

Why not? Sorry, I only saw your "faster, faster, faster, es5, debugger!" (a good list, just not complete).

JSC is implementing ES5. AFAICT they're ahead of us. Ollie is rewriting their parser to be top-down to implement strict mode, after which I think they are "done".

JSC has good debugger support, getting better all the time. Random commenters on ajaxian.com still say firebug is indispensable, but the gap is closing. Not that us trying to drop our 'monkey for JSC would give us debugger wins for free, just saying I still have a hard time reading your shorthand goals as other than "be like JSC".

Don't get me wrong, we could do worse than "be like JSC". We are learning and even using JSC code. But we can't switch engines and we have other goals beyond Apple's, in particular prototyping standards strawman proposals that also win for us, and supporting our platform while we evolve it to suck less.

/be

Comment 12

[Andreas Gal :gal]

As I have said above I don't mind dropping JS_Becomes, but I have still not heard a single valid argument why. But I am happy to drop it without a valid argument if that ends the discussion.

As for disabling proxies altogether, we should take that discussion to the proxies bug. Wrappers are about to use the internal API. The scripted part is not necessary for that. We can hide the "Proxy" name in product builds if we want to. I would like to have it enabled on trunk until we ship at the very least so we can judge compatibility. Disabling the scripted part for content is a 1-liner.

I see a lot of value in having it enabled for now. It seems to make it a lot easier to fuzz for certain hard non-native object cases using proxies. Gary already found 2 security bugs that were tripped by proxies being non-native, but don't really need proxies. Igor found a third one that can be triggered using other things, but also via proxies. I think there is more value in fixing these security bugs than discussing how to turn off what makes us find them.

Comment 13

[Robert Sayre]

(In reply to comment #10)
> Comment 7 is also wrong in claiming "[w]e don't seem willing or able to turn
> off API and script-exposed features that are useful in some way." 

I didn't intend that to sound so snide--sometimes we really can't turn stuff off, and sometimes we decide certain extensions and/or API will continue to be present even though only a few people use it.

> Indeed the
> bug referenced by "Just this week we have people spending time on the
> interaction of proxies with js_MarkSharpObjects" (bug 566141) is a pre-existing
> bug, nothing particular to do with Proxies.

I was going by
https://bugzilla.mozilla.org/show_bug.cgi?id=566141#c8

sure, not the "fault" of proxies, but it shows increased attack surface. And this bug is about creating API commitments for these things. For the third time, I don't see why we want to jump in here. It's not an insult to suggest these things should stay under the covers at first.

> The temptation to fight instead of doing work on the *almost
> entirely unrelated* performance

Looking at the followups, security / fuzzbugs, and prep work relating to performance patches we do will show them to be very much related, unfortunately. For example, I don't think other engines had to deal with __iterator__ and e4x when they were trying to speed up string-fasta.

We should be more careful about adding things that will create compatibility commitments and attack surface. We can get plenty of real world experience with proxies by using them internally. We just might want to change them after Firefox 4 ships.

Comment 14

[Andreas Gal :gal]

I attached a patch to disable scripted proxies to the proxies bug:

https://bugzilla.mozilla.org/attachment.cgi?id=446271&action=diff

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> I attached a patch to disable scripted proxies to the proxies bug:
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=446271&action=diff

Why bother to do that? Please if you are gonna do any such thing add a JS_HAS_HARMONY_PROXIES or whatever to jsversion.h, and use ifdefs.

/be

Comment 16

[Andreas Gal :gal]

I don't intend applying the patch. I am just pointing out what it takes to disable it for a product build if that decision is made.

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #13)
> > The temptation to fight instead of doing work on the *almost
> > entirely unrelated* performance
> 
> Looking at the followups, security / fuzzbugs, and prep work relating to
> performance patches we do will show them to be very much related,
> unfortunately. For example, I don't think other engines had to deal with
> __iterator__ and e4x when they were trying to speed up string-fasta.

We had to bypass those extensions. That's all ("almost entirely unrelated"). But you picked a good example.

There are other touch-points due to E4X being quasi-native, and stuff like destructuring gets in my hair -- still, the name of the game in the perf-wars is making the ES3-ish standard code fast. No benchmarks use these extensions, so bypasses are the most we have to sweat.

> We should be more careful about adding things that will create compatibility
> commitments and attack surface. We can get plenty of real world experience with
> proxies by using them internally. We just might want to change them after
> Firefox 4 ships.

We're not going to get the feedback everyone in TC39 is committed to getting if we don't have them on in TM and betas. Keeping them unscriptable is completely counterproductive apart from our wrapper use-cases, and undermines testing.

Take the heat, you're in the kitchen!

/be

Comment 18

[Brendan Eich [:brendan]]

Fuzz-generated bugs cause us stress. But "reducing attack surface" when there are other ways to provoke a bug than Proxies, by making Proxies unscriptable, does us no service. Proxies help us find some of these latent bugs. If they cannot be exploited except by Proxies, I'd like to see a case.

Arguing about increased attack surface is like arguing about being more pregnant. We simply have fuzz-bugs to fix, including by turning off old extensions. There will certainly be Proxy-only bugs, too. Those do not mean we bail on Proxies, any more than any bug means turn off the underlying "feature".

/be

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #13)
> We just might want to change them after Firefox 4 ships.

One last point, this is important: we will change them to track ES6. They're very likely to be in ES6, but nothing is sure (even that 6 will stick; might have to spin a quick ES5 rev and go to 7 for the done-by-2013 edition). Anyone using them before the new edition gets MDC warnings about their proto-standard status, how they might change due to the standard changing.

There's no other way to get the wide testing the TC39 members want. This is what w3c aspires to do with last-call and so on (observed more in the breach but it's still good). ES5 pretty much went the other way and we know how that sucks (bugs in the spec, hardships in the implementation, but very hard to change now that it's going to ISO).

Some versioning pain is inevitable, part of the web if only due to cross-browser interop. Since Proxies won't be cross-browser for some time, we have good ground for saying "beware standardization changes" and savvy developers (the kind who self-select into using Proxies) will cope. Anyone not tall enough to ride may fall off the roller coaster, but that's the web for you!

/be

Comment 20

[Robert Sayre]

(In reply to comment #17)
> (In reply to comment #13)
> > > The temptation to fight instead of doing work on the *almost
> > > entirely unrelated* performance
> > 
> > Looking at the followups, security / fuzzbugs, and prep work relating to
> > performance patches we do will show them to be very much related,
> > unfortunately. For example, I don't think other engines had to deal with
> > __iterator__ and e4x when they were trying to speed up string-fasta.
> 
> We had to bypass those extensions. That's all ("almost entirely unrelated").

I have the IM logs to prove this wasn't frictionless! :)


> > We should be more careful about adding things that will create compatibility
> > commitments and attack surface. We can get plenty of real world experience with
> > proxies by using them internally. We just might want to change them after
> > Firefox 4 ships.
> 
> We're not going to get the feedback everyone in TC39 is committed to getting if
> we don't have them on in TM and betas. 

In TM and betas, we can do whatever you want. Proxy objects for all web content is the right way to get data.

> Keeping them unscriptable is completely
> counterproductive apart from our wrapper use-cases, and undermines testing.

I wasn't clear about what I was suggesting. I've noticed that v8 has scriptable objects available only internally (#StringBuilder, etc), and not available to web content or browser extensions. If we need scriptable proxies in our own code, that is probably the right way to go at first, since I don't see how we'll have time to know whether the initial design is right. JS_Becomes can become js_Becomes... don't see the controversy there.

Comment 21

[Andreas Gal :gal]

No need for js_Becomes. We have obj->swap. JS_Becomes is in jsproxy.h and we can just drop it. I can use obj->swap from xpconnect. I was just trying to avoid using non-public APIs from xpconnect, but its not worth the discussion, really.

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #20)
> (In reply to comment #17)
> > (In reply to comment #13)
> > > > The temptation to fight instead of doing work on the *almost
> > > > entirely unrelated* performance
> > > 
> > > Looking at the followups, security / fuzzbugs, and prep work relating to
> > > performance patches we do will show them to be very much related,
> > > unfortunately. For example, I don't think other engines had to deal with
> > > __iterator__ and e4x when they were trying to speed up string-fasta.
> > 
> > We had to bypass those extensions. That's all ("almost entirely unrelated").
> 
> I have the IM logs to prove this wasn't frictionless! :)

Certainly not frictionless -- no frictionless lunches -- but this is a slightly different argument. The task to "go make benchmarks fast" is almost entirely about ES3 language performance, and the bypasses are mostly in place.

Redoing iteration for a speedup revisited the __iterator__ extension, but the imacros before that separated out the case where that property was found and used a pre-existing shape guard to bypass.

There's a cost for sure, especially for people newer to the code than shaver and I (and we forget stuff all the time). We have a perplexing pile of code. It would be simpler if it were ECMA-262 only, maybe with a few extensions that can play safe (in 20-20 hindsight). Not even sure getters and setters count as Igor pointed out. But this is not our world, it's much more like JSC's.

Recruiting new hackers means bracing them for the proto-standards mission, plus the cleanup old, under-rewritten code -- without falling into sophomore rewrite hell.

> In TM and betas, we can do whatever you want. Proxy objects for all web content
> is the right way to get data.

We should talk later about keeping them on for Fx4. It may be the right thing, just as we've done to evolve the language for the last umpteen years.

> I wasn't clear about what I was suggesting. I've noticed that v8 has scriptable
> objects available only internally (#StringBuilder, etc), and not available to
> web content or browser extensions.

Yeah, but they suck worse than JSC at not advancing standards, hiding behind the last edition or the upstream (JSC!).

> If we need scriptable proxies in our own
> code, that is probably the right way to go at first, since I don't see how
> we'll have time to know whether the initial design is right.

We have high confidence based on tomvc's work and its progenitor in AmbientTalk but who knows? If we play safe we may never know. Or we may give ground for a useless bikeshedding exercise in ES6's later development. Playing to win has its risks but they are not fatal; playing safe incurs risks too.

> JS_Becomes can become js_Becomes... don't see the controversy there.

I don't like gal backing down so easily -- wuss! :-P

I'm more concerned about other things, though, so I'm gonna wuss out too. Any C or C++ API has lots of sharp edges. JS API has many old, badly designed ones (to quote Sigourney Weaver from Galaxy Quest on flamethrowers at the exit of the "Chompers": "These serves no useful purpose! Whoever wrote this episode should die!"). JS_Becomes is hardly irrevocable or universe-ending, and I'm now in agreement with gal that we should keep it (for now).

Igor, what say you?

/be

Comment 23

[Brendan Eich [:brendan]]

Wait, JS_Becomes is in jsproxy.h? And jsproxy.h is not in EXPORTS? Why are we arguing?

Make it JS_FRIEND_API, keep it there.

/be

Comment 24

[Igor Bukanov]

I see 3 problems with JS_Becomes or even its internal version like JSObject::swap. 

First even a single use of it may require to patch later a lot of code just to make it safe. jsproxy.cpp contains 28 TryHandlerTrap calls to protect against damage that JSProxy.fix can induce. Granted, it could be possible to make the protection less noisy, but the fact that it was simpler to add those 28 calls than to write less noisy patch is telling.

Second so far JSProxy.fix is the only user of the API and nobody has asked about it before. I am hearing about using it for multi-threaded proxies, but we do not have a patch yet to judge the need for generalization.

Third the API cannot be used as is to change objects internals. Function instances are allocated through different GC free lists than other objects and swapping Function class for something else would require some considerations. I suppose other users of JS_Becomes would be required to do most of the checks that JS_FixProxy is doing to proper deal with functions. So JS_FixProxy is already low-level and adding sub-low level helpers like JSObject::swap IMO makes it harder to follow the code.

Comment 25

[Andreas Gal :gal]

Attachment: Make it a friend API as per brendan.

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #24)
> I see 3 problems with JS_Becomes or even its internal version like
> JSObject::swap. 

The summary of this bug should remain what Igor set it to be, since we are still considering becomes safety.

> First even a single use of it may require to patch later a lot of code just to
> make it safe. jsproxy.cpp contains 28 TryHandlerTrap calls to protect against
> damage that JSProxy.fix can induce. Granted, it could be possible to make the
> protection less noisy, but the fact that it was simpler to add those 28 calls
> than to write less noisy patch is telling.

I'm not sure it's that telling. Brute force when in doubt, and all that. The more subtle and smaller solution is not always the first solution.

> Second so far JSProxy.fix is the only user of the API and nobody has asked
> about it before. I am hearing about using it for multi-threaded proxies, but we
> do not have a patch yet to judge the need for generalization.

This is a fair point, so we aren't making a public API. This does not mean we take out becomes.

> Third the API cannot be used as is to change objects internals. Function
> instances are allocated through different GC free lists than other objects and
> swapping Function class for something else would require some considerations.

Agreed.

> I suppose other users of JS_Becomes would be required to do most of the checks
> that JS_FixProxy is doing to proper deal with functions. So JS_FixProxy is
> already low-level and adding sub-low level helpers like JSObject::swap IMO
> makes it harder to follow the code.

JS_FixProxy is not enough for JS_THREADSAFE elimination.

I think we are ok with engine-private becomes until proven good or bad. Is it fruitful to debate about future events? It's not fruitful to debate about public API since that is not being put forth.

/be

Comment 27

[Igor Bukanov]

(In reply to comment #26)
> This is a fair point, so we aren't making a public API. This does not mean we
> take out becomes.

OK, so lets just kill JS_Becomes and add better comments and asserts to JSObject::swap.

Comment 28

[Brendan Eich [:brendan]]

Resummarizing as Igor intended. I agree with this but it's an easy bug to fix, not a huge deal. We shouldn't add JS_* C functions to non-API (not matching js*api.h) headers anyway. If we need some kind of functional veneer, it would be js::becomes -- but isn't obj->swap(other) enough?

/be

Comment 29

[Andreas Gal :gal]

Attachment: patch

remove js_becomes and fix swap for function objects.

Comment 30

[Andreas Gal :gal]

Attachment: bonus: over-allocate FunctionProxyClass to the size of JSFunction so we can swap them later

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 446658 [details] [diff] [review]
bonus: over-allocate FunctionProxyClass to the size of JSFunction so we can swap them later

>+static size_t
>+GetObjectSize(JSObject *obj)
>+{
>+    /* We over-allocate function proxies so its safe to swap them with functions. */
>+    return ((obj->isFunction() && !obj->getPrivate()) ||
>+            obj->isFunctionProxy()) ? sizeof(JSFunction)
>+                                    : sizeof(JSObject);

Put the ? first on its own line, same for : right under it, both in the same column as the first ( around the condition.

Mentioned on iChat using js::Foo instead of js_Foo for the friend functions -- C++ only, no C.

/be

Comment 32

[Andreas Gal :gal]

Actually I confused myself a bit there. Proxies can never turn into functions, only into callable objects, so I took out the over allocation but left the swap support for functions in.

Comment 33

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/060a7e1a5635

Comment 34

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/060a7e1a5635