Missing property IC needs to check proto chain for proxies

RESOLVED FIXED in Firefox 17



6 years ago
6 years ago


(Reporter: billm, Assigned: billm)



Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox15 unaffected, firefox16 unaffected, firefox17 fixed, firefox18 fixed, firefox-esr10 unaffected)


(Whiteboard: [js:t], [qa-])


(2 attachments)

Created attachment 657721 [details] [diff] [review]

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.
Created attachment 657722 [details] [diff] [review]

Actually, maybe it's not that hard (assuming we don't expect proxies to be on the proto chain in the common case).
Attachment #657722 - Flags: review?(luke)
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?


6 years ago
Attachment #657722 - Flags: review?(luke) → review+

Comment 3

6 years ago
Whiteboard: [js:t]
Comment on attachment 657722 [details] [diff] [review]

[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.
Attachment #657722 - Flags: approval-mozilla-aurora?
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...
(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.
Ah, sounds good.  Thanks!

Comment 9

6 years ago
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18


6 years ago
Attachment #657722 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox-esr10: --- → unaffected
status-firefox15: --- → unaffected
status-firefox16: --- → unaffected
status-firefox17: --- → fixed
status-firefox18: --- → fixed
Nominating this for in-testsuite with the attached testcase patch.
Flags: in-testsuite?
Keywords: verifyme
Keywords: testcase
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.
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.
Given that there is a unit test for this bug, I will be marking it as [qa-].
Keywords: verifyme
Whiteboard: [js:t] → [js:t], [qa-]
You need to log in before you can comment on or make changes to this bug.