1107443 - Make WindowProxy throw if you attempt to define a non-configurable property

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Per https://www.w3.org/Bugs/Public/show_bug.cgi?id=27502

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the sandbox mirroring stuff is making this pretty complicated.

I tried to mask off the bit in JS_CopyPropertyFrom, but the define that throws is actually due to the set caused by this line:

  var Cc = Components.classes;

and we're ending up finding a non-configurable descriptor on the proto for this (because presumably someone already did "var Cc" on this window).  So we do JS_SetPropertyById.

I figured this would be safe, but it's not, because it ends up calling JSObject::setGeneric which calls JSObject::nonNativeSetProperty which calls JSObject::nonNativeSetProperty which ends up in js::BaseProxyHandler::set which calls js::SetPropertyIgnoringNamedGetter which calls js::DirectProxyHandler::defineProperty.

And now we've converted a set to a define and I no longer have a way to tell that it's not an Object.defineProperty call.  :(

Comment 2

[Boris Zbarsky [:bzbarsky]]

So why are proxies converting a set to a define, exactly?  They're really not equivalent...

Comment 3

[Boris Zbarsky [:bzbarsky]]

> So why are proxies converting a set to a define, exactly?

Apparently "because they're buggy".  The fix for bug 1083211 fixed this.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, so this leaves the property-defining ecma_5/misc/function-definition-eval.js does.  Should I just disable that part of the test?

Comment 5

[Jason Orendorff [:jorendorff]]

I'd delete it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So I have this working except for one thing: the addon SDK.  Those tests are hidden, but of course we don't want to actually break SDK extensions...

So my current plan is to land fixes for all non-SDK tests, then make this change, but only if !IsCallerChrome.  That might allow me to get some web compat testing without having to fix up all the SDK bits (of which there are quite a lot).  If this turns out web-compatible, I'll go back and get the SDK fixed.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Stop defining non-configurable properties on the window in devtools

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop defining non-configurable properties on the window in dom and xpconnect JS files

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop defining non-configurable properties on the window in xpconnect sandbox code

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Fix JS engine tests that define non-configurable properties on the window without meaning to

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Make attempts to define a non-configurable property via WindowProxy throw

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 diff -w, for review convenience

Comment 13

[Boris Zbarsky [:bzbarsky]]

Erik, Irakli, let me know if you see a problem with the proposed course of action in comment 6.

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Boris Zbarsky [:bz] from comment #13)
> Erik, Irakli, let me know if you see a problem with the proposed course of
> action in comment 6.

It sounds like your change would not affect the sdk until later, when you hope to fix the sdk too, so that sounds fine to me.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Erik, thanks.  What are the M(JP) tests?  Those are apparently running the addon-sdk but not as chrome, so they're definitely being affected by these changes...  Do I need to worry about them?

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 8547653 [details] [diff] [review]
part 5.  Make attempts to define a non-configurable property via WindowProxy throw

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

I have a fairly big question about this one. r=me with that addressed; but don't hesitate to ask for a new review if it's nontrivial.

::: dom/base/nsGlobalWindow.cpp
@@ +791,5 @@
> +  // doing.  In the meantime, this still allows us to test web compat
> +  // behavior.
> +  if (desc.isPermanent() && !nsContentUtils::IsCallerChrome()) {
> +    return ThrowErrorMessage(cx, MSG_DEFINE_NON_CONFIGURABLE_PROP_ON_WINDOW);
> +  }

Huh! I'm not sure why this is sufficient.

It seems like you should be able to get here with JSPROP_PERMANENT *not* set (so that desc.isPermanent() returns false) and JSPROP_IGNORE_PERMANENT set, and that would result in a nonconfigurable property being created, if the property does not already exist. But your test actually checks that case. Do you know why it passes?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Hmm.  First I hear of JSPROP_IGNORE_PERMANENT.  ;)

It looks like it's set in the following cases:

1)  SetPropertyByDefining when HasOwnProperty claims the property exists.
2)  BaseProxyHandler::set in the same situation.
3)  PropDesc::populatePropertyDescriptor if !hasConfigurable().  But this is only true if
    we had a setUndefined on the propdesc, or did PropDesc::initFromPropertyDescriptor with
    a desc that has JSPROP_IGNORE_PERMANENT.  So I don't think this can introduce the flag
    into the system.

That seems to be it.  So where does "would result in a nonconfigurable property being created, if the property does not already exist" come in?  I don't see how this flag can be set if the property doesn't already exist.

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Boris Zbarsky [:bz] from comment #15)
> Erik, thanks.  What are the M(JP) tests?  Those are apparently running the
> addon-sdk but not as chrome, so they're definitely being affected by these
> changes...  Do I need to worry about them?

Hey, sorry for the late response.

Short answer is that the M(JP) test suite should be enabled everywhere soon, hopefully within a week, so making that red would be an issue.  At the moment it's red for other reasons anyhow.

I'm not sure what you mean that they running the sdk not as chrome?  is it more like a mochi-content test?

I guess you do need to worry about them, maybe Mossop can provide more information?

Comment 19

[Dave Townsend [:mossop]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> Erik, thanks.  What are the M(JP) tests?  Those are apparently running the
> addon-sdk but not as chrome, so they're definitely being affected by these
> changes...  Do I need to worry about them?

They're just the regular SDK tests but running with the mochitest harness as a base. You can run them locally right now with "mach jetpack-package" or "mach jetpack-addon" depending on which are broken. If your changes are breaking then I'd be worried and as Erik says the intention is to turn them on globally once they are green (probably after bug 1114752 lands). Most of the SDK runs in sandboxes I think with the system principal so they should be counted as chrome. page-mod content scripts should run with the principal of the page they attach to, so it's possible those are the cases being affected.

I'm getting a little lost trying to understand what this change is doing so I can't guess what might be breaking right now though.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what you mean that they running the sdk not as chrome?

Just that.  They're running it in an environment where IsCallerChrome() tests false.

> is it more like a mochi-content test?

Yes.

> You can run them locally right now with "mach jetpack-package" or "mach jetpack-addon"
> depending on which are broken.

Try run is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2882a59014b0&exclusion_state=all

I'm guessing its jetpack-package?

> Most of the SDK runs in sandboxes I think with the system principal

That should be fine.

> page-mod content scripts should run with the principal of the page they attach to

Would those be calling defineProperty/defineProperties on the window?

> I'm getting a little lost trying to understand what this change is doing

This change makes defineProperty or defineProperties with non-configurable descriptors on a window throw when called from non-chrome.  Note that descriptors are non-configurable by default, unless you say they're configurable.

Comment 21

[Jason Orendorff [:jorendorff]]

(In reply to Boris Zbarsky [:bz] from comment #17)
> That seems to be it.  So where does "would result in a nonconfigurable
> property being created, if the property does not already exist" come in?  I
> don't see how this flag can be set if the property doesn't already exist.

According to the standard, Object.defineProperty(obj, id, {value: 3}); is supposed to call obj.[[DefineOwnProperty]](), passing a property descriptor that has no [[configurable]] attribute. This is what JSPROP_IGNORE_PERMANENT is supposed to represent.

I guess it's not working yet. It's on my Q1 goals to make sure stuff like that works, and you're doing the right thing by adding the test now. So everything's good here. When I fix Object.defineProperty, it'll break your test and I'll hit you up for a review. :)

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Fix the addon SDK to not try defining non-configurable properties on content windows

Comment 23

[Peter Van der Beken [:peterv]]

Comment on attachment 8547650 [details] [diff] [review]
part 3.  Stop defining non-configurable properties on the window in xpconnect sandbox code

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

Ugh.

Comment 24

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/c079324a730e
   https://hg.mozilla.org/integration/mozilla-inbound/rev/a5452520acc2
   https://hg.mozilla.org/integration/mozilla-inbound/rev/21c74787b9b9
   https://hg.mozilla.org/integration/mozilla-inbound/rev/c90b2b0f5a03
   https://hg.mozilla.org/integration/mozilla-inbound/rev/d5dc874588ac
   https://hg.mozilla.org/integration/mozilla-inbound/rev/784b54d92929

Let's see how much of the web this breaks....

Comment 25

[Boris Zbarsky [:bzbarsky]]

I filed bug 1125167 to track whether we want to ship this, also.

Comment 26

[Boris Zbarsky [:bzbarsky]]

And disabled again for now because of gaia-unit-tests failing: https://hg.mozilla.org/integration/mozilla-inbound/rev/835c01857c0d

Comment 27

[Ryan VanderMeulen [:RyanVM]]

The disabling in comment 26 caused the newly-added tests to start permafailing. Disabled.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce0b4280ff3

https://treeherder.mozilla.org/logviewer.html#?job_id=5832575&repo=mozilla-inbound

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c079324a730e
https://hg.mozilla.org/mozilla-central/rev/a5452520acc2
https://hg.mozilla.org/mozilla-central/rev/21c74787b9b9
https://hg.mozilla.org/mozilla-central/rev/c90b2b0f5a03
https://hg.mozilla.org/mozilla-central/rev/d5dc874588ac
https://hg.mozilla.org/mozilla-central/rev/784b54d92929
https://hg.mozilla.org/mozilla-central/rev/835c01857c0d
https://hg.mozilla.org/mozilla-central/rev/7ce0b4280ff3

Comment 29

[Boris Zbarsky [:bzbarsky]]

Alright.  So I added some printf logging to the place where the exception is thrown... and I don't see those printfs in the full logs for the failing test runs.  Am I just looking in the wrong places?  I expect to see "BAILING OUT OF DEFINE" if the throw happens.

See https://treeherder.mozilla.org/#/jobs?repo=try&revision=537d5978b8fa for the try push.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Click one of the failing Gip jobs. Bottom-left pane on Treeherder. gecko-xxxxxxxx.log.

From one of your runs (Linux64 Gip(a)):
JavaScript error: file:///builds/slave/test/build/application/b2g/components/BrowserElementParent.js, line 82: NS_ERROR_NOT_IMPLEMENTED: SetNFCFocus for in-process mode is not yet supported
BAILING OUT OF DEFINE
0 as_createLaunchContext(initialPanelId = "root", initialPanelHandler = [object Object], activityHandler = undefined) ["app://settings.gaiamobile.org/js/startup.js":424]
    this = [object Object]
1 anonymous(initialPanelId = "root") ["app://settings.gaiamobile.org/js/startup.js":491]
    this = [object Object]

Comment 31

[Boris Zbarsky [:bzbarsky]]

Perfect, thank you!

Any idea offhand how (or whether?) I can do a try push with a custom gaia?

Comment 32

[Boris Zbarsky [:bzbarsky]]

So there are two places in gaia that are defining non-configurable properties on windows.  One is apps/keyboard/test/unit/keyboard/layout_rendering_manager_test.js and looks accidental.  The other is in apps/settings/js/startup.js and is very purposefully defining a non-configurable property on the window.

Arthur, it looks like you added this code in bug 1007600.  Why does this property need to be non-configurable?

Comment 33

[Arthur Chen [:arthurcc][inactive after 6/30]]

The property stores information regarding how the setting app gets launched. Setting it to non-configurable I just want to make sure it won't be manipulated by other scripts.

It is a fail-safe so I can set it to configurable if necessary.

Comment 34

[Boris Zbarsky [:bzbarsky]]

OK, great.  I created a PR for Gaia at https://github.com/mozilla-b2g/gaia/pull/27658

Comment 35

[Arthur Chen [:arthurcc][inactive after 6/30]]

Looks good!

Comment 36

[Boris Zbarsky [:bzbarsky]]

OK, so how do I go about getting that gaia bit merged?  Do I just push it to the main gaia repo myself?  I don't seem to have the bits to merge the PR in github.

Comment 37

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: gaia part

Please make the commit message in the form of 'Bug 1107443 - ....'. I am also requesting a review from the owner of keyboard.

Once you get r+, add a "checkin-needed" keyword, the patch is going to be merged by the release manager.

Comment 38

[Boris Zbarsky [:bzbarsky]]

OK.  Sounds like I should create a separate bug for the gaia bit, then.

Comment 39

[Boris Zbarsky [:bzbarsky]]

I filed bug 1126124 and amended the PR to have the new commit message.  Not sure whether I need to create a new PR to get the reviews over there or not...

Comment 40

[Arthur Chen [:arthurcc][inactive after 6/30]]

Sounds good, let's review the patch there. Please upload the attachment to bug 1126124 and make the review request.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8554968 [details]
gaia part

OK, moved.

Comment 42

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/ffe74d2a6dc94d0ec8c030753f254ebbf22e13aa
Bug 1107443 part 5. Fix the addon SDK to not try defining non-configurable properties on content windows. r=mossop

Comment 43

[Boris Zbarsky [:bzbarsky]]

So per some discussion on es-discuss, I'm going to change the behavior of our attempted patch a bit.  Instead of throwing on any non-configurable incoming descriptor, I'm going to throw only if this is a descriptor which explicitly specified "configurable: false".

In the long term that means precisely just checking isPermanent(), per comment 16 and comment 21, or being more explicit and checking for JSPROP_INGORE_PERMANENT before checking isPermanent().  In the short term, I could do JS_AlreadyHasOwnPropertyById in the window and not throw if there is no property yet, I think...  That's not quite the long-term behavior we want, though (e.g. it would throw if you try to defineProperty with the same name as an existing var or other window property and no "configurable: false" set in the descriptor, whereas that should not throw), so I'm a bit loath to do it.  And I don't think we have a way right now to tell that the descriptor was missing "configurable", right?

Jason, what's the timeframe on the changes to Object.defineProperty that comment 21 mentions?

Comment 44

[Mark S. Miller]

Since right now we're in a data gathering phase, breakages are rare, and the current behavior doesn't actually break jQuery since this one does have the exception path, perhaps we should: 
Leave the current patch as it is for a bit, since it is conservative enough to catch all remaining breakages that would be of interest. 

Obviously, we'd need to track that we need to actually fix this before it can make it out of Nightly. But hopefully by then we can fix it correctly -- taking account of the absence or presence of "configurable:" in the descriptor itself (per comment 21 IIUC). Perhaps by then we can also choose which fix based on more observed breakages.

Comment 45

[Boris Zbarsky [:bzbarsky]]

We have no data about breakages being rare, actually.  That's why we're trying to gather data.

I would like to make sure we don't ship behaviors other than the final one anywhere outside nightly, for sure.  And I'd like to avoid unnecessary breakage for nightly testers, even, if possible.  The question is whether it's worth even testing what's in this bug on nightly or whether the Object.defineProperty changes are close enough that we should just base things on those.

Comment 46

[Mark S. Miller]

> We have no data about breakages being rare, actually. [...]

Sorry, I misunderstood. Given that, I withdraw the suggestion.

Comment 47

[Jason Orendorff [:jorendorff]]

(In reply to Boris Zbarsky [:bz] from comment #43)
> Jason, what's the timeframe on the changes to Object.defineProperty that
> comment 21 mentions?

I think it's still 3 weeks out. That is, I expect it to land before February 24 with probability 70%. I've got a series of patches that do error-handling, leading up to it. I've taken a stab at standard DefineProperty and found that it's a bigger job than I thought when I wrote comment 21. But still in Q1.

If you need JSPROP_IGNORE_* right away, let me know and I can write you a quick hack for that.

Comment 48

[Boris Zbarsky [:bzbarsky]]

OK.  I think I should just wait for that, then.  I would really want to land this near the start of a cycle anyway, and at this point we're smack-dab mid-cycle.  So landing this after the next branchpoint on Feb 23 totally works for me.  What's the bug# to depend on here?

If you think it would take a lot more than 3 weeks and are willing to hack up something by 3 weeks from now that would give me JSPROP_IGNORE_PERMANENT in defineProperty, that would be awesome.

Comment 49

[Jason Orendorff [:jorendorff]]

Bug 1125624.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Looks like the work mentioned in comment 48 has actually happened; it's just that bug 1125624 was not involved.  :(

Going to update this patch...

Comment 51

[Boris Zbarsky [:bzbarsky]]

Attachment: Make WindowProxy try to throw if you attempt to explicitly define a non-configurable property

Er, or actually I missed bug 1125624 getting resolved.

In any case, this patch seems to be reasonably green on try, though still waiting for some tests.  I filed bug 1178638 for turning this on for beta/release and bug 1178639 about making getOwnPropertyDescriptor claim all properties on window are configurable.

Comment 52

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e12159b9f891

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e12159b9f891

Comment 54

[Boris Zbarsky [:bzbarsky]]

Ugh, the leave-open should have gone away....

Comment 55

[Kohei Yoshino [:kohei]]

Somehow this has slipped under my radar. Just posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/window-now-throws-when-defining-non-configurable-property-currently-only-on-nightly-and-developer-edition/

Comment 56

[Boris Zbarsky [:bzbarsky]]

Note that this is being backed out in bug 1329323.

Comment 57

[Florian Scholz (Open Web Docs)]

This didn't stick, so don't document it. See also bug 1329321.