Bug 33450 – property access (. operator) incorrectly returning prototype value not instance value

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: JavaScriptCore

[P T Withington]

Created attachment 46233 [details]
.zip file of application demonstrating bug in Safari Version 4.0.4 (6531.21.10)

We have a large/complex Javascript system that appears to be triggering a (possibly storage or timing-dependent) error in the Javascript engine.  The symptoms are that an object property is being accessed using the `.` operator and the value being returned is _not_ the value in the object, but instead the value in the object's __proto__.  If we replace the failing case by rewriting `x.prop` to `x['prop']`, the code works as expected.  We have not been able to create a small test case that exhibits this error.  We do see this error appear in a number of applications generated by our system, but the error is not always in the same location, and a test case that is failing in the current Safari release Version 4.0.4 (6531.21.10), may not fail in the Webkit Nightly build.

I have attached the smallest example that I can make as a .zip file.  If you unpack the .zip file and open the .html within, it will halt on a null dereference in the current Safari release.  It works correctly in Firefox.

The erroneous evaluation occurs in the file lps/includes/lfc/LFCdhtml.js on line 667.  If the expression `$2.defaultplacement` is rewritten as `$2['defaultplacement']`, then the application behaves correctly.

This same test application does _not_ fail in the current Webkit nightly r52978, but we still see failures of a similar nature with other programs in the nightly build, so I do not believe that this bug has been fixed, just that it is an intermittent bug that is triggered by particular storage or timing patterns.  I can try to create an additional test that demonstrates the failure on the nightly build if that will help.

[P T Withington]

This bug is being tracked in the OpenLaszlo bug database as http://jira.openlaszlo.org/jira/browse/LPP-8626

[Oliver Hunt]

What are the other cases where it fails?  We know the bug you're explicitly calling out, and have fixed it already, and to our knowledge there shouldn't be any other cases where it can occur.  Could you please point to a case where you're getting a prototype lookup instead of the correct object?

[P T Withington]

Maciej asked:

> Do you know if this is a regression from a previous Safari version? 

Yes, this is a regression.  The attached id=46233 test case does not fail in Safari 4.0.2

> Is there any change you could make a more minimal failing testcase? 657.71 KB is a lot to go through.

Not yet, unfortunately.

> Have you tested whether it still happens with WebKit nightlies? It's possible we have fixed it already.

I have verified that id=46233 test case works correctly in nightly build Version 4.0.4 (6531.21.10, r53036).

Am checking if our other test cases (which are all larger) are also fixed by this most recent nightly.

[P T Withington]

Oliver:

I have now verified that our other two large test cases are working correctly in the webkit nightly Version 4.0.4 (6531.21.10, r53036).

It appears the bug is indeed fixed in the cases we were tracking down.

---

We are currently working around the bug in the released version of Safari by rewriting all our property accesses to use `[]`.  Is there any way to know from lexical analysis which `.` operations are likely to fail so we do not have to do this transformation globally?

[Oliver Hunt]

(In reply to comment #4)
> Oliver:
> 
> I have now verified that our other two large test cases are working correctly
> in the webkit nightly Version 4.0.4 (6531.21.10, r53036).
> 
> It appears the bug is indeed fixed in the cases we were tracking down.
> 
> ---
> 
> We are currently working around the bug in the released version of Safari by
> rewriting all our property accesses to use `[]`.  Is there any way to know from
> lexical analysis which `.` operations are likely to fail so we do not have to
> do this transformation globally?

Alas no.  The bug is due to us incorrectly caching lookup to the prototype object over an object that we consider a dictionary.

*** This bug has been marked as a duplicate of bug 32402 ***

[P T Withington]

(In reply to comment #5)
> (In reply to comment #4)
> > Is there any way to know from
> > lexical analysis which `.` operations are likely to fail so we do not have to
> > do this transformation globally?
> 
> Alas no.  The bug is due to us incorrectly caching lookup to the prototype
> object over an object that we consider a dictionary.

Rats.  Guess we have to stick with our work-around until the next Safari release, and hope people update.

[Oliver Hunt]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Is there any way to know from
> > > lexical analysis which `.` operations are likely to fail so we do not have to
> > > do this transformation globally?
> > 
> > Alas no.  The bug is due to us incorrectly caching lookup to the prototype
> > object over an object that we consider a dictionary.
> 
> Rats.  Guess we have to stick with our work-around until the next Safari
> release, and hope people update.

Yeah, sorry about that, it's a fairly egregious bug :-(


Hmmm, you maybe able to avoid it if you can afford to introduce an additional object into the prototype chain. IIRC this bug is specific to direct prototype look ups, eg.
function f(o) { return o.b; }
o = {__proto__:{b:5}}
f(o);

eg.
o = {__proto__:{__proto__:{b:5}}}
f(o)

Would not be able to hit it (i'm skipping the steps required to induce the bug, just trying to give the object/prototype structure)

[P T Withington]

(In reply to comment #7)
> Hmmm, you maybe able to avoid it if you can afford to introduce an additional
> object into the prototype chain.

I'll give that I try, because that would be something we could do with a runtime quirk that would only need to be invoked for the broken release, rather than burdening all JS engines.

I'm sure you're not able to answer this, but any estimate when this fix will hit the streets?

Thanks for your help.

[P T Withington]

(In reply to comment #7)
> IIRC this bug is specific to direct prototype
> look ups

Your suggested workaround doesn't seem to work for me.  I rejiggered our class substrate to insert an extra "empty" object in the __proto__ chain between each instance and its constructor's prototype (where the class default values reside), but that did not change the behavior.

The regression test added to LayoutTests/fast/js/script-tests/dictionary-prototype-caching.js for Bug 32402 is testing a 3-deep __proto__ chain, so it seems the cache error can't be worked around by just introducing additional protos.  Bummer.

[Oliver Hunt]

(In reply to comment #9)
> (In reply to comment #7)
> > IIRC this bug is specific to direct prototype
> > look ups
> 
> Your suggested workaround doesn't seem to work for me.  I rejiggered our class
> substrate to insert an extra "empty" object in the __proto__ chain between each
> instance and its constructor's prototype (where the class default values
> reside), but that did not change the behavior.
> 
> The regression test added to
> LayoutTests/fast/js/script-tests/dictionary-prototype-caching.js for Bug 32402
> is testing a 3-deep __proto__ chain, so it seems the cache error can't be
> worked around by just introducing additional protos.  Bummer.

Ah dammit, i was just adding assertions to the direct prototype lookup, the fix was for multiple levels.  Sorry :-(

[P T Withington]

One other work-around I am trying:  instead of installing the default values in the constructor prototype, I will have the constructor install the default values directly in each instance.  A little inefficient, but this seems to work.

Any reason that shouldn't be sufficient?

At least that way, I can restrict the work-around to the broken runtime, rather than inflicting it on every runtime.

[Oliver Hunt]

(In reply to comment #11)
> One other work-around I am trying:  instead of installing the default values in
> the constructor prototype, I will have the constructor install the default
> values directly in each instance.  A little inefficient, but this seems to
> work.
> 
> Any reason that shouldn't be sufficient?
> 
> At least that way, I can restrict the work-around to the broken runtime, rather
> than inflicting it on every runtime.

That should be fine