This came up in the critsmash meeting... there appear to be a bunch of sharp variable-related critical security bugs and roc was making the case to remove it. This is a thorny issue, but we might as well have a bug on file and the arguments in a central place. So, here goes...

Actually I know next to nothing about sharp variables. I just observed that a) at least two of the 30-odd sg-crits across Gecko that we discussed today involved sharps b) no other JS engine supports them and c) they're not in ES5, so why shouldn't we disable them?
(This is a non-rhetorical question, e.g. "We are trying to get them into some future standard" would be a fine answer.)

Couple of points:
1. They've been part of SpiderMonkey for a long while, so they are used. Even by our tests and shell output. Some embedders use them for full serialization of cyclic/multiply-connected graphs (works so long as you avoid built-in objects; like cyclic JSON).
2. The bugs are AFAIK all self-discovered. They come from us, from fuzzers in the vast majority, and we are lucky to have a lack of attention due to sharp variables being non-standard. This could change to our detriment.
There's been talk of a standard toSource method based on let expressions (http://wiki.ecmascript.org/doku.php?id=strawman:let_expressions or our ES4-era variant) to bind the sharp variables as non-magic lexical bindings, but (a) that's work; (b) it requires alpha-renaming, very hard in the face of eval. No strawman proposal yet, so this is not on the Harmony radar.
We could rip out sharps but then we'd break uneval/toSource, also used by the serializing embedders and pretty-printers (shell output falls into this category).
Sharps have by my measure (cvs and hg log comment skim) caused more self-filed sg:crit trouble than E4X, for instance, and they're arguably less useful, but when you need to see the join and cycle points, or more crucially, eval back to an object graph that is isomorphic to what was uneval'ed, then there is no substitute.
JS lacks Object.hashcode still, so keeping a memo of object references means O(n^2)-compounding linear search of an array. But Object.hashcode is on track for the next edition of the standard, along with let expressions. So the possibility of making sharp variables into "library code" (removing them from the source language, meaning from SpiderMonkey's lexer and parser, having them appear only in strings generated by toSource and perhaps uneval, not recognized by eval) is before us.
At this point we are going to fix the two sg:crits anyway, to backport the fixes. Then we could either remove sharps, and probably toSource and uneval, for Firefox 4. That would break some people's code. Maybe they'd hear in time and try to cope, but we haven't prototyped Object.hashcode.
Of course we could just say "too bad". Platforms don't thrive by keeping dead wood piled high, that leads to bug infestations if not fires. But successful platforms provide better forms to supplant bad old ones, and look for uptake or at least usability evidence from real users, before cutting the old stuff. This is obvious from Unix system call tables, and Windows.
The same sort of argument about "not in the competition" could be made about a great many things, including XSLT (zero-day at the end of 2008 IIRC), XPath, a bit of CSS, and even SVG a few years ago. Some of that was standards-track if not standardized, but we rightly objected (e.g. in the case of SVG) to the quality of the standard as well as process-rules violations that resulted in the de-jure standard.
We are a platform, and even if we wish we were just a browser (a fast way to go out of business in view of our best funded and fastest-performing competition), we have platform commitments. Sharps are a small part of these promises. They are useful when dealing with non-tree JS object graphs.
So I'd rather fix them (since we have to for maintenance releases) and see about standardizing something like them, possibly without source language syntax, and even if not for ES6, than rip them out. But I'd like to hear from Igor, who has had to bear the brunt lately. Cc'ing dherman for his thoughts too.
/be

(In reply to comment #2)
> The same sort of argument about "not in the competition" could be made about a
> great many things, including XSLT (zero-day at the end of 2008 IIRC), XPath, a
> bit of CSS, and even SVG a few years ago.
FWIW XSLT and XPath have been in other browsers since long before 2008, and they get used a little bit on the Web.
One option you didn't analyze would be to disable sharps (and E4X and other trouble) for Web content but not for embedders.

SpiderMonkey embedders are one class of platform users, but sharps (and E4X, which is an ISO standard, and a subject Ecma TC39 is committed to returning to) are used by Firefox users. I don't think our front-end XUL JS does, but it did back in the day (Ben Goodger). I believe I saw sharps in Google's anti-phishing JS too.
We try not to split the platform with too many ifdefs. Roc, you'll be pleased to hear we are very close to eliminating JS_THREADSAFE (for Fx4).
/be

You mention sharps and their connection to uneval/toSource. It seems worth noting that it's unclear the semantics of uneval/toSource are what a programmer writing code actually wants. Neither can, or will, capture the precise original semantics of the method or object in question due to free variables. Sharpened getters and setters are another breakdown point. Arrays with non-array indexes will not be faithfully transcribed. Object prototypes, and properties found on them, don't appear in transcriptions, also breaking instanceof. Equality naturally breaks. ES5's more powerful property and object-mutation controls (writability, configurability, enumerability, extensibility) admit further information not exposed by to-source conversion. Programmers receive all this information in the debugger, but they get none of it with uneval.
uneval also doesn't preserve watches. ;-)

(In reply to comment #6)
> > Sharps have by my measure (cvs and hg log comment skim) caused more self-filed
> > sg:crit trouble than E4X, for instance
>
> I see 7 for sharps and 19 for E4X. Too many.
>
> https://bugzilla.mozilla.org/buglist.cgi?quicksearch=ALL+%22sg:crit%22+engine+sharp
>
> https://bugzilla.mozilla.org/buglist.cgi?quicksearch=ALL+%22sg:crit%22+engine+e4x%2Cxml
You are over-counting here. Consider bug 561539 -- it's not an E4X bug, rather a bug in weak root management. E4X helped find the bug. AFAICS it could bite any object.
Re: comment 7 -- same applies to JSON, but it's still useful. Sharp variables are like JSON with cycles and join points. Systems like JSON (a standard now) and uneval/eval in SpiderMonkey can be useful without being fully meta-programmable and property (including private data in host objects) preserving. Canvas doesn't have a retained model as SVG does, so you have to do your own redrawing. Etc.
This is not a "nit-pick sharps" bug, since the same kind of approach would find fault with every last web standard, starting with HTML and including standard CSS and JS. This is about the trade-offs (imponderable though they may be) in removing sharps. At least (thanks to mrkbap) it's a forthright, standalone bug. :-/
/be

Okay, JSON intentionally limits its own scope (same for canvas and others). I buy that. Indeed it's precisely that limitation which makes it useful. Analogously, uneval/toSource limits itself to source-text- and reevaluation-compatibility. For the design space, that's a fairly small limitation, certainly much smaller than the limits JSON chooses.
I don't think this should be purely a nit-bug either, but nits certainly have relevance to the overall picture and analysis.

Yeah, sharp variables are scoped narrowly, a subset language if you will:
JSON < eval/uneval < ES3+eval/uneval
Sharps came from Common Lisp, which has the similar JSON+functions+join/cycle-points scope.
/be

(In reply to comment #14)
> The first hit is not in our code. Google's code search indexes obsolete source
> code as well as up-to-date source code.
Why in the world do they do that?
I thought that old Ben G. code was changed, was surprised to see it in codesearch results. Thanks for clearing that up!
/be

This is a long comment, but bottom line: A fairly thorough effort to try to find all sharp variable users found two known users and one more suspected user. We do not use them at all.
- - - -
I searched our codebase for sharp variables and found none except in JS regression tests.
Searching for the identifiers |JS_ValueToSource|, |toSource|, and |uneval| found more tests, plus a few places where toSource or uneval is used to generate human-readable text, such as the shell REPL and the browser's console. In every case the string clearly isn't being round-tripped via eval, so sharp variables are not being used as a language feature.
Searching bugzilla for other possible users of sharp variables, I found bug 367600 and bug 335051, both filed several years ago and by the same person. We can ask him.
An AMO search turns up a small number of places where people do things like:
GM_setValue('tnt_twitter.friend_cache',uneval(tnt_twitter.friend_cache));
...
tnt_twitter.friend_cache = eval(GM_getValue('tnt_twitter.friend_cache')) || {};
but in this particular example the object graph being serialized never contains loops, and I'd be willing to examine them all case by case. It's a small number of candidates, a couple dozen.
Via Google Code Search, I found some js-beautify unit tests, somebody's GreaseMonkey user script for ikarium.com (I submitted a 2-line patch to fix this one; it took 15 seconds), someone else's Vimperator plugin, and a copy of jsfunfuzz. The rest is all obsolete copies of Mozilla or our own regression tests.
Balancing all this against the maintenance cost and the security and stability issues, I think it is clear that sharp variables have to go.

FWIW, here's my current thinking: TC39 may want to consider something in this space for creating DAGs and cyclic graphs with immutable data structures like the records and tuples of Brendan's "Harmony of my dreams" blog post. But I don't think the full generality of sharp-expressions as they exist in SpiderMonkey is necessary. We should consider more restricted sub-languages for cyclic data-structures [1].
So I'm personally fine with eliminating sharp-expressions to clear the space for a more restricted, better-behaved version down the road. I doubt I will have enough time to spec it for Harmony, and it's not as high-priority to me as other features we're working on.
One risk of keeping them around (in addition to the existing security risks) is that if we do come up with something for ES^n using similar syntax, we will possibly have to make incompatible changes.
But I'm totally unqualified to comment on what the implications for existing users would be if we eliminate them. I leave that to y'all to decide.
Dave
[1] Example: http://docs.racket-lang.org/reference/shared.html -- the printed results in the examples use sharp-syntax, but sharp-syntax *can't* be used in source programs to create cycles. The Racket compiler rejects cyclic AST's.

Comment 16 clearly misses Wes and his employees' firewalled content that uses sharps. SpiderMonkey has embeddings like PTC's ProEngineer one, maybe sharps matter there.
My point is googling is not enough, if you're trying to compare the apples in users' and embedders' eyes against the oranges of our maintenance burden.
Our maintenance burden is non-trivial. But for sharps, at least since the bad old MarkSharpObjects bugs were fixed, the patches are often easier than thought at first (see bug 630377 if you have access).
I think this bug should be dup'ed against bug 486643; I'll make it depend on that bug for now. Deprecate before obsoleting. To enable JS users to self-host an object graph codec, we would need weak maps, which are on the near-term "prototype the Harmonious proposal" agenda. So bug 486643 depends on bug 547941.
/be
/be

(In reply to comment #19)
> Generally speaking, in the rest of the browser we haven't let corporate
> intranet usage
This isn't necessarily about corporate intranet usage. Google can't see a lot of the web. Wes should weigh in.
> have much influence over our priorities for the Web and for our
> own product.
We could let the embedders configure sharps on, while we turn them off, but the ifdef'ed code will rot and we are trying to remove ifdefs, not add or perpetuate them.
As soon as WeakMaps are in we can start deprecating sharps. The cycle can be quick with the right incentives on all sides. Just yanking stuff on short or no notice, where that stuff that has valued users, is bad business.
/be

(In reply to comment #20)
> This isn't necessarily about corporate intranet usage. Google can't see a lot
> of the web. Wes should weigh in.
I think we need positive evidence to justify continued investment in sharps, not just fears that Jason's search might not have been exhaustive enough.
> We could let the embedders configure sharps on, while we turn them off, but the
> ifdef'ed code will rot and we are trying to remove ifdefs, not add or
> perpetuate them.
Sharp-using embedders can maintain the #ifdefed code if they want to.
> Just yanking stuff on short or no notice, where that stuff that has valued
> users, is bad business.
Yeah but who are the valued users? Wes. Who else?

(In reply to comment #21)
> (In reply to comment #20)
> > This isn't necessarily about corporate intranet usage. Google can't see a lot
> > of the web. Wes should weigh in.
>
> I think we need positive evidence to justify continued investment in sharps,
> not just fears that Jason's search might not have been exhaustive enough.
We got some on m.d.t.js-engine, but this is not an exact evidence-based science, and you know it.
Does Google Maps still use XSLT? If not, can we remove that?
Same goes for other parts of Gecko.
We're not making "continued investments" beyond bug fixes. Removing costs us too and we're not doing it without deprecation and obsolescence after a decent interval. I'm going to throw some weight behind this.
> > We could let the embedders configure sharps on, while we turn them off, but the
> > ifdef'ed code will rot and we are trying to remove ifdefs, not add or
> > perpetuate them.
>
> Sharp-using embedders can maintain the #ifdefed code if they want to.
See what I wrote. We're not taking sporadic patches from embedders who work downstream a ways, and anyway, we're trying to remove ifdefs.
> > Just yanking stuff on short or no notice, where that stuff that has valued
> > users, is bad business.
>
> Yeah but who are the valued users? Wes. Who else?
Why not work on those graphics blockers you told me were so important compared to, say competing with IE9 on drawImage perf? I note pwalton helping out.
Yes, I'm being "mean" here. And you are out of line.
/be

In case it is not crystal clear, I'm module owner and I'm against sudden-death for features that have clear users and some deprecation/obsolescence path ahead of them in existing, lower-numbered bugs.
We have had a real zero-day in XSLT. Where were the long knives?
You can appeal the module ownership module if you want to overrule me. In the mean time: blockers.
/be

Speaking as an embedder not visible to Google ;) or even m.d.t.js-engine, we're not using sharps yet but have serialisation on our roadmap. So I wouldn't be too happy to see sharps go without replacement (weak maps sound like they would be adequate).
Even if we need to put a little bit of effort into making sharp-based serialisation work for our purposes, it's a much better starting point than none.
Having them #if'd out probably isn't much use as I discovered when trying to revive the #if'd File object the other day... ;)
Joachim

We will remove sharp variables after deprecating them in favor of a library based on WeakMaps. This could happen by Firefox 5. I've taken the blocking bug 486643. This one can stay in the pool until that bug is fixed, after which the patch for this bug will remove all sharp-related code.
/be

(In reply to comment #25)
> We will remove sharp variables after deprecating them in favor of a library
> based on WeakMaps. This could happen by Firefox 5.
What about addressing the bug 633278 as a part of the deprecation process?

(In reply to comment #27)
> (In reply to comment #25)
> > We will remove sharp variables after deprecating them in favor of a library
> > based on WeakMaps. This could happen by Firefox 5.
>
> What about addressing the bug 633278 as a part of the deprecation process?
That doesn't necessarily help. It costs us to fix, and it adds versionitis. If we are to remove sharps, then why go through intermediate states?
One reason would be that the replacement library built on WeakMaps enforces the restriction proposed in that bug. But again we could just cut to the chase.
/be

Part of the problem (I've done it too) is hackers want to get rid of sharps but also or first, reform them. Unless there's a bad bug requiring fiddling with them I say it's better to leave them alone and work on the replacement library and its weakmap infrastructure.
Replacement library code will appear as attachments to bug 486643.
/be

(In reply to comment #29)
> Part of the problem (I've done it too) is hackers want to get rid of sharps but
> also or first, reform them. Unless there's a bad bug requiring fiddling with
> them I say it's better to leave them alone and work on the replacement library
> and its weakmap infrastructure.
This a false dichotomy; my position is somehow unhackerly? I think we *should* keep sharps; they're a good, necessary thing. Restricting them as suggested in bug 633278 will greatly reduce the complexity involved, as their range becomes much more restricted, without losing any necessary behavior in the cases where sharps work properly.

(In reply to comment #30)
> This a false dichotomy; my position is somehow unhackerly? I think we *should*
> keep sharps; they're a good, necessary thing.
The position that sharp variables if they were restricted further would be good and worth having is a distinctly minority view, one espoused by no one else in this bug.
> Restricting them as suggested in bug 633278 will greatly reduce the complexity
> involved, as their range becomes much more restricted, without losing any
> necessary behavior in the cases where sharps work properly.
Even if they were restricted, the larger points against them -- that very very very few people use them, that they aren't implemented by any other browsers, that they impose parsing and execution complexity, that they complicate implementation, that these costs are ongoing, and perhaps most of all that there isn't a compelling case that cyclic values require first-class syntactic support -- remain.
But this is all more or less by the wayside if we've decided sharp variables are going away with the arrival of weak maps or what-have-you -- just in need of clarification for you.

> This a false dichotomy; my position is somehow unhackerly? I think we *should*
> keep sharps; they're a good, necessary thing. Restricting them as suggested in
> bug 633278 will greatly reduce the complexity involved, as their range becomes
> much more restricted, without losing any necessary behavior in the cases where
> sharps work properly.
We should work on alternatives, but "we should have sharps" is putting the syntax cart before the semantics horse. We're talking about a rationalized serialization feature that can represent sharing. Maybe it's a library, maybe it needs syntax. I don't have a clear opinion yet, but I think people should actually advance a concrete design, rather than chipping away piece by piece at the existing feature.
Dave

> The position that sharp variables if they were restricted further would be good
> and worth having is a distinctly minority view, one espoused by no one else in
> this bug.
See comment 17 first paragraph.
Dave

(In reply to comment #25)
> We will remove sharp variables after deprecating them in favor of a library
> based on WeakMaps. This could happen by Firefox 5.
Let's try to focus the comments in this bug on getting this done. We need:
- Change toSource/uneval to stop using sharp variables
- Land WeakMaps (bug 547941 - the tricky bit)
- Provide sample code of how to serialize cyclic object graphs in pure JS
using WeakMap (bug 486643, see attachment there)
- Remove all sharp-related code (this bug, easy patch)
Is there already a bug on toSource/uneval? What exactly should they do with cyclic objects?
How soon do the other pieces need to be in place in order for us to remove sharp variables in Firefox 5?
Did I miss any steps?

(In reply to comment #30)
> I think we *should* keep sharps; they're a good, necessary thing.
I know only two decoders that can read serialized object graph with sharps. They are SM and Narcissus parsers. So currently sharps cannot be a necessary thing as they can be replaced with explicit JS code that the parsers would read just as well.
IMO sharps could only be good if one want JSON-on-steroids format that allows to write/read an arbitrary object graph. But such format is unrelated to JS syntax.

(In reply to comment #30)
> (In reply to comment #29)
> > Part of the problem (I've done it too) is hackers want to get rid of sharps but
> > also or first, reform them. Unless there's a bad bug requiring fiddling with
> > them I say it's better to leave them alone and work on the replacement library
> > and its weakmap infrastructure.
>
> This a false dichotomy
I wasn't making a dichotomy.
But we should definitely look at fuzz bugs filed in the last couple of days instead of chatting here! There, how's that?
/be

(In reply to comment #35)
> (In reply to comment #25)
> > We will remove sharp variables after deprecating them in favor of a library
> > based on WeakMaps. This could happen by Firefox 5.
>
> Let's try to focus the comments in this bug on getting this done. We need:
>
> - Change toSource/uneval to stop using sharp variables
That's this bug, but I suggest removing these methods altogether. Not making some fiddled-with, chipped-away-at "third way". Tertium non datur.
> - Land WeakMaps (bug 547941 - the tricky bit)
Gal is on it, WeakMaps are in harmony:proposals, this is a near-term goal.
> - Provide sample code of how to serialize cyclic object graphs in pure JS
> using WeakMap (bug 486643, see attachment there)
Nice, that's the bug for it.
> - Remove all sharp-related code (this bug, easy patch)
I think that is your first bullet point too.
> Is there already a bug on toSource/uneval? What exactly should they do with
> cyclic objects?
They're non-standard and part of the JS_HAS_SHARP_VARS configury (or were). They are a package deal and "just library code", so the built-ins all go once we have deprecated and evangelized with the library code you've started.
> How soon do the other pieces need to be in place in order for us to remove
> sharp variables in Firefox 5?
Do not fix this bug before its dependencies.
/be

(In reply to comment #35)
> - Change toSource/uneval to stop using sharp variables
> - Land WeakMaps (bug 547941 - the tricky bit)
> - Provide sample code of how to serialize cyclic object graphs in pure JS
> using WeakMap (bug 486643, see attachment there)
> - Remove all sharp-related code (this bug, easy patch)
Before doing that we should clarify the uneval behavior when one has custom toSource methods. Currently they are supported and executed during the serialization phase while they are recorded in the object map. That leads to surprises. For example, consider the following example that demonstrates a class that prepare custom serilaizer that should allow to reconstruct both a custom prototype and the constructor argument:
function MyClass(arg) {
this.arg = arg;
}
MyClass.prototype.toSource = function() {
var arg_source = uneval(this.arg);
return '(new MyClass('+arg_source+'))';
}
var arg = [1,2,3];
var a = new MyClass(arg);
var b = new MyClass(arg);
print(uneval([a, b]));
print(uneval([a, a]));
print(uneval([a, a, b]));
When run in shell the output is:
[(new MyClass(#1=[1, 2, 3])), (new MyClass(#1#))]
[(new MyClass([1, 2, 3])), (new MyClass([1, 2, 3]))]
[(new MyClass(#2=[1, 2, 3])), (new MyClass(#2#)), (new MyClass(#2#))]
Note how nicely sharps works for the first line fully preserving the graph. But the second and third are wrong - the second one duplicates both "a" and "arg" while the third line duplicates "a".
It would be nice if the new let or function closure based uneval() would allow to implement a custom serializer that could properly integrates into the object graph in all cases.

(In reply to comment #39)
> Before doing that we should clarify the uneval behavior when one has custom
> toSource methods. [...]
I noticed all this weirdness too.
Comment 38 proposes putting WeakMap in users' hands and letting them take care of it. I think that is the right approach.

(In reply to comment #38)
> > - Change toSource/uneval to stop using sharp variables
>
> That's this bug, but I suggest removing these methods altogether. Not making
> some fiddled-with, chipped-away-at "third way". Tertium non datur.
I'm all for that. Remove JS_ValueToSource too? We can either check in toSource.js for the shell REPL's benefit or revert to printing ToString(result) as we were doing before. The Firefox Web console uses uneval but I bet they'll be even happier with a JS implementation of it.
This means toSource.js needs to include stuff like Date.prototype.toSource. I'll make a note in the bug.
> > How soon do the other pieces need to be in place in order for us to remove
> > sharp variables in Firefox 5?
>
> Do not fix this bug before its dependencies.
Just trying to plan ahead.

(In reply to comment #41)
> I'm all for that. Remove JS_ValueToSource too?
Yes, see below.
> We can either check in
> toSource.js for the shell REPL's benefit or revert to printing ToString(result)
> as we were doing before.
Let's dogfood toSource.js -- why not?
> The Firefox Web console uses uneval but I bet they'll
> be even happier with a JS implementation of it.
Right, dogfood is yummy.
> This means toSource.js needs to include stuff like Date.prototype.toSource.
> I'll make a note in the bug.
Thanks.
/be

(In reply to comment #40)
> Comment 38 proposes putting WeakMap in users' hands and letting them take care
> of it. I think that is the right approach.
Yeah, I'm persuaded, too. If the user can define toSource functions, then the language layer can't reliably do graph printing that respects whatever semantics the user had in mind when they wrote their toSource function. The best solution is to allow the same user who wrote the toSource function to write the graph traversal code as well.

Created attachment 590527[details][diff][review]
Remove it from serialization, too
I missed a few bits in toSource code.
For the first patch:
56 files changed, 79 insertions(+), 1112 deletions(-) (with test changes)
23 files changed, 42 insertions(+), 677 deletions(-) (without test changes)
For the second patch:
3 files changed, 65 insertions(+), 128 deletions(-)
The second patch mostly preserves the existing structure of serialization. I only barely understand it, myself, and would remove or rewrite it if the opportunity presented itself. But that's a rather bigger change, compared to this minimal change. (Well, not absolutely minimal, as I did a bit of moving declarations from C position to C++ position, to aid in understanding. But still it's functionally much more minimal than it could be.)

(In reply to Dave Herman [:dherman] from comment #48)
> Not to be difficult, but have we helped the addons that use sharps to update
> their code?
>
> Dave
Do we know of any add-ons using sharp variables? I know I've never run into one, and add-on developers rarely stray away from the most basic language features.