Closed Bug 1107443 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: site-compat)

Attachments

(8 files, 1 obsolete file)

3.29 KB, patch
jimb
: review+
Details | Diff | Splinter Review
3.92 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.05 KB, patch
peterv
: review+
jorendorff
: review+
Details | Diff | Splinter Review
2.23 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
18.80 KB, patch
peterv
: review+
jorendorff
: review+
Details | Diff | Splinter Review
7.93 KB, patch
Details | Diff | Splinter Review
2.19 KB, patch
mossop
: review+
Details | Diff | Splinter Review
5.89 KB, patch
peterv
: review+
Details | Diff | Splinter Review
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.  :(
So why are proxies converting a set to a define, exactly?  They're really not equivalent...
Flags: needinfo?(jorendorff)
> So why are proxies converting a set to a define, exactly?

Apparently "because they're buggy".  The fix for bug 1083211 fixed this.
Flags: needinfo?(jorendorff)
OK, so this leaves the property-defining ecma_5/misc/function-definition-eval.js does.  Should I just disable that part of the test?
Flags: needinfo?(jorendorff)
I'd delete it.
Flags: needinfo?(jorendorff)
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Erik, Irakli, let me know if you see a problem with the proposed course of action in comment 6.
Flags: needinfo?(rFobic)
Flags: needinfo?(evold)
(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.
Flags: needinfo?(evold)
Attachment #8547648 - Flags: review?(jimb) → review+
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?
Flags: needinfo?(evold)
Attachment #8547650 - Flags: review?(jorendorff) → review+
Attachment #8547652 - Flags: review?(jorendorff) → review+
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?
Attachment #8547653 - Flags: review?(jorendorff) → review+
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.
Flags: needinfo?(jorendorff)
(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?
Flags: needinfo?(evold)
Flags: needinfo?(dtownsend)
Flags: needinfo?(bzbarsky)
(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.
Flags: needinfo?(dtownsend)
> 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.
Flags: needinfo?(bzbarsky)
(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. :)
Flags: needinfo?(jorendorff)
Attachment #8549872 - Flags: review?(dtownsend) → review+
Attachment #8547649 - Flags: review?(peterv) → review+
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.
Attachment #8547650 - Flags: review?(peterv) → review+
Attachment #8547653 - Flags: review?(peterv) → review+
I filed bug 1125167 to track whether we want to ship this, also.
Depends on: 1125167
And disabled again for now because of gaia-unit-tests failing: https://hg.mozilla.org/integration/mozilla-inbound/rev/835c01857c0d
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.
Flags: needinfo?(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]
Flags: needinfo?(ryanvm)
Perfect, thank you!

Any idea offhand how (or whether?) I can do a try push with a custom gaia?
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?
Flags: needinfo?(arthur.chen)
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.
Flags: needinfo?(arthur.chen)
OK, great.  I created a PR for Gaia at https://github.com/mozilla-b2g/gaia/pull/27658
Flags: needinfo?(arthur.chen)
Looks good!
Flags: needinfo?(arthur.chen)
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.
Flags: needinfo?(arthur.chen)
Attached file gaia part (obsolete) —
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.
Flags: needinfo?(arthur.chen)
Attachment #8554968 - Flags: review?(rlu)
Attachment #8554968 - Flags: review?(arthur.chen)
OK.  Sounds like I should create a separate bug for the gaia bit, then.
Depends on: 1126124
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...
Flags: needinfo?(arthur.chen)
Sounds good, let's review the patch there. Please upload the attachment to bug 1126124 and make the review request.
Flags: needinfo?(arthur.chen)
Comment on attachment 8554968 [details]
gaia part

OK, moved.
Attachment #8554968 - Attachment is obsolete: true
Attachment #8554968 - Flags: review?(rlu)
Attachment #8554968 - Flags: review?(arthur.chen)
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
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?
Flags: needinfo?(jorendorff)
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.
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.
> We have no data about breakages being rare, actually. [...]

Sorry, I misunderstood. Given that, I withdraw the suggestion.
(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.
Flags: needinfo?(jorendorff)
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.
Flags: needinfo?(jorendorff)
Bug 1125624.
Flags: needinfo?(jorendorff)
Depends on: 1125624
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...
Blocks: 1178638
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.
Attachment #8627519 - Flags: review?(peterv)
Depends on: 1179088
Attachment #8627519 - Flags: review?(peterv) → review+
Ugh, the leave-open should have gone away....
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1197958
Depends on: 1206995
Depends on: 1261572
Depends on: 1326392
Note that this is being backed out in bug 1329323.
This didn't stick, so don't document it. See also bug 1329321.
Keywords: dev-doc-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.