Last Comment Bug 787847 - Missing property IC needs to check proto chain for proxies
: Missing property IC needs to check proto chain for proxies
Status: RESOLVED FIXED
[js:t], [qa-]
: testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: Bill McCloskey (:billm)
: general
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-02 17:16 PDT by Bill McCloskey (:billm)
Modified: 2012-11-13 02:46 PST (History)
5 users (show)
anthony.s.hughes: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
fixed
fixed
unaffected


Attachments
testcase (367 bytes, patch)
2012-09-02 17:16 PDT, Bill McCloskey (:billm)
no flags Details | Diff | Review
patch (1.82 KB, patch)
2012-09-02 17:25 PDT, Bill McCloskey (:billm)
luke: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Review

Description Bill McCloskey (:billm) 2012-09-02 17:16:55 PDT
Created attachment 657721 [details] [diff] [review]
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 (:billm) 2012-09-02 17:25:17 PDT
Created attachment 657722 [details] [diff] [review]
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 [:bz] (Out June 25-July 6) 2012-09-02 18:48:04 PDT
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] 2012-09-04 11:20:45 PDT
Thanks!
Comment 5 Bill McCloskey (:billm) 2012-09-11 17:09:49 PDT
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 [:bz] (Out June 25-July 6) 2012-09-11 17:11:24 PDT
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 (:billm) 2012-09-11 17:26:45 PDT
(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 [:bz] (Out June 25-July 6) 2012-09-11 17:34:00 PDT
Ah, sounds good.  Thanks!
Comment 9 Ed Morley [:emorley] 2012-09-12 13:55:54 PDT
https://hg.mozilla.org/mozilla-central/rev/ef498cfb4fcd
Comment 10 Bill McCloskey (:billm) 2012-09-14 16:18:17 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/a6440cd0e612
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-19 10:36:41 PDT
Nominating this for in-testsuite with the attached testcase patch.
Comment 12 Manuela Muntean [Away] 2012-11-12 07:59:56 PST
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 [:bz] (Out June 25-July 6) 2012-11-12 09:25:11 PST
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] 2012-11-13 02:46:00 PST
Given that there is a unit test for this bug, I will be marking it as [qa-].

Note You need to log in before you can comment on or make changes to this bug.