628333 - Object.getOwnPropertyNames() does not returns non-enumerable names if given object is from other compartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

This breaks some tests on jetpack-sdk: https://bugzilla.mozilla.org/show_bug.cgi?id=628112

Here is simplified snippet to reproduce a bug:

var desc = { foo: { value: "bar" } };
var object = Components.utils.evalInSandbox('Object.create({}, desc);',
Components.utils.Sandbox('http://mozilla.com'));
Object.getOwnPropertyNames(object).length ===
Object.getOwnPropertyNames(Object.create({}, desc))

P.S. Origin of `desc` is irrelevant it just there just to illustrate misbehavior.

Comment 1

[Andreas Gal :gal]

myk will further reduce the test case, this seems to happen between chrome and chrome as well

Comment 2

[Myk Melez [:myk] [@mykmelez]]

The change that broke the test in bug 628112 happened between the nightly builds on January 20 and 21, but there's a problem with the testcase in comment 0, which throws the same exception "Error: uncaught exception: ReferenceError: desc is not defined" on both builds when evaluated in the error console.

Irakli: can you provide a bit more information about the test case and how it can be used to demonstrate the bug?  Also, is it essential that the sandbox have a content principal (http://mozilla.com), or does the bug happen even if it has a chrome principal (like the system principal)?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Defining desc in the sandbox solves the problem I was encountering in comment 2. Here's a simplified testcase:

var principal = Components.classes["@mozilla.org/systemprincipal;1"].createInstance(Components.interfaces.nsIPrincipal);
var sandbox = Components.utils.Sandbox(principal);
var desc = { foo: { value: "bar" } };
sandbox.desc = desc;
var object = Components.utils.evalInSandbox('Object.create({}, desc);', sandbox);
Object.getOwnPropertyNames(object); // doesn't return any property names

However, this fails in older builds as well as the latest ones.  So it isn't clear how this is the cause of the test failure in bug 628112, which started with the January 21 nightly build.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Desc could be even used inline

Components.utils.evalInSandbox('Object.create({}, { foo: { value: "bar" } }', Components.utils.Sandbox(principal))

result should be ['foo'], but it's not.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

This is definitely a regression, and it seems due to compartments, as the testcase in comment 3 returns "foo" in 4.0b6 but nothing in 4.0b7 and newer builds.

Nevertheless, it doesn't look like the cause of the test failure in bug 628112, as that test only started failing with the January 21 nightly build.  Thus removing that bug dependency.

But re-requesting blocking2.0: betaN+, which was removed inadvertently.

Comment 6

[Andreas Gal :gal]

Please post complete, working code snippets. Comment #4 is missing the definition for principal and is also missing a paren ). We are going in circles until we have a clear and working test case.

Comment 7

[Andreas Gal :gal]

Comment 3 seems to work, so I will work with that until something smaller is provided.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Andreas: here's a slightly simpler version of the testcase in comment 3:

var principal =
Components.classes["@mozilla.org/systemprincipal;1"].createInstance(Components.interfaces.nsIPrincipal);
var sandbox = Components.utils.Sandbox(principal);
var object = Components.utils.evalInSandbox("Object.create({}, { foo: { value: 'bar' } });", sandbox);
Object.getOwnPropertyNames(object); // should return "foo"

When evaluated in the error console in 4.0b6, it displays "foo".  In 4.0b7 and newer builds, it doesn't display anything.

Comment 9

[Andreas Gal :gal]

Attachment: patch

Comment 10

[Jeff Walden [:Waldo]]

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

Automated testcase, please.  (Hm, speaking of which, I need to write one for a bug myself...)

Comment 11

[Andreas Gal :gal]

Myk promised to make a test.

http://hg.mozilla.org/tracemonkey/rev/72cb2f4a893c

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: testcase

Here's that testcase.  It fails without Andreas' patch and works with it.  I didn't know where to put it, so I slung it into js/src/xpconnect/tests/unit.  Is there a better place?

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 506588 [details] [diff] [review]
testcase

That works as well as anything, although I would prefer if, rather than pulling in all of XPConnect for a JS-only bug, we just wrote a testcase based on proxies and dumped it in js1_8_5/extensions/.  If that's not too much problem for you, I'd prefer that.  But this is probably (famous last words) not going to break without something in the JS test suite breaking, so whatever.

Comment 14

[Andreas Gal :gal]

Attachment: patch

The layering was wrong. Good thing we have Waldo and his pedantic test writing habit =)

Comment 15

[Jeff Walden [:Waldo]]

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

I don't think pedantry had much to do with this, just dumb luck that my mistake in writing that old test, plus the existing mistake in the code (I think I didn't write that originally), canceled out.  :-)  Although I guess including tests counts somewhat as pedantry, if you define the term down far enough.  ;-)

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/72cb2f4a893c
http://hg.mozilla.org/mozilla-central/rev/6f9c6b316b35 (backout)
Note: not marking as fixed because last changeset is a backout.

Comment 17

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/fa0ee8f877e8

Comment 18

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/fa0ee8f877e8