1152106 - XPCWrappedNativeScope::AttachComponentsObject() should not try to redefine a nonconfigurable property

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Over in bug 1148750, I had some patches that cause JS_DefinePropertyById to behave like the standard. If you try to use it to change the value of an existing non-configurable, non-writable property, it fails.

Well, it causes certain Talos jobs to start timing out. I tracked it down: when we enablePrivileges (or something like that), Components gets redefined, and the second definition must pave over the first.

Here is the most direct fix. No change in behavior immediately, but it will allow bug 1148750 to land.

If there's any other way to fix this, let's do that instead. Adding a use of JSPROP_REDEFINE_NONCONFIGURABLE is bad karma.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Pass JSPROP_REDEFINE_NONCONFIGURABLE when trying to redefine a nonconfigurable property in another of XPConnect's many morality plays disguised as code

Comment 2

[Boris Zbarsky [:bzbarsky]]

Wait.  Why does AttachComponentsObject need to overwrite the existing Components value?  And why is that existing value non-configurable?  Where is it coming from?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #2)
> Wait.  Why does AttachComponentsObject need to overwrite the existing Components value?

Because the existing value is the Components shim.

> And why is that existing value non-configurable?

Because Components was non-configurable and we wanted the shim to look as much like Components as possible. We could probably change this.

> Where is it coming from?

https://hg.mozilla.org/mozilla-central/annotate/ab0490972e1e/dom/base/nsDOMClassInfo.cpp#l2378

Comment 4

[Boris Zbarsky [:bzbarsky]]

That's a link to the properties on the Components.interfaces object being defined.  Those are non-configurable, as is the "interfaces" property of the Components shim object but that has nothing to do with the "Components" property on the global.

This would be defined via the property descriptor we fill in at the end there via FillPropertyDescriptor, and that's not going to be using JSPROP_PERMANENT.  It's not even doing JSPROP_READONLY, since we're passing "false" for the readonly boolean argument to FillPropertyDescriptor.

And I can confirm that if I do on a web page in the console:

  Object.getOwnPropertyDescriptor(window, "Components")

I get back:

  Object { configurable: true, enumerable: true, value: Object, writable: true }

So I don't think the existing value is the Components shim.  And I see no instances of "const Components" in our tree.  Hence the question about where this existing non-configurable Components value is coming from.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I mean, XPCWrappedNativeScope::AttachComponentsObject passes JSPROP_PERMANENT when defining.  But I'd expect it to get called once per scope, and even then to end up with the same value as it did on the previous call...

Comment 6

[Bobby Holley (:bholley)]

Oh, my mistake, you're right. Good catch Boris.

In that case, the issue is probably the edge case where we end up defining an unprivileged Components object (nsXPCComponentsBase), and then subsequently enabling UniversalXPConnect, in which case we need to overwrite that with the fully privileged components object.

https://hg.mozilla.org/mozilla-central/annotate/ab0490972e1e/js/xpconnect/src/XPCJSRuntime.cpp#l532

To be clear, there are 3 types of objects:
* The Components shim (created lazily for web content)
* Unprivileged Components, aka |nsXPCComponentsBase| - this has a real Components.interfaces but not Components.classes. We need it for XBL.
* The full bonafide Components object aka |nsXPCComponents : public nsXPCComponentsBase|, exposed to System Principal globals, and also on-demand to globals that do enablePrivilege. 

I'm not sure offhand which situation has both an unprivileged Components object (which we basically only need for XBL) _and_ then later on does enablePrivilege (i.e. Talos). We can almost certainly eliminate this case if we understand where it's being used. The problem is most likely that our Talos scripts are getting loaded in a way that they get an nsXPCComponentsBase. We should almost certainly get rid of that.

Jason, can you dig up how this Talos script is being loaded?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, I see.

So the other option is to just define as configurable in the nsXPCComponentsBase case.  If we only get there for XBL+unprivileged anyway, then it's presumably happening in the XBL scope and presumably we have enough control there that this would be an OK behavior change.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #7)
> So the other option is to just define as configurable in the
> nsXPCComponentsBase case.  If we only get there for XBL+unprivileged anyway,
> then it's presumably happening in the XBL scope and presumably we have
> enough control there that this would be an OK behavior change.

That would certainly do in a pinch, but I want to confirm that this is actually XBL code doing the enablePrivilege, rather than some other random code on a weird codepath that still gets nsXPCComponentsBase and shouldn't. If it _is_ XBL, I guess we can just define it as non-configurable, at the risk of creating a new footgun. We could also do it as non-configurable in all cases (nsXPCComponents as well), but that would widen the footgun.

Comment 9

[Jason Orendorff [:jorendorff]]

nsInputStreamPump::OnStateStop -> ... ->
  nsScriptLoader::OnStreamComplete -> ... ->
    nsScriptLoader::EvaluateScript -> ... ->
      JS::Evaluate -> ... ->
        EnablePrivilege ->
          xpc::EnableUniversalXPConnect ->
            XPCWrappedNativeScope::AttachComponentsObject ->
              JS_DefinePropertyById

The filename option passed to JS::Evaluate is:

file://$OBJDIR/mozharness/venv/lib/python2.7/site-packages/talos/page_load_test/a11y/a11y.js

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Don't try to redefine the non-configurable global Components property when EnableUniversalXPConnect is called multiple times

Comment 11

[Jason Orendorff [:jorendorff]]

Bad news: I don't think this is sufficient. Looking closer...

Comment 12

[Jason Orendorff [:jorendorff]]

OK, after a lot of flailing around in the debugger, I found out that Components is first defined on this Window via xpc::InitGlobalObject (with aFlags=2).

I'm going to try making Components configurable.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #12)
> OK, after a lot of flailing around in the debugger, I found out that
> Components is first defined on this Window via xpc::InitGlobalObject (with
> aFlags=2).

Ok, so _that's_ what I was interested in in comment 8. What is the callstack that causes us to define without OMIT_COMPONENTS_OBJECT?

Comment 14

[Jason Orendorff [:jorendorff]]

It's being called from CreateNativeGlobalForInner(), in dom/base/nsGlobalwindow.cpp.

TreatAsRemoteXUL(aPrincipal) is true, in the case of the particular object where Components is being redefined later.

(The other case, nsContentUtils::IsSystemPrincipal(aPrincipal), also happens, but apparently for those we do not later redefine Components.)

Comment 15

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #14)
> It's being called from CreateNativeGlobalForInner(), in
> dom/base/nsGlobalwindow.cpp.
> 
> TreatAsRemoteXUL(aPrincipal) is true, in the case of the particular object
> where Components is being redefined later.

Ah! I see. That's why this only affects Talos.

> (The other case, nsContentUtils::IsSystemPrincipal(aPrincipal), also
> happens, but apparently for those we do not later redefine Components.)

Yes, because for those cases we should never be doing the enablePrivilege bit.

Ok. So I think the safest/smallest change is to modify AttachComponentsObject to only define non-configurable in the case that the object is actually a full components object (rather than an nsXPCComponentsBase). You can detect this by QIing mComponents to nsIXPCComponents, or just tracking a bool somewhere.

That'll keep the non-configurable behavior in the cases where it may matter most (preventing addons from accidentally overwriting Components and breaking things), while still giving us the ability to upgrade our components object as-necessary.

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - Make the global Components property configurable in cases where EnableUniversalXPConnect may later need to redefine it

Comment 17

[Jason Orendorff [:jorendorff]]

Both patches are necessary to fix this bug - the second one alone is not sufficient. That's why it's called "part 2". I'll land both.

Comment 18

[Jason Orendorff [:jorendorff]]

Talos-other finishes normally with these patches.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49554e115696

Comment 19

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/693ab25e4c62
https://hg.mozilla.org/integration/mozilla-inbound/rev/f84caefdf90d

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/693ab25e4c62
https://hg.mozilla.org/mozilla-central/rev/f84caefdf90d