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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: irakli, Assigned: gal)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: hardblocker, fixed-in-tracemonkey)

Attachments

(2 attachments, 1 obsolete attachment)

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.
Version: unspecified → Trunk
(Assignee)

Updated

7 years ago
Assignee: general → gal
blocking2.0: --- → betaN+
(Assignee)

Comment 1

7 years ago
myk will further reduce the test case, this seems to happen between chrome and chrome as well
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)?
Whiteboard: hardblocker
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.
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.
blocking2.0: betaN+ → ---
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.
No longer blocks: 628112
blocking2.0: --- → ?
blocking2.0: ? → betaN+
(Assignee)

Comment 6

7 years ago
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.
(Assignee)

Comment 7

7 years ago
Comment 3 seems to work, so I will work with that until something smaller is provided.
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.
(Assignee)

Comment 9

7 years ago
Created attachment 506566 [details] [diff] [review]
patch
(Assignee)

Updated

7 years ago
Attachment #506566 - Flags: review?(jwalden+bmo)
Comment on attachment 506566 [details] [diff] [review]
patch

Automated testcase, please.  (Hm, speaking of which, I need to write one for a bug myself...)
Attachment #506566 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 11

7 years ago
Myk promised to make a test.

http://hg.mozilla.org/tracemonkey/rev/72cb2f4a893c
Whiteboard: hardblocker → hardblocker, fixed-in-tracemonkey
Created attachment 506588 [details] [diff] [review]
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?
Attachment #506588 - Flags: review?(jwalden+bmo)
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.
Attachment #506588 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 14

7 years ago
Created attachment 506654 [details] [diff] [review]
patch

The layering was wrong. Good thing we have Waldo and his pedantic test writing habit =)
Attachment #506566 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #506654 - Flags: review?(jwalden+bmo)
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.  ;-)
Attachment #506654 - Flags: review?(jwalden+bmo) → review+
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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Depends on: 638735
You need to log in before you can comment on or make changes to this bug.