Enforce __exposedProps__ for Sandboxes

RESOLVED FIXED in mozilla22

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

unspecified
mozilla22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

In bug 784233, we relaxed the mandatory __exposedProps__ enforcement on Sandboxes to make things easier on jetpack. Jetpack should now be in good shape here, so it's time to remove that hack.
Created attachment 729231 [details] [diff] [review]
Enforce __exposedProps__ for Sandboxes. v1
Attachment #729231 - Flags: review?(gkrizsanits)
https://tbpl.mozilla.org/?tree=Try&rev=21d57f5f238f
Comment on attachment 729231 [details] [diff] [review]
Enforce __exposedProps__ for Sandboxes. v1

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

Looks good, not much to add just a few questions:

-        Math.sin("foo" in writable);

Was this only a debugger helper line that got into the repo somehow? Or did this line ever have some purpose?

+++ b/js/xpconnect/tests/unit/test_bug854558.js

More test is always good, but why do we need this? Don't the test you added to test_cows + the "Test readable property" part in the same file cover these cases already? Or you just wanted to have some tests without the voodoo in getCOW? 

Finally this is unrelated just for my education, I dug into this test a little and was wondering why are the Object.hasOwnProperty.call related parts commented out in isProp / isPropHidden? Shouldn't those tests just work?
Attachment #729231 - Flags: review?(gkrizsanits) → review+
(In reply to Gabor Krizsanits [:krizsa :gabor] (out until March 25)) from comment 
> -        Math.sin("foo" in writable);
> 
> Was this only a debugger helper line that got into the repo somehow? Or did
> this line ever have some purpose?

Yeah, I think it was just debugger stuff.

> 
> +++ b/js/xpconnect/tests/unit/test_bug854558.js
> 
> More test is always good, but why do we need this?

Because for some reason I removed it in bug 784233. I probably thought it would fail with the Sandbox change, but forgot that the whole getCOW machinery would actually cause that object to live in the window's scope, not the sandbox's.

> Finally this is unrelated just for my education, I dug into this test a
> little and was wondering why are the Object.hasOwnProperty.call related
> parts commented out in isProp / isPropHidden? Shouldn't those tests just
> work?

They appear to work locally. I'll uncomment them. :-)
https://hg.mozilla.org/integration/mozilla-inbound/rev/478ef0049c1a
https://hg.mozilla.org/mozilla-central/rev/478ef0049c1a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Good news that according to Alex the jetpack tests are failing because of a bug in some test helpers. Meaning it can be fixed easily without consequences.
Depends on: 855354
Blocks: 784305
You need to log in before you can comment on or make changes to this bug.