Chai: Build breaking on Safari 10.0.1

[lucasfcosta]

Hey everyone, so we've got a failing build for Safari 10.0.1, as you can see here, for example.
I'll be able to solve this tomorrow, but if someone could solve this today it would be nice too.
I don't know exactly why this happens, but right now I think this affects our canary version too.

We've got problems in the following tests:

• should extensible
• should sealed
• should frozen

[shvaikalesh]

Hey @lucasfcosta, thanks for the log. Fails happen on tests with proxies I've added in #823. However, I can't reproduce it with

  var proxy = new Proxy({}, {
    ownKeys: function() {
      throw new TypeError();
    }
  });

  Object.preventExtensions(proxy);
  Object.isSealed(proxy);

(I have macOS 10.11.6 and Safari 10.0.1, just like Sauce Labs)

Looks like a bug in JSC's JIT or something...

[lucasfcosta]

@shvaikalesh I just tried to run all tests here too and even on Safari 9.1.2 they all pass.
So, what should we do about it?

[shvaikalesh]

They pass on Safari 9 cause it does not have proxies. We should try to reproduce it on local machine and report to WebKit guys.

[lucasfcosta]

I have just tried it here with Safari's latest version and I was able to reproduce these problems. However, I have no idea why this is happening. I made some tests and this is what I found out:

First of all, I tried to add a try/catch surroundinggetter.callon theaddPropertyutility method and my catch really wasn't called, indicating no error was thrown on theextensible assertion.

I also tried to call Object.isExtensible(obj) inside the extensible assertion and surround it with a try/catch, but here too no error was thrown.

Finally I tried to add an Object.isExtensible(this) to the shouldGetter inside the should interface and also added a console.log to see if the isExtensible handler would be called, but it wasn't.

---

So, finally, after 3 hours of hard debugging, I think I've found a bug in Safari.

When adding a property to the Object.prototype that returns this when it's accessed, undefined gets returned.

You can check this by running the following code at Safari:

var proxy = new Proxy({}, {
  get: function(target, prop) {
    console.log('Tried to access: ' + prop);
  }
});

Object.defineProperty(Object.prototype, 'bla', {
  get: function() {
    return this;
  }
});

console.log(proxy.bla.test);

In the code above, Tried to access: test should have been logged to the console but it isn't.

Am I missing something here? So, what are we going to do?

[meeber]

I don't think the code sample you provided will work in any environment. The proxy trap intercepts the initial proxy.bla call and then doesn't return anything so it returns undefined implicitly.

[lucasfcosta]

@meeber You're actually right.
So I'm back to mark 0.
😓

[lucasfcosta]

Hello everyone,
So, I'm totally lost at this, I've been debugging this the whole afternoon and I can't understand why assertion's object has no proxy handlers.

I also tried this:

try {
  Object.isExtensible(proxy.should.be.__flags['object']);
} catch (e) {
  console.log('An error was thrown, so the proxy was called');
}

And had no success.
What I'm a 100% sure is that the assertion's object is a proxy in NodeJS, but in Safari it's just that same object without the proxy wrapper.
Any ideas on what it might be?

[meeber]

@lucasfcosta I'm not sure. I don't have access to a macbook so I'm hamstrung.

Do you know if the issue is specific to properties added onto Object.prototype, or does the same issue occur for SomeCustomObject.prototype?

[lucasfcosta]

@meeber I tested adding properties to the Object.prototype in safari and them accessing properties from a this object wrapped in a proxy inside this property's getter and everything still worked fine, so I have no idea about what Safari's implementation does different from the Node one.

[lucasfcosta]

Hello everyone, so, any news on this?
I've been playing with this for quite some time and I still can't figure what's happening on Safari.
I'll be very busy in the next two weeks due to the end of semester in college but I'll try to tackle this again as soon as I have time, if anyone has any idea about what's wrong, please let me know.

[meeber]

@lucasfcosta Borrowed a macbook. Here's the issue: In most environments, if you create a proxy of an object that has a getter defined via Object.defineProperty, then inside of that getter, this refers to the proxy. But in Safari, this refers to the original object, not the proxy.

I guess the next step is to study the spec and find out who is doing the right thing: Safari or non-Safari. It's certainly possible that Safari is the only one meeting the spec.

Note that the impact of this on Chai is that assertions cannot be performed on proxy objects in Safari using should; it'll always default to the non-proxified version of the object. The fact that these three tests are failing is simply because these are the only three tests in our suite for should that perform assertions directly on a proxy (and relies on the proxy traps to pass the test).

Here's a stand-alone example that doesn't involve Chai or Object.prototype:

"use strict";                                                                   

const cat = {                                                                   
  whiskers: 5,                                                                  
};                                                                              

Object.defineProperty(cat, "meow", {                                            
  get: function () {                                                            
    console.log("What is this?");                                               
    console.log(this);                                                          
    console.log("Is this extensible?");                                         
    console.log(Object.isExtensible(this));                                     
  },                                                                            
});                                                                             

const proxyCat = new Proxy(cat, {                                               
  isExtensible: function () {                                                   
    console.log("Extensibility checked!");                                      
    return true;                                                                
  }                                                                             
});                                                                             

console.log("Begin cat test");                                                  
cat.meow;                                                                       
console.log("End cat test");                                                    

console.log("Begin proxy test");                                                
proxyCat.meow;                                                                  
console.log("End proxy test");

Output in most environments:

Begin cat test
What is this?
{ whiskers: 5 }
Is this extensible?
true
End cat test
Begin proxy test
What is this?
{ whiskers: 5 }
Is this extensible?
Extensibility checked!
true
End proxy test

Output in Safari (proxy trap never triggered):

Begin cat test
What is this?
{ whiskers: 5 }
Is this extensible?
true
End cat test
Begin proxy test
What is this?
{ whiskers: 5 }
Is this extensible?
true
End proxy test

[shvaikalesh]

Awesome job, @meeber.

proxyCat.meow should do (see GetValue) proxyCat.[[Get]]("meow", proxyCat), [[Get]] should "forward" to cat.[[Get]]("meow", proxyCat) (see [[Get]] in 9.5; because proxy handler has no get trap) that should execute cat.[[GetOwnProperty]]("meow").[[Get]].[[Call]](proxyCat). Safari does it wrong.

@meeber, would you mind if I simplify the example even further and proceed with reporting it to JSC guys and maybe add test to test-262?

[keithamus]

In the meantime - that means to fix this, we could devise a test to ensure Proxies are well behaved by checking the this of a created Proxy, and if it fails the test, disable Proxy support for that environment right?

[meeber]

@shvaikalesh Sounds great, please do!

@keithamus No because this issue doesn't really impact Chai's new fancy proxy features. Instead, it affects when an end user performs an assertion on one of their own proxy objects using should syntax.

[keithamus]

Okay that makes sense. So the actionable here is to fix the tests to not assert on proxies?

[meeber]

Yup, @shvaikalesh is opening a bug report, and in the meantime I think we should comment out the three offending tests, as they specifically test proxy objects with the should syntax.

[shvaikalesh]

The bug in WebKit was resolved, the test should pass in WebKit nightly.