Care about fixing all the other getters as well? Shouldn't be too much work...
On Wed, Jul 23, 2014 at 5:10 PM, Toon Verwaest <[email protected]> wrote: > Ok, great. Thanks for the report ;) > > > On Wed, Jul 23, 2014 at 4:53 PM, 'Erik Arvidsson' via v8-dev < > [email protected]> wrote: > >> > I can't do it yet, since I'm waiting for dom accessors to be turned >> into real JS accessors >> >> Get in line ;-) >> >> I made the change to ArrayLengthGetter and all tests still pass. However, >> the ArrayLengthSetter has some issues and I ran into a new bug (3460). >> >> >> On Wed, Jul 23, 2014 at 10:27 AM, Toon Verwaest <[email protected]> >> wrote: >> >>> Yeah I think so. >>> >>> I'm actually kinda surprised that the getters are implemented in this >>> way at this point. I'd update all those methods to use the Holder() they >>> get from the info. In GetPropertyWithAccessor you can see that the holder >>> passed to the PropertyCallbackArguments is the holder of the accessors. I'm >>> pretty sure that's the object from which you want to actually fetch the >>> value. That should fix the get semantics at least. >>> >>> My plan is to, in the future, make ExecutableAccessInfo behave entirely >>> like a data property which just so happens to be implemented using >>> accessors internally. That will affect the store behavior. I can't do it >>> yet, since I'm waiting for dom accessors to be turned into real JS >>> accessors; until then it would break things like window.all. That will fix >>> the ExecutableAccessorInfo setter problem you are having with unscopable. >>> >>> hth, >>> Toon >>> >>> >>> On Wed, Jul 23, 2014 at 4:12 PM, Erik Arvidsson <[email protected]> >>> wrote: >>> >>>> >>>> >>>> >>>> On Wed, Jul 23, 2014 at 8:08 AM, <[email protected]> wrote: >>>> >>>>> My comment below is based on a short explanation of the problem by >>>>> rossberg. Let >>>>> me know if some things are still unclear. >>>>> >>>>> Overall: Please don't work around "bugs" (wasn't really a bug until >>>>> unscopable >>>>> since it wasn't observable in any way) in one part of the system by >>>>> obfuscating >>>>> another part. >>>>> >>>>> >>>>> https://codereview.chromium.org/384963002/diff/120001/src/objects.cc >>>>> File src/objects.cc (right): >>>>> >>>>> https://codereview.chromium.org/384963002/diff/120001/src/ >>>>> objects.cc#newcode165 >>>>> src/objects.cc:165: } >>>>> Euh, no. >>>>> >>>>> What you want to do is fix a bug in Array.length; but what you do >>>>> instead is modify get-property to change the semantics of all API >>>>> accessors... >>>>> >>>>> The right fix is to change ArrayLengthGetter to not use This() (via >>>>> GetThisFrom), but rather read the value from the Holder(). That also >>>>> removes the need for FindInstanceOf. >>>> >>>> >>>> I'm happy to hear that you do not think this work around is the right >>>> approach. >>>> >>>> I think I might still be a bit confused. >>>> >>>> I was under the impression that this was not a one off bug but a >>>> conceptual problem with API accessors. If that is not the case I'm happy to >>>> comment out the failing tests and then fix the one off bugs >>>> (ArrayLengthGetter and others). >>>> >>>> I thought the issue was that `holder[[Get]](name, receiver)` for an API >>>> accessor always get the actual value from the receiver? Looking at >>>> accessors.cc, it seems that almost all of these callbacks use `info.This()` >>>> so all of them should be updated? >>>> >>>> >>>> >>>> >>>> >>>>> >>>>> >>>>> https://codereview.chromium.org/384963002/ >>>>> >>>>> -- >>>>> -- >>>>> v8-dev mailing list >>>>> [email protected] >>>>> http://groups.google.com/group/v8-dev >>>>> ---You received this message because you are subscribed to the Google >>>>> Groups "v8-dev" group. >>>>> To unsubscribe from this group and stop receiving emails from it, send >>>>> an email to [email protected]. >>>>> For more options, visit https://groups.google.com/d/optout. >>>>> >>>> >>>> >>>> >>>> -- >>>> erik >>>> >>>> >>>> -- >>>> -- >>>> v8-dev mailing list >>>> [email protected] >>>> http://groups.google.com/group/v8-dev >>>> --- >>>> You received this message because you are subscribed to the Google >>>> Groups "v8-dev" group. >>>> To unsubscribe from this group and stop receiving emails from it, send >>>> an email to [email protected]. >>>> For more options, visit https://groups.google.com/d/optout. >>>> >>> >>> -- >>> -- >>> v8-dev mailing list >>> [email protected] >>> http://groups.google.com/group/v8-dev >>> --- >>> You received this message because you are subscribed to the Google >>> Groups "v8-dev" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to [email protected]. >>> For more options, visit https://groups.google.com/d/optout. >>> >> >> >> >> -- >> erik >> >> >> -- >> -- >> v8-dev mailing list >> [email protected] >> http://groups.google.com/group/v8-dev >> --- >> You received this message because you are subscribed to the Google Groups >> "v8-dev" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected]. >> For more options, visit https://groups.google.com/d/optout. >> > > -- -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
