Closed
Bug 628333
Opened 14 years ago
Closed 14 years ago
Object.getOwnPropertyNames() does not returns non-enumerable names if given object is from other compartment
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: irakli, Assigned: gal)
References
Details
(Whiteboard: hardblocker, fixed-in-tracemonkey)
Attachments
(2 files, 1 obsolete file)
857 bytes,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
2.69 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•14 years ago
|
Version: unspecified → Trunk
Assignee | ||
Updated•14 years ago
|
Assignee: general → gal
blocking2.0: --- → betaN+
Assignee | ||
Comment 1•14 years ago
|
||
myk will further reduce the test case, this seems to happen between chrome and chrome as well
Comment 2•14 years ago
|
||
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)?
Updated•14 years ago
|
Whiteboard: hardblocker
Comment 3•14 years ago
|
||
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.
Reporter | ||
Comment 4•14 years ago
|
||
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+ → ---
Comment 5•14 years ago
|
||
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: --- → ?
Updated•14 years ago
|
blocking2.0: ? → betaN+
Assignee | ||
Comment 6•14 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•14 years ago
|
||
Comment 3 seems to work, so I will work with that until something smaller is provided.
Comment 8•14 years ago
|
||
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•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #506566 -
Flags: review?(jwalden+bmo)
Comment 10•14 years ago
|
||
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•14 years ago
|
||
Myk promised to make a test. http://hg.mozilla.org/tracemonkey/rev/72cb2f4a893c
Whiteboard: hardblocker → hardblocker, fixed-in-tracemonkey
Comment 12•14 years ago
|
||
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 13•14 years ago
|
||
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•14 years ago
|
||
The layering was wrong. Good thing we have Waldo and his pedantic test writing habit =)
Attachment #506566 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #506654 -
Flags: review?(jwalden+bmo)
Comment 15•14 years ago
|
||
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+
Comment 16•14 years ago
|
||
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.
Assignee | ||
Comment 17•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/fa0ee8f877e8
Comment 18•14 years ago
|
||
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/fa0ee8f877e8
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•