580389 - Ecmascript5 - Implementing features where possible for XULRunner < 2.*

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: implementation of es5 additions

Alternatively you can pull / look at changes here:

http://github.com/Gozala/jetpack-sdk/commit/8432f20f027f0733292d7e01a69ddcc332c8e42e

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Added license block and updated few comments

http://github.com/Gozala/jetpack-sdk/commit/e947e42c9434b763bb8eba0653d667dc5973e661

Comment 3

[Myk Melez [:myk] [@mykmelez]]

I'm still working my way through the patch, but one general thought: it's not clear to me that we should be offering implementations of features that don't actually do what the spec specifies.  It seems like it would be better to only implement those features that we can actually make to work according to spec.  Otherwise we will mislead developers into thinking the features actually work, which will lead to unexpected behavior and bugginess when they don't.

Object.seal and Object.freeze are good examples.  Neither has any effect, but developers might assume they do because they're present, in which case they'll be in for a surprise when objects they think are sealed or frozen turn out not to be.

Wouldn't it be better to only implement and start using those features that can actually be made to work?

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Probably you are right. But in case FF4 is going to be shipped with those features I still think it makes sense to freeze / seal objects and if it will remain mutable in older versions, well it was always the case so not a big deal.

If we'll keep them probably makes sense to note in the docs that it will work only in FF4>

BTW whom can I get from what is planned from es5 for FF4?

Comment 5

[Myk Melez [:myk] [@mykmelez]]

I chatted with Irakli offline, and I now understand the use case for this better.

The primary goal is to enable core Jetpack SDK modules to use ES5 features like Object.freeze to improve security on an ES5-capable version of Firefox while continuing to work (albeit without the improvements) on older versions of Firefox.

A secondary goal is to enable modules to move from nonstandard syntax (like __defineGetter__) and custom implementations (like Array constructor checks) to ES5 standard equivalents.

In order to achieve the first goal, Object.freeze needs to be defined and to behave as though it worked (i.e. to not throw an exception unless an exception would be thrown by an ES5-capable version of Firefox for a given argument).

And it's a worthy goal, so the current implementation in the patch is the right one.  We'll just have to make sure to document the purpose of this work well to reduce the risk of misleading people into thinking it actually implements all these features in a non-ES5 browser.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 459066 [details] [diff] [review]
Added license block and updated few comments

Overall, this looks great!  Just a few minor issues, nothing major.

Some general notes:

* end statements with semi-colons (except for single statements in one-line blocks), which is the convention in Firefox and Jetpack code to avoid bugs that tend to crop up in code that doesn't use them;
* use two-space indent for consistency with Firefox and Jetpack code.

Also note other good recommendations in the style guides:

  https://developer.mozilla.org/En/Developer_Guide/Coding_Style
  https://wiki.mozilla.org/Labs/Jetpack/Reboot/Style_Guide


>diff --git a/packages/jetpack-core/lib/es5.js b/packages/jetpack-core/lib/es5.js

>+/** ES5 15.2.3.11 */
>+if (!Object.isSealed) Object.isSealed = function (object) false
>+/** ES5 15.2.3.12 */
>+if (!Object.isFrozen) Object.isFrozen = function (object) false

I wonder if these should return "true" if Object.seal/Object.freeze has been called on the object, for consistency with the result in an ES5-capable browser.


>+/** ES5 15.2.3.6 */
>+if (!Object.defineProperty) Object.defineProperty = 
>+    function defineProperty(object, name, descriptor) {

I wonder if it would be possible to implement support for enumerable.  defineProperty would have to create a custom iterator (if one didn't already exist) that consulted a private list of non-enumerable properties (matched to the objects on which they are non-enumerable) to determine which ones to return during enumeration.  But how to avoid leaking objects given that the private list maintains references to them?

It's not important for this patch, especially since no existing module needs this functionality, just something to think about.


>+                    throw new Error('Can\'t assigt to non writable properties')

Nit: assigt -> assign


>+    // #Note can't modify length it in es3.
...
>+    // #Note can't delete prototype it in es3.

These are really useful notes (although the word "it" in them seems like a typo)!  It'd be great to have other such notes for other places in the code where we can't do something in ES3 (like seal and freeze)!


>diff --git a/packages/jetpack-core/lib/securable-module.js b/packages/jetpack-core/lib/securable-module.js

>+       var es5 = self.fs.getFile(self.fs.resolveModule(rootDir, 'es5'))

Since es5.js isn't actually a module, I wonder if it should be located somewhere else in the tree.  I can't think of a better place for it, though, so this is great for now.


>diff --git a/packages/jetpack-core/tests/test-es5.js b/packages/jetpack-core/tests/test-es5.js

>+"strict mode";

Is this intended to be |"use strict";|?


>+    test.assertEqual(false, Array.isArray(args), 'argumens is not an array')

Nit: argumens -> arguments

It might also be worth testing that |new Array()| is an array according to Array.isArray.


>+    } else test.assertEqual(true, Object.isExtensible(object), '`preventExtensions` can\'t be implemented there for isExtensible must returt `true`')

Nit: returt -> return (here and elsewhere)


>+        test.assertEqual(true, Object.getOwnPropertyDescriptor(object, 'number').writable, 'after calling sealing objects descriptors must not change')

Nit: calling sealing objects -> calling seal, object's


>+exports['test:freeze'] = function(test) {
>+    let object = { number: 5, string: 'test' }
>+    Object.freeze(object)
>+    if (isNative(Object.freeze)) {

Shouldn't this also test that the values of properties cannot be changed?

On the other hand, I wonder if we need these tests at all for the native implementations.  After all, presumably they'll have test coverage in a JavaScript engine test suite, right?


>+exports['test:getOwnPropertyNames'] = function(test) {
>+    let object = { number: 5, string: 'test', __proto__: { trap: 'oops!' } }
>+    test.assertEqual(Object.getOwnPropertyNames(object).toString(), ['number', 'string'].toString())

I don't think Spidermonkey defines an enumeration order, which means this test could result in a false negative (even if the engine currently uses the same order, and the test currently passes), this should sort both arrays before converting them to strings.


>+exports['test:keys'] = function(test) {
>+    let object = { number: 5, string: 'test', __proto__: { trap: 'oops!' } }
>+    test.assertEqual(Object.keys(object).toString(), ['number', 'string'].toString())
>+}

Same here.


>+exports['test:bind this'] = function(test) {
>+    let object = { test: function foo() this }
>+    object.test.prototype = { constructor: object.test, test: 'uoau'}
>+    let func = object.test.bind(object)
>+    test.assertEqual(object, func())
>+    test.assertEqual(object.test.prototype, (new func()).__proto__)
>+    test.assertEqual(object, func.boundTo)
>+    test.assertEqual(object.test, func.bound)
>+    test.assertEqual([object].toString(), func.boundArgs.toString())

Nit: these could use messages explaining what they are testing, as it isn't obvious at first glance.


>\ No newline at end of file

Nit: end files with newlines.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

One more thing!  On Firefox 3.6.*, all tests pass, but on the latest nightly build, I see four test failures:

error: TEST FAILED: test-es5.test:getOwnPropertyDescriptor value(writable,configurabe,enumerable) (failure)
error: fail: "override" != "test"
info: Traceback (most recent call last):
  File "resource://jetpack-core-jetpack-core-lib/timer.js", line 58, in notify
    this._callback.apply(null, []);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 257, in anonymous
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 282, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 300, in start
    this.test.testFunction(this);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 57, in runTest
    test(runner);
  File "resource://jetpack-core-jetpack-core-tests/test-es5.js", line 143, in anonymous
    test.assertEqual('override', object.test)
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 229, in assertEqual
    this.fail(message);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 147, in fail
    console.trace();
.error: fail: should throw when modifying non writable properties (true != false)
info: Traceback (most recent call last):
  File "resource://jetpack-core-jetpack-core-lib/timer.js", line 58, in notify
    this._callback.apply(null, []);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 257, in anonymous
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 282, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 300, in start
    this.test.testFunction(this);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 57, in runTest
    test(runner);
  File "resource://jetpack-core-jetpack-core-tests/test-es5.js", line 153, in anonymous
    test.assertEqual(true, threw, 'should throw when modifying non writable properties')
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 229, in assertEqual
    this.fail(message);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 147, in fail
    console.trace();
.......error: fail: should throw when modifying non writable properties (true != false)
info: Traceback (most recent call last):
  File "resource://jetpack-core-jetpack-core-lib/timer.js", line 58, in notify
    this._callback.apply(null, []);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 257, in anonymous
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 282, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 300, in start
    this.test.testFunction(this);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 57, in runTest
    test(runner);
  File "resource://jetpack-core-jetpack-core-tests/test-es5.js", line 190, in anonymous
    test.assertEqual(true, threw, 'should throw when modifying non writable properties')
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 229, in assertEqual
    this.fail(message);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 147, in fail
    console.trace();
.......error: fail: should throw when modifying non writable properties (true != false)
info: Traceback (most recent call last):
  File "resource://jetpack-core-jetpack-core-lib/timer.js", line 58, in notify
    this._callback.apply(null, []);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 257, in anonymous
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 282, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 300, in start
    this.test.testFunction(this);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 57, in runTest
    test(runner);
  File "resource://jetpack-core-jetpack-core-tests/test-es5.js", line 228, in anonymous
    test.assertEqual(true, threw, 'should throw when modifying non writable properties')
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 229, in assertEqual
    this.fail(message);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 147, in fail
    console.trace();

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Addressing review commets

http://github.com/Gozala/jetpack-sdk/commit/643518488694041706e49a3e4f2a1d4c8ee8a71a

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Hi Myk,

I believe I have addressed all the issues that you've mentioned. Please find my comments on some of them inline. 

(In reply to comment #6)
> Comment on attachment 459066 [details] [diff] [review]
> Added license block and updated few comments
> 
> Overall, this looks great!  Just a few minor issues, nothing major.
> 
> Some general notes:
> 
> * end statements with semi-colons (except for single statements in one-line
> blocks), which is the convention in Firefox and Jetpack code to avoid bugs that
> tend to crop up in code that doesn't use them;
> * use two-space indent for consistency with Firefox and Jetpack code.
> 
> Also note other good recommendations in the style guides:
> 
>   https://developer.mozilla.org/En/Developer_Guide/Coding_Style
>   https://wiki.mozilla.org/Labs/Jetpack/Reboot/Style_Guide
> 
> 
> >diff --git a/packages/jetpack-core/lib/es5.js b/packages/jetpack-core/lib/es5.js
> 
> >+/** ES5 15.2.3.11 */
> >+if (!Object.isSealed) Object.isSealed = function (object) false
> >+/** ES5 15.2.3.12 */
> >+if (!Object.isFrozen) Object.isFrozen = function (object) false
> 
> I wonder if these should return "true" if Object.seal/Object.freeze has been
> called on the object, for consistency with the result in an ES5-capable
> browser.
> 

I think it's a good idea and I have implemented it since work for non-enumerable properties was overlapping this.

> 
> >+/** ES5 15.2.3.6 */
> >+if (!Object.defineProperty) Object.defineProperty = 
> >+    function defineProperty(object, name, descriptor) {
> 
> I wonder if it would be possible to implement support for enumerable. 
> defineProperty would have to create a custom iterator (if one didn't already
> exist) that consulted a private list of non-enumerable properties (matched to
> the objects on which they are non-enumerable) to determine which ones to return
> during enumeration.  But how to avoid leaking objects given that the private
> list maintains references to them?
> 
> It's not important for this patch, especially since no existing module needs
> this functionality, just something to think about.
> 

I think it's brilliant idea. I have implemented this as well. In order to avoid leaks I don't store any objects, instead I generate unique id per object that has non-enumerable property or is sealed / frozen ... and store it in the object's non enumerable hidden property. On the module side I have a registry where I store state (frozen, sealed, inextensible) and descriptor property values.  

> 
> >+                    throw new Error('Can\'t assigt to non writable properties')
> 
> Nit: assigt -> assign
> 
> 
> >+    // #Note can't modify length it in es3.
> ...
> >+    // #Note can't delete prototype it in es3.
> 
> These are really useful notes (although the word "it" in them seems like a
> typo)!  It'd be great to have other such notes for other places in the code
> where we can't do something in ES3 (like seal and freeze)!
> 
> 
> >diff --git a/packages/jetpack-core/lib/securable-module.js b/packages/jetpack-core/lib/securable-module.js
> 
> >+       var es5 = self.fs.getFile(self.fs.resolveModule(rootDir, 'es5'))
> 
> Since es5.js isn't actually a module, I wonder if it should be located
> somewhere else in the tree.  I can't think of a better place for it, though, so
> this is great for now.
> 

Yeah I have been thinking about it as well but since loader will change soon, I think it's better to leave it for now. BTW I also changed the way module is loaded to avoid re-evaluation and to fix cross module functionality.

> 
> >diff --git a/packages/jetpack-core/tests/test-es5.js b/packages/jetpack-core/tests/test-es5.js
> 
> >+"strict mode";
> 
> Is this intended to be |"use strict";|?
> 
> 
> >+    test.assertEqual(false, Array.isArray(args), 'argumens is not an array')
> 
> Nit: argumens -> arguments
> 
> It might also be worth testing that |new Array()| is an array according to
> Array.isArray.
> 
> 
> >+    } else test.assertEqual(true, Object.isExtensible(object), '`preventExtensions` can\'t be implemented there for isExtensible must returt `true`')
> 
> Nit: returt -> return (here and elsewhere)
> 
> 
> >+        test.assertEqual(true, Object.getOwnPropertyDescriptor(object, 'number').writable, 'after calling sealing objects descriptors must not change')
> 
> Nit: calling sealing objects -> calling seal, object's
> 
> 
> >+exports['test:freeze'] = function(test) {
> >+    let object = { number: 5, string: 'test' }
> >+    Object.freeze(object)
> >+    if (isNative(Object.freeze)) {
> 
> Shouldn't this also test that the values of properties cannot be changed?
> 
> On the other hand, I wonder if we need these tests at all for the native
> implementations.  After all, presumably they'll have test coverage in a
> JavaScript engine test suite, right?
> 

I think it's valid point and I have removed all the tests that were not testing 
module itself.

> 
> >+exports['test:getOwnPropertyNames'] = function(test) {
> >+    let object = { number: 5, string: 'test', __proto__: { trap: 'oops!' } }
> >+    test.assertEqual(Object.getOwnPropertyNames(object).toString(), ['number', 'string'].toString())
> 
> I don't think Spidermonkey defines an enumeration order, which means this test
> could result in a false negative (even if the engine currently uses the same
> order, and the test currently passes), this should sort both arrays before
> converting them to strings.
> 
> 
> >+exports['test:keys'] = function(test) {
> >+    let object = { number: 5, string: 'test', __proto__: { trap: 'oops!' } }
> >+    test.assertEqual(Object.keys(object).toString(), ['number', 'string'].toString())
> >+}
> 
> Same here.
> 
> 
> >+exports['test:bind this'] = function(test) {
> >+    let object = { test: function foo() this }
> >+    object.test.prototype = { constructor: object.test, test: 'uoau'}
> >+    let func = object.test.bind(object)
> >+    test.assertEqual(object, func())
> >+    test.assertEqual(object.test.prototype, (new func()).__proto__)
> >+    test.assertEqual(object, func.boundTo)
> >+    test.assertEqual(object.test, func.bound)
> >+    test.assertEqual([object].toString(), func.boundArgs.toString())
> 
> Nit: these could use messages explaining what they are testing, as it isn't
> obvious at first glance.
> 
> 
> >\ No newline at end of file
> 
> Nit: end files with newlines.

I think I also fixed all the spelling errors, at least the ones mentioned and others that spell check was able to catch.

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

BTW I also checked with nightly and all the tests pass now, there is one failing on beta1 but that's because of the bug in native implementation of getOwnPropertyNames. In 3.6 all the tests pass as well

Comment 11

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #4)
> BTW whom can I get from what is planned from es5 for FF4?

I bumped into Rob Sayre today and asked him this question.  The JS team plans complete support for ES5 in Firefox 4, landing all of the necessary work by September 1.  The weekly meeting notes <https://wiki.mozilla.org/Platform#Meeting_Notes_.28template.29> include updates on the status of this work.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 460545 [details] [diff] [review]
Addressing review commets

Looks great!  Just a few minor issues, but overall this seems just about ready to land.  Note: I experimented with github line notes for this review, so my comments are all in http://github.com/Gozala/jetpack-sdk/commit/643518488694041706e49a3e4f2a1d4c8ee8a71a.

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Addressing review remarks

Inline comments indeed look nicer :)

I have replied on some of them.

Here is updated patch:
http://github.com/Gozala/jetpack-sdk/commit/2cb962e997d09bee767189ee5ddf5cc184622c5d

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 461516 [details] [diff] [review]
Addressing review remarks

Just a couple of minor nits, which I've noted in the github line notes.  Otherwise, this looks great, r=myk!

Comment 15

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Myk I commented on one of your notes in a previous review, but probably it was better to comment here:

As you mentioned & highlighted I use:
@param {Object} object
syntax instead of:
@param object {Object}

The reason for that is slight difference of jsdocs syntax in comparison to javadocs. Obviously there is no real standard on jsdocs, but syntax is well adopted across the common tools and is widely used on the web as well.

One of the widely used tools for documenting js are:
   
http://code.google.com/p/jsdoc-toolkit/wiki/TagReference
http://jsdoc.sourceforge.net/


I do believe it would be better to follow jsdoc style, but if you disagree with that I can update my patch.

Comment 16

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: updated patch (fixing typo in docs)

If I remember correctly, it will be you who will push the change to the repo. Please let me know what do you thing on my previous comment & let me know if I need to do anything else.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #15)
> I do believe it would be better to follow jsdoc style, but if you disagree with
> that I can update my patch.

Hmm, Firefox code typically uses javadoc style, but if jsdoc style is different, then I'm ok with using that style instead.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

I committed the latest patch on the bug, so this is now fixed by changeset https://hg.mozilla.org/labs/jetpack-sdk/rev/5abee1c9aa2e.

Comment 19

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".