854558 - Enforce __exposedProps__ for Sandboxes

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Enforce __exposedProps__ for Sandboxes. v1

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=21d57f5f238f

Comment 3

[Gabor Krizsanits (INACTIVE)]

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?

Comment 4

[Bobby Holley (:bholley)]

(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. :-)

Comment 5

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/478ef0049c1a

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/478ef0049c1a

Comment 7

[Gabor Krizsanits (INACTIVE)]

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.