Build breaking on Safari 10.0.1 #855

Open
lucasfcosta opened this Issue Oct 31, 2016 · 16 comments

Projects

None yet

4 participants

@lucasfcosta
Member
lucasfcosta commented Oct 31, 2016 edited

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
@lucasfcosta lucasfcosta added the bug label Oct 31, 2016
@shvaikalesh
Member
shvaikalesh commented Oct 31, 2016 edited

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
Member

@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
Member

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
Member
lucasfcosta commented Nov 2, 2016 edited

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
Contributor
meeber commented Nov 2, 2016

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
Member

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

@lucasfcosta
Member
lucasfcosta commented Nov 2, 2016 edited

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
Contributor
meeber commented Nov 3, 2016

@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
Member

@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
Member

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
Contributor
meeber commented Nov 16, 2016 edited

@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
Member
shvaikalesh commented Nov 16, 2016 edited

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
Member

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
Contributor
meeber commented Nov 16, 2016

@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
Member

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

@meeber
Contributor
meeber commented Nov 16, 2016

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.

@meeber meeber added a commit to meeber/chai that referenced this issue Nov 16, 2016
@meeber meeber test: comment out broken tests in Safari 10
Three tests on the `should` interface are failing in Safari 10 due to a
bug related to proxies. These tests should be re-enabled once the bug is
fixed. See chaijs#855.
80d2c70
@lucasfcosta lucasfcosta added a commit that referenced this issue Nov 17, 2016
@meeber @lucasfcosta meeber + lucasfcosta test: comment out broken tests in Safari 10 (#869)
Three tests on the `should` interface are failing in Safari 10 due to a
bug related to proxies. These tests should be re-enabled once the bug is
fixed. See #855.
8a2ef67
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment