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

RESOLVED FIXED in Firefox 40

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

unspecified
mozilla40
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8589352 [details] [diff] [review]
Pass JSPROP_REDEFINE_NONCONFIGURABLE when trying to redefine a nonconfigurable property in another of XPConnect's many morality plays disguised as code
Attachment #8589352 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Attachment #8589352 - Flags: review?(bobbyholley) → review+
(Assignee)

Updated

3 years ago
Blocks: 1148750
Wait.  Why does AttachComponentsObject need to overwrite the existing Components value?  And why is that existing value non-configurable?  Where is it coming from?
(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
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.
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...
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?
Flags: needinfo?(jorendorff)
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.
(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.
(Assignee)

Comment 9

3 years ago
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
(Assignee)

Comment 10

3 years ago
Created attachment 8589833 [details] [diff] [review]
Don't try to redefine the non-configurable global Components property when EnableUniversalXPConnect is called multiple times
Attachment #8589833 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Attachment #8589352 - Attachment is obsolete: true
Attachment #8589833 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 11

3 years ago
Bad news: I don't think this is sufficient. Looking closer...
(Assignee)

Comment 12

3 years ago
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.
Flags: needinfo?(jorendorff)
(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?
(Assignee)

Comment 14

3 years ago
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.)
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 16

3 years ago
Created attachment 8591109 [details] [diff] [review]
part 2 - Make the global Components property configurable in cases where EnableUniversalXPConnect may later need to redefine it
Attachment #8591109 - Flags: review?(bobbyholley)
(Assignee)

Comment 17

3 years ago
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.
(Assignee)

Comment 18

3 years ago
Talos-other finishes normally with these patches.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49554e115696
Attachment #8591109 - Flags: review?(bobbyholley) → review+
(Assignee)

Updated

3 years ago
Summary: In XPCWrappedNativeScope::AttachComponentsObject(), use JSPROP_REDEFINE_NONCONFIGURABLE when redefining a nonconfigurable property → XPCWrappedNativeScope::AttachComponentsObject() should not try to redefine a nonconfigurable property
(Assignee)

Comment 19

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/693ab25e4c62
https://hg.mozilla.org/integration/mozilla-inbound/rev/f84caefdf90d
https://hg.mozilla.org/mozilla-central/rev/693ab25e4c62
https://hg.mozilla.org/mozilla-central/rev/f84caefdf90d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.