1377587 - Remove __exposedProps__

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Comment 1

[Jason Orendorff [:jorendorff]]

Looks like the Addon SDK is the last thing using it.

Comment 2

[(no longer active)]

The SDK is going to be removed in bug 1371065.  We don't need to wait until after 57 for this one.

Comment 3

[Kris Maglione [:kmag]]

This is still used by a lot of extensions:

https://dxr.mozilla.org/addons/search?q=__exposedProps__

I'm not opposed to removing it before 57, since it's been deprecated for a long time, but it will definitely break things.

Comment 4

[(no longer active)]

Yeah OK we can wait then.  Thanks for checking!

Comment 5

[Andrew McCreight [:mccr8]]

Outside of tests, I only see one occurrence of __exposedProps__, in the AddonSDK's PlainTextConsole.

Comment 6

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> Outside of tests, I only see one occurrence of __exposedProps__, in the
> AddonSDK's PlainTextConsole.

We can ignore that. The only reason I haven't removed that module yet is I haven't had the time to make sure the sdk/system/globals module isn't tangled up with any of the stuff the devtools or the remaining loader tests use. (And since I'm giving better than even odds to the loader tests using it, the time to untangle it from that test harness.)

But the SDK doesn't actually use XOWs, so that shouldn't actually matter at all, unless some SDK or add-on code is doing something obscenely stupid, like passing that console object to content code.

Comment 7

[Andrew McCreight [:mccr8]]

Just for fun, I hacked up a patch to disable exposedProps. It looks like the major work here will be figuring out what to do with the various test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84bc8191923a0902bbb2581febb7eb17d4cbde75&selectedJob=125001854
Tests use it in some places, but when I talked to bholley he said that some of the special powers stuff uses a separate mechanism involving a flag on a compartment, so presumably the uses of exposedProps could be replaced with SpecialPower somehow.

Comment 8

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] from comment #7)
> Just for fun, I hacked up a patch to disable exposedProps. It looks like the
> major work here will be figuring out what to do with the various test
> failures:

Hm. It looks like most of these test failures aren't actually related to __exposedProps__, but the fact that attempting to access properties on objects without __exposedProps__ now throws rather than warns.

In some of those cases, I suspect we actually have bugs. For instance, here:

http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/dom/base/test/test_blockParsing.html#58

we get a failure because attempting to access the "capture" property throws. Which means that the "once" property was probably silently ignored before.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #8)
> (In reply to Andrew McCreight [:mccr8] from comment #7)
> > Just for fun, I hacked up a patch to disable exposedProps. It looks like the
> > major work here will be figuring out what to do with the various test
> > failures:
> 
> Hm. It looks like most of these test failures aren't actually related to
> __exposedProps__, but the fact that attempting to access properties on
> objects without __exposedProps__ now throws rather than warns.

Seems like a great first step would be to preserve the warn behavior and disable all the actual __exposedProps__ stuff.

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Kris Maglione [:kmag] from comment #8)
> Hm. It looks like most of these test failures aren't actually related to
> __exposedProps__, but the fact that attempting to access properties on
> objects without __exposedProps__ now throws rather than warns.

Good catch. I was doing that at first, but then I got greedy. Reverting that a bit fixes most of the errors, except for test_cpows.xul which may need to be fixed, and a number of tests that at a glance seem to be explicitly testing __exposedProps__: test_cows.xul, test_exposeInDerived.xul, test_bug1082450.js, test_bug813901.js, test_bug780370.js, test_bug854558.js, test_bug853709.js.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: Bug 1377587, part 1 - Always act like __exposedProps__ is missing.

test_bug813901.js: error message changed

test_bug854558.js: don't allow exposedProps in sandboxes any more.

test_bug780370.js: I think this is failing due to a changed error message, but I'm not sure how to fix it.

test_bug853709.js: we don't expect __exposedProps__ to do anything any more

test_bug1082450.js: Remove some checks to make sure it works, but failing cases still fail

Review commit: https://reviewboard.mozilla.org/r/171890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171890/

Comment 12

[Andrew McCreight [:mccr8]]

I fixed some of the failures, but in some cases, the exception appears to be an error corresponding to an NS_ERROR thing. I don't remember exactly what that means or how to fix it. I haven't worked out yet how to fix test_cpows.xul, and I haven't looked at  test_cows.xul or test_exposeInDerived.xul.

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #12)
> I fixed some of the failures, but in some cases, the exception appears to be
> an error corresponding to an NS_ERROR thing. I don't remember exactly what
> that means or how to fix it.
Oh right, I need to actually report an error when returning false, like the old code did.

Comment 14

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] from comment #12)
> I haven't worked out yet how to fix test_cpows.xul, and I haven't looked at
> test_cows.xul or test_exposeInDerived.xul.

CPOW usage should be mostly going away outside of tests, so maybe we can just
drop the parts of the tests that rely on __exposedProps__? Just looking at the
test code, it's not really clear to me what the purpose of __exposedProps__ is
there, anyway... They don't seem to be used in cross-origin contexts.s

But either way, I think we probably want to stop allowing privileged CPOWs to
be exposed to content scopes at this point.

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 8900507 [details]
Bug 1377587, part 1 - Always act like __exposedProps__ is missing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171890/diff/1-2/

Comment 16

[Andrew McCreight [:mccr8]]

In the latest iteration of the patch, I've left ExposedPropertiesOnly::deny() alone, and made ExposedPropertiesOnly::check() always act like there is no __exposedProperties__ on the object. This means that it returns false, but does not call into JS_ReportErrorASCII(). The rest of the changes are mostly adjusting tests to ensure that if you access something that has exposedProps on it, it fails instead of succeeds. I've done this by debugging failures, but I should really audit the tests to make sure there aren't any odd cases that we test where we succeed when we should fail. I haven't yet trying to figure out test_cpows.xul, but test_cows.xul is passing locally.

Comment 17

[Andrew McCreight [:mccr8]]

I've now audited every instance of __exposedProperties__ in the code base, to make sure there wasn't some random test that we should be failing that is passing. I actually found a minor bug in test_bug1065185.html, where we were skipping the first iteration of the test (this was actually an instance of a test that should have been failing with the __exposedProps__ removal, but was not). I also went through and cleaned up various comments. After my patch, the only place __exposedProps__ should appear in the code base is in the error message for ReportWrapperDenial which explains that __exposedProps__ is no longer supported. I just need to make sure that a try run is still green.

Comment 18

[Andrew McCreight [:mccr8]]

As Kris pointed out, this does not depend on removing the AddonSDK.

Comment 19

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #17)
> After my patch, the only place __exposedProps__ should appear in the code base is in
> the error message for ReportWrapperDenial which explains that
> __exposedProps__ is no longer supported.
Well, plus various tests that check that __exposedProps__ doesn't do anything.

Comment 20

[Andrew McCreight [:mccr8]]

I won't land this until we figure out what to do with bug 1392026, but I have the patches finished, so I'll just put them up for review.

One thing I'm not sure of is whether this comment still is accurate: "For extra security, we override the traps that allow content to pass an object to chrome, and perform extra security checks on them." For now, I've left it in place.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56020a7c1acf058a29b9e0a94f1ecd990fa31d07

Comment 21

[Andrew McCreight [:mccr8]]

Attachment: Bug 1377587, part 2 - Rename ExposedPropertiesOnly to OpaqueWithSilentFailing.

This class doesn't have anything to do with __exposedProps__ any more,
so give it a more descriptive name. We'd still like to remove it
entirely eventually.

Review commit: https://reviewboard.mozilla.org/r/177406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177406/

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 8900507 [details]
Bug 1377587, part 1 - Always act like __exposedProps__ is missing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171890/diff/2-3/

Comment 23

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8900507 [details]
Bug 1377587, part 1 - Always act like __exposedProps__ is missing.

https://reviewboard.mozilla.org/r/171890/#review183224

::: commit-message-ab308:7
(Diff revision 3)
> +The tests that I changed all involve testing the behavior with
> +__exposedProps__. I adjusted them to expect it to fail, or to adjust
> +the error message they get when they fail. That seemed better than
> +deleting them entirely.

I'm not quite sure why are you keeping some of these tests. To me it would make sense to just remove all instances of __exposedProps__ from the tree with this patch. Could you elaborate a bit on this why you prefer it this way? I've decided to r+ it anyway, because of the time zone difference I don't want to hold up this patch over something that can be done easily as a followup. One thing we should do before landing is to inform the add-on team about this, because last time I checked with some special flag legacy addons could still work on nightly because of some legacy addon porting work.

::: dom/base/test/chrome/cpows_child.js:108
(Diff revision 3)
> +    if (is_remote) {
> +      ok(obj.a == undefined, "__exposedProps__ should not work");
> +    } else {
> +      // The same process test is not run as content, so the field can
> +      // be accessed even though __exposedProps__ has been removed.
> -    ok(obj.a == 1, "correct value from parent");
> +      ok(obj.a == 1, "correct value from parent");

I wonder if we should just turn this test off for non-e10s...

::: dom/base/test/chrome/cpows_child.js:269
(Diff revision 3)
> -  ok(obj.wont_die.f == 2, "got reverse CPOW");
> +  ok(obj.wont_die.f == undefined, "got reverse CPOW");
>    obj.will_die = null;
>    Components.utils.schedulePreciseGC(function() {
>      addMessageListener("cpows:lifetime_test_3", (msg) => {
> -      ok(obj.wont_die.f == 2, "reverse CPOW still works");
> +      ok(obj.wont_die.f == undefined, "reverse CPOW still works");

Would this line throw if the object wrapped by the CPOW on the parent side died? If so then it should be mentioned here birefly that we're relying on this fact, if not then maybe we should just remove the test. We should move away from CPOWs completly anyway...

::: js/xpconnect/tests/chrome/test_bug1065185.html:32
(Diff revision 3)
>    function loaded() {
> -    switch(++gLoadCount) {
> +    switch(gLoadCount++) {
>        case 0:
> -        doMonitor([]);
> +        doMonitor([/access to property "a"/i]);
>          window[0].wrappedJSObject.probe = { a: 2, __exposedProps__: { 'a': 'r' } };
> -        is(window[0].eval('probe.a'), 2, "Accessed exposed prop");
> +        is(window[0].eval('probe.a'), undefined, "Accessed exposed prop");

Why do we want to keep this test?

::: js/xpconnect/tests/chrome/test_cows.xul:138
(Diff revision 3)
> +    is(writable.foo, undefined,
> +       "reading from a write-only exposed prop should return undefined");

Shouldn't we just remove all the exposedProps tests from this file or the entire test file and just add some simple tests for OpaqueWithSilentFailing instead?

Comment 24

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8905610 [details]
Bug 1377587, part 2 - Rename ExposedPropertiesOnly to OpaqueWithSilentFailing.

https://reviewboard.mozilla.org/r/177406/#review183246

\o/

Comment 25

[Andrew McCreight [:mccr8]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23)
> I'm not quite sure why are you keeping some of these tests.

My basic reasoning was twofold. First, I wanted to make sure that I was actually removing all of the __exposedProps__ behavior that we're testing for, so I figured I'd just go ahead and update the tests to make sure the resulting behavior was reasonable. If I'm going to update the tests anyways, why not land the updated tests? Secondly, I am worried about, perhaps unreasonably, that some kind of bungled merge or whatever could restore the behavior of exposedProps and we wouldn't notice because there are no tests.

It certainly doesn't make much sense to leave it in there long term. I can file a followup bug to remove these tests entirely, and then in a month or two somebody can go through and double check that things are really really removed and then delete the tests. Does that sound reasonable?

> One thing we should do before landing is to inform the add-on team
> about this, because last time I checked with some special flag legacy addons
> could still work on nightly because of some legacy addon porting work.

Alright, I can figure out who to contact, though at this point I expect a lot of legacy addons are broken anyways due to other changes.
 
> I wonder if we should just turn this test off for non-e10s...

I can talk to Bill about that. I also need to figure out if that behavior on non-e10s is expected or not.

> Would this line throw if the object wrapped by the CPOW on the parent side
> died? If so then it should be mentioned here birefly that we're relying on
> this fact, if not then maybe we should just remove the test.

That is a little strange. I think with bug 1397513 this will throw whether or not the object is alive, so maybe in the course of investigating that I can figure out if we depend on this returning undefined.

> Why do we want to keep this test?
See above.

> Shouldn't we just remove all the exposedProps tests from this file or the
> entire test file and just add some simple tests for OpaqueWithSilentFailing
> instead?

See above. I'm hoping it won't be too hard to replace OpaqueWithSilentFailing with Opaque and then we won't need any testing.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a058c19c235
part 1 - Always act like __exposedProps__ is missing. r=krizsa
https://hg.mozilla.org/integration/autoland/rev/cdb4e0259eeb
part 2 - Rename ExposedPropertiesOnly to OpaqueWithSilentFailing. r=krizsa

Comment 27

[Gabor Krizsanits (INACTIVE)]

(In reply to Andrew McCreight [:mccr8] from comment #25)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23)
> It certainly doesn't make much sense to leave it in there long term. I can
> file a followup bug to remove these tests entirely, and then in a month or
> two somebody can go through and double check that things are really really
> removed and then delete the tests. Does that sound reasonable?

Right, this makes perfect sense.

> 
> > One thing we should do before landing is to inform the add-on team
> > about this, because last time I checked with some special flag legacy addons
> > could still work on nightly because of some legacy addon porting work.
> 
> Alright, I can figure out who to contact, though at this point I expect a
> lot of legacy addons are broken anyways due to other changes.

I guess we should just give Andy a heads-up, just in case. Andy, by removing
__exposedProps__ some legacy add-ons will surely break. That would not be an 
issue since this is 57, but a few weeks ago I heard that there was a special
flag to reenable legacy add-ons on nightly for porting reasons. IF that is
still the case I thought it's the best if you know about this change.
(if that flag is already gone you can surely ignore this...)

>  
> > I wonder if we should just turn this test off for non-e10s...
> 
> I can talk to Bill about that. I also need to figure out if that behavior on
> non-e10s is expected or not.

Perfect.

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0a058c19c235
https://hg.mozilla.org/mozilla-central/rev/cdb4e0259eeb

Comment 29

[Andy McKay]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #27)
> I guess we should just give Andy a heads-up, just in case. Andy, by removing
> __exposedProps__ some legacy add-ons will surely break. That would not be an 
> issue since this is 57, but a few weeks ago I heard that there was a special
> flag to reenable legacy add-ons on nightly for porting reasons. IF that is
> still the case I thought it's the best if you know about this change.
> (if that flag is already gone you can surely ignore this...)

There is no expectation for add-ons on Nightly to keep working. Please don't let that stop or distract you.