787847 - Missing property IC needs to check proto chain for proxies

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: testcase

As far as I can tell, the missing property IC code doesn't check if there's a proxy on the prototype chain. This means that there could be a scripted proxy that claims not to have a property, but then later claims to have it. The methodjit will mistakenly act as if the property is always absent.

The attached testcase works with no command line options but fails with -m -n -a.

Just noticed this will working on the dynamic proto stuff. I'll work on a patch tomorrow.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

Actually, maybe it's not that hard (assuming we don't expect proxies to be on the proto chain in the common case).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that the global in the DOM will end up with a proxy on its proto chain at some point as we implement WebIDL.  Will that cause unacceptable performance problems?  Or is the missing property thing rare for the global anyway?

Comment 3

[Luke Wagner [:luke]]

Thanks!

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef498cfb4fcd

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 657722 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 777630
User impact if declined: Incorrect JavaScript behavior when using scripted proxies.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low. It simply disables an optimization.
String or UUID changes made by this patch: None.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Luke, Bill, I'd really like to find out what the state of comment 2 is.  If I need to change implementation plans for the Window object, it would be good to know while still planning...

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> Luke, Bill, I'd really like to find out what the state of comment 2 is.  If
> I need to change implementation plans for the Window object, it would be
> good to know while still planning...

It seems sort of unlikely to me that we'll see real-world situations where there are a lot of property accesses to properties that don't exist for objects with proxies on the proto chain. However, it is possible. If that happens, we can add a special case for your special kind of proxy so that it can still use the missing prop IC. Either way, I don't think you have to worry.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, sounds good.  Thanks!

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ef498cfb4fcd

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6440cd0e612

Comment 11

[u279076]

Nominating this for in-testsuite with the attached testcase patch.

Comment 12

[Manuela Muntean [Away]]

I couldn't reproduce this neither on Win 7 64-bit, nor on Ubuntu 12.04 32-bit.

I used for this the builds from:

ftp://ftp.mozilla.org/pub/firefox/nightly/2012/09/2012-09-02-mozilla-central-debug/  (jsshell-linux-i686.zip  for Ubuntu and jsshell-win32.zip for Windows), but I received errors in both cases. 

Reporter, could you please give me more details on how should I procede in order to reproduce this bug? 

A changeset from when the bug is reproducible would be very useful.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Manuela, the attached testcase is a JS engine unit test.  You either want to run it as part of our test suite or modify it so it's not using things like assertEq and whatnot.

Comment 14

[Manuela Muntean [Away]]

Given that there is a unit test for this bug, I will be marking it as [qa-].