Last Comment Bug 776760 - ecma_5/extensions/cross-global-getPrototypeOf.js always fails
: ecma_5/extensions/cross-global-getPrototypeOf.js always fails
Status: RESOLVED FIXED
[js:p1]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
: general
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-23 16:53 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-07-26 05:09 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (2.55 KB, patch)
2012-07-25 11:32 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-07-23 16:53:15 PDT
It reduces to this:

var g = newGlobal();
g.evaluate("var a = { a: 1 }; " +
           "var obj = Object.create(a);");
assertEq(Object.getPrototypeOf(g.obj), g.a);
g.evaluate("var b = { b: 1 }; " +
           "obj.__proto__ = b;");
assertEq(Object.getPrototypeOf(g.obj), g.b);

Weirdly, if you comment out the first assert, which ought not to have any side effects, the second one passes.

I'll flip Terrence for this one.

(Note that we *could* just add "fails" to the magic comment at the top of the file and wait for Waldo to get back.)
Comment 1 David Mandelin [:dmandelin] 2012-07-24 17:02:56 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #0)
> It reduces to this:
> 
> var g = newGlobal();
> g.evaluate("var a = { a: 1 }; " +
>            "var obj = Object.create(a);");
> assertEq(Object.getPrototypeOf(g.obj), g.a);
> g.evaluate("var b = { b: 1 }; " +
>            "obj.__proto__ = b;");
> assertEq(Object.getPrototypeOf(g.obj), g.b);
> 
> Weirdly, if you comment out the first assert, which ought not to have any
> side effects, the second one passes.
> 
> I'll flip Terrence for this one.
> 
> (Note that we *could* just add "fails" to the magic comment at the top of
> the file and wait for Waldo to get back.)

Do we know which patch caused it? We should just back out if possible.
Comment 2 Steve Fink [:sfink] [:s:] 2012-07-25 10:34:18 PDT
It comes from bug 770344, which is also where the test was added.
Comment 3 Luke Wagner [:luke] 2012-07-25 11:32:09 PDT
Created attachment 645832 [details] [diff] [review]
fix

It looks like the failing test was actually added by bug 770344.  The Math.sin() call right before the failing assertEq would seem to indicate that Waldo was working on this and the fix was somehow accidentally lost.  I actually remember talking to Waldo about this problem and I think this patch is the intended solution (at least for now; hopefully we can remove the scripted proxy handler disjunct after the direct proxy work).
Comment 4 Jason Orendorff [:jorendorff] 2012-07-25 13:55:52 PDT
Comment on attachment 645832 [details] [diff] [review]
fix

Review of attachment 645832 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable to me.

(It still doesn't make any sense to me for Object.prototype to call the __proto__ getter. Instead, both should call the same C++ function. But yeah, let's land this.)
Comment 5 Luke Wagner [:luke] 2012-07-25 14:13:02 PDT
Well, it would seem that my patch causes orange:
  https://tbpl.mozilla.org/?tree=Try&rev=137102959e2d

Since this bug existed before bug 770344 (filed as bug 764307), I think we should just remove the offending assert.
Comment 6 Luke Wagner [:luke] 2012-07-25 16:59:57 PDT
Removed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3238a5e6e0d8
Comment 7 Ed Morley [:emorley] 2012-07-26 05:09:09 PDT
https://hg.mozilla.org/mozilla-central/rev/3238a5e6e0d8

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