Closed Bug 790732 Opened 12 years ago Closed 11 years ago

Stop defining |Components| object in content scopes

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
relnote-firefox --- 22+

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: addon-compat, dev-doc-needed, relnote)

Attachments

(8 files, 2 obsolete files)

3.14 KB, patch
Details | Diff | Splinter Review
2.69 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
955 bytes, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.34 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
5.04 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.66 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.12 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.91 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
I've been saying I'd do this since we decided to do it in march. And now I've finally started really working on it.

The basic idea is that we don't want Components to be exposed to web content, but we still need it in content scopes because XBL bindings are cloned into content scopes, and they (occasionally) need it for QI and Components.lookupMethod. So we define Components on a special, hidden object, and splice that object onto the scope chain for cloned XBL methods. Assuming the XBL code doesn't manually leak the Components object, there should be no other-way (from a object-capability perspective) for web content to get its hands on |Components|.

Conceptually this is pretty simple, but there are hiccups. One of them is that anonymous XBL methods (constructors and destructors) already use the bound element on the scope chain. Jorendorff is going to expose an API that allows us to use |With| objects for this in bug 790676.

Another hitch is what to about all the existing code in our test suite that uses Components. We can't simply expose Components as SpecialPowers.wrap(systemWindow.Components), because that means that Components.interfaces.nsIFoo isn't a bonafide nsJSIID object, which break QI. So as a first step, I'm going to tweak the enablePrivilege code to just define the Components object on the window, and also add a SpecialPowers mechanism to get the local Components object. Hopefully those two together should give put us in a decent place.
Blocks: 693733
> anonymous XBL methods (constructors and destructors) already use the bound element

Why is this a problem?  You can handle that today, without new APIs, with pseudocode like so:

  scopeObj = JS_NewObject(cx, nullptr, nullptr, element);
  JS_DefineProperty(cx, scopeObj, "Components", ...);
  // Use scopeObj as the parent for your method

It'll change behavior in cases when the element or the document had a property called "Components", but those should be quite rare I would think.
(In reply to Boris Zbarsky (:bz) from comment #1)
> > anonymous XBL methods (constructors and destructors) already use the bound element
> 
> Why is this a problem?  You can handle that today, without new APIs, with
> pseudocode like so:
> 
>   scopeObj = JS_NewObject(cx, nullptr, nullptr, element);
>   JS_DefineProperty(cx, scopeObj, "Components", ...);
>   // Use scopeObj as the parent for your method
> 
> It'll change behavior in cases when the element or the document had a
> property called "Components", but those should be quite rare I would think.

Yes, but I'm trying to avoid making N XBL scope objects. It doesn't matter too much, but I'd like the ability to make dynamic updates to the XBL scope object if need be, and I don't like the idea of that many Components references floating around. Certainly could work if bug 790676 turns out to be a chore, though.
> Yes, but I'm trying to avoid making N XBL scope objects

I'm not sure how bug 790676 will prevent the need for that, exactly...  Would you just end up making the scope chain be elements -> document -> window -> singleton (per-compartment?) xbl scope object or something?
The idea is to expose the ability for JSAPI users to make |With| environments (which, perhaps in the future, don't even need to be JS objects). So the scope chain would be:

With(element) -> mXBLScopeObject -> window
But right now the scope chain goes through all the element's ancestors and the document before getting to the window.  Are you sure you can change that without breaking things?
(In reply to Boris Zbarsky (:bz) from comment #5)
> But right now the scope chain goes through all the element's ancestors and
> the document before getting to the window.  Are you sure you can change that
> without breaking things?

For HTML documents (where we're actually going to apply this), that parent chain is generally element->document (with the exception of form elements). So we could probably just do With() for each element in the parent chain without significant practical overhead. Or just With(element)->With(document)->... might be good enough.
Oh, we're not doing this for chrome XBL?  In that case it might be ok, yeah.
(In reply to Boris Zbarsky (:bz) from comment #7)
> Oh, we're not doing this for chrome XBL?  In that case it might be ok, yeah.

We could, but I'm assuming we might as well avoid introducing extra unnecessary objects for chrome scopes, since it would add up with all the JSMs we have (though, realistically, it's probably negligible).
A lot of this turns out to be getting the test suite fixed up, since lots and lots of tests (that don't use enablePrivilege) still QI using Components.interfaces. I've got some approaches. Trying now:

https://tbpl.mozilla.org/?tree=Try&rev=7d2e6d98c3bb
bz, sheppy: What kind of communication are we going to need to do for this one? The outcome of this bug will be that |Components| is no longer visible to content. This could break various random things (people checking for Components to do browser-detection, for example). The major thing that this will break (on purpose!) is the ability for content scripts to QI random DOM objects to various interfaces. For this part, we could push out a warning whenever a content script accessed Components.interfaces. Does doing this for one cycle before actually removing it sound necessary and/or sufficient?
(also, smaug says he's seen people getting XHR interface constants off of Components.interfaces.nsIXMLHttpRequest).
Keywords: relnote
Bobby, is this blocked by bug 781689? That bug removes some code in content that does QI and getInterface using things in Components.interfaces.
(In reply to Andrew McCreight [:mccr8] from comment #13)
> Bobby, is this blocked by bug 781689? That bug removes some code in content
> that does QI and getInterface using things in Components.interfaces.

It sure does.
Depends on: 781689
tps://tbpl.mozilla.org/?tree=Try&rev=2133df1c1ea8
Joel, one thing about this patch is that it's going to bork the dozen-or-so crashtests that use |Components|. Now, those crashtests are green right now, because currently they'll just throw when they access the undefined property and the test will finish, which is considered PASS for crashtests. But they're not testing what they're supposed to.

You had a patch to hook SpecialPowers up to the reftest harness, right? Is there any way we could still do that, and just skip the jsreftest bit that was causing problems?
The patch I had in in bug 762908.  The problem with it is that it was only tested on jsreftests which there was a couple failures that kept cropping up as a side effect.
(In reply to Joel Maher (:jmaher) from comment #18)
> The patch I had in in bug 762908.  The problem with it is that it was only
> tested on jsreftests which there was a couple failures that kept cropping up
> as a side effect.

Ok. Do you think it's worth repurposing that patch to make SpecialPowers available to crashtests? Seems like an easy change, and most certainly worthwhile IMO (and I'm sure Jesse would agree).
since it doesn't work in jsreftests, we might actually make it work for crashtests :)
(In reply to Joel Maher (:jmaher) from comment #20)
> since it doesn't work in jsreftests, we might actually make it work for
> crashtests :)

I filed bug 792029 for this. IIRC, the reason it didn't work for jsreftests wasn't in the making-SpecialPowers-available part, but in the using-SpecialPowers-to-rewrite-the-harness part. It seems like converting this to be available to crashtests without mucking with harness code should be a 5-minute job.
Depends on: 792036
Ok, this is green on try now (at least for linux-debug). I've decided to split the testsuite-wrangling into bug 792036, which can land separately as a prereq.
> What kind of communication are we going to need to do for this one?

You want to talk to Jorge, I think.
Blocks: 429070
(In reply to Boris Zbarsky (:bz) from comment #23)
> > What kind of communication are we going to need to do for this one?
> 
> You want to talk to Jorge, I think.

For addon compat? It seems to me like that's not likely to be the biggest issue here (since addons are generally privileged). I'd think that web compat would be the bigger issue (in the same vein as enablePrivilege), but then again I really have no idea.
Could you add a telemetry probe to get some more information?
(In reply to Olli Pettay [:smaug] from comment #25)
> Could you add a telemetry probe to get some more information?

We could, though I'm not sure what kind of delay that would impose on this project. Is it worth doing a telemetry probe just on Nightly? Though at that point, we almost might as well just land it and see if the breakage is significant enough to warrant a backout and more outreach. This stuff has to go away sooner or later...
Blocks: 794943
Depends on: 795275
I landed telemetry and a console warning in bug 795275 for mozilla18, which recently became the release version.

Originally, about 20% of Nightly/Aurora users were hitting an access to Components somewhere in their session. About 2 weeks before FF18 was released, the number dropped significantly, which presumably meant that one or more major websites noticed the warning during testing and fixed it. The numbers are now 10-11%, and hopefully will fall more.

This goes to show the effectiveness of console warnings!
Attached patch Bit-rotted WIP.Splinter Review
I found this patch lying around. It's going to need to change due to the recent
XBL scope stuff that landed, but I wanted to post it here to avoid forgetting about it.
Now that we have XBL scopes, we no longer need any extra machinery from the JS engine to do this.
No longer depends on: 790676
Depends on: 834697
Depends on: 843231
No longer depends on: 843231
Changing the bug summary, since the XBL scope stuff means that we can just turn it off for content and XBL will still work.
Summary: Move |Components| object onto an object on the XBL scope chain → Stop defining |Components| object in content scopes
Hm, not sure how that happened. Anyway, pushing again:

https://tbpl.mozilla.org/?tree=Try&rev=c6401e6e5dce
Depends on: 843711
I added this when I thought we'd be defining Components on an object on the XBL
scope chain. At this point, it's not necessary anymore.
Attachment #717963 - Flags: review?(mrbkap)
This method is larely deprecated. The only two consumers are JSD and the setup
for the safe JSContext, neither of which use system principal and thus neither
of which should have |Components|.
Attachment #717964 - Flags: review?(mrbkap)
Attachment #717965 - Flags: review?(mrbkap)
\o/
Attachment #717966 - Flags: review?(mrbkap)
This may break greasemonkey scripts that depend on inheriting |Components| from the content window. The compat solution for now is just to pass { wantComponents: true } as a sandboxOption when creating the sandbox for content script evaluation.
Thanks for the heads up.  By quick estimate from my last snapshot of userscripts.org: about 500 of 80,000 scripts do this.  Mostly platypus generated ones, but not exclusively.
(In reply to Anthony Lieuallen from comment #40)
> Thanks for the heads up.  By quick estimate from my last snapshot of
> userscripts.org: about 500 of 80,000 scripts do this.  Mostly platypus
> generated ones, but not exclusively.

By "do this", I assume you mean accessing Components in content?
Yes-ish.  I mean are (the source of) a user script which contains the string "Components.", not preceded by "//".
Attachment #717963 - Flags: review?(mrbkap) → review+
Attachment #717964 - Flags: review?(mrbkap) → review+
Attachment #717965 - Flags: review?(mrbkap) → review+
Comment on attachment 717966 [details] [diff] [review]
Part 4 - Omit Components object for content scopes. v1

\o/
Attachment #717966 - Flags: review?(mrbkap) → review+
Try push on all platforms to flush out any last dependencies in the test suite:

https://tbpl.mozilla.org/?tree=Try&rev=e02eec70a603

I'll send some mail to dev-platform letting people know this is coming.
relnote-firefox: --- → ?
I _think_ this try run looks good, except for two late-breaking usages of Components in M-5 which I've now fixed. There are some other oranges, but I'm pretty sure they're due to dbaron's predicted orange spike due to NS_ASSERTION changes and not due to this patch.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4e1f22477180
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cac9eeba156e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/68c3cee0d464
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/00b10e10d356
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8d36cce3e3
Blocks: 848030
Unfortunately, this appears to have caused talos-other hangs/crashes. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bef7c417aa93
Depends on: 848998
So, I fixed the Talos stuff, but I've held off on relanding because I wanted to sort out the compat issues. The consensus on dev-platform seems to be that we want to shim in support for the _existence_ of a Components object, as well as the associated interface constants. It also seems good to have this behavior behind a pref.

So I've now done that. I've re-organized my patches here such that we no longer back out the warning/telemetry, but instead make the associated tests for that stuff pref-controlled. I'm also planning on landing this stuff preffed off, and enabling the pref in a followup push. Patches coming up.
Attachment #717965 - Attachment is obsolete: true
Attachment #717966 - Attachment is obsolete: true
Attachment #727972 - Flags: review?(mrbkap)
Attachment #727973 - Flags: review?(mrbkap)
Attachment #727968 - Flags: review?(mrbkap) → review+
Comment on attachment 727969 [details] [diff] [review]
Part 4 - Define a lazily-resolved shim for Components. v1

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

r=me with my questions answered or addressed.

::: dom/base/nsDOMClassInfo.cpp
@@ +5223,5 @@
> +  const char *geckoName;
> +  const char *domName;
> +};
> +
> +const InterfaceShimEntry kInterfaceShimMap[] =

This needs a comment explaining which interfaces were chosen and why.

@@ +5260,5 @@
> +  { "nsIDOMXULCheckboxElement", "XULCheckboxElement" },
> +  { "nsIDOMXULPopupElement", "XULPopupElement" } };
> +
> +const uint32_t kInterfaceShimCount =
> +  sizeof(kInterfaceShimMap) / sizeof(kInterfaceShimMap[0]);

I'm sure there's either a macro or a template function that does this for us.

@@ +5269,5 @@
> +  // Create a fake Components object.
> +  JSObject *components = JS_NewObject(cx, nullptr, nullptr, global);
> +  NS_ENSURE_TRUE(components, NS_ERROR_OUT_OF_MEMORY);
> +  bool ok = JS_DefineProperty(cx, global, "Components", JS::ObjectValue(*components),
> +                              JS_PropertyStub, JS_StrictPropertyStub, JSPROP_ENUMERATE);

Components is currently readonly/permanent. Should the shim likewise be non-configurable?

@@ +5277,5 @@
> +  JSObject *interfaces = JS_NewObject(cx, nullptr, nullptr, global);
> +  NS_ENSURE_TRUE(interfaces, NS_ERROR_OUT_OF_MEMORY);
> +  ok = JS_DefineProperty(cx, components, "interfaces", JS::ObjectValue(*interfaces),
> +                         JS_PropertyStub, JS_StrictPropertyStub,
> +                         JSPROP_ENUMERATE | JSPROP_PERMANENT);

(Here and below) Why PERMANENT without READONLY?

@@ +5320,5 @@
>      return NS_OK;
>    }
>  
> +  if (id == XPCJSRuntime::Get()->GetStringID(XPCJSRuntime::IDX_COMPONENTS)) {
> +    *_retval = true;

Nit: I think *_retval is guaranteed to be true on entry to this function.
Attachment #727969 - Flags: review?(mrbkap) → review+
Attachment #727970 - Flags: review?(mrbkap) → review+
Attachment #727972 - Flags: review?(mrbkap) → review+
Attachment #727973 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #55)

> This needs a comment explaining which interfaces were chosen and why.

Ok.

> I'm sure there's either a macro or a template function that does this for us.

Do you happen to know what that might be?

> > +  // Create a fake Components object.
> > +  JSObject *components = JS_NewObject(cx, nullptr, nullptr, global);
> > +  NS_ENSURE_TRUE(components, NS_ERROR_OUT_OF_MEMORY);
> > +  bool ok = JS_DefineProperty(cx, global, "Components", JS::ObjectValue(*components),
> > +                              JS_PropertyStub, JS_StrictPropertyStub, JSPROP_ENUMERATE);
> 
> Components is currently readonly/permanent. Should the shim likewise be
> non-configurable?

I don't think so. In particular, that will break the UniversalXPConnect hack that defines Components in the content scope as soon as enablePrivilege is called, because it won't be able to redefine the property. More to the point, this thing is just a shim, so I don't see a huge reason to do anything like that.

> (Here and below) Why PERMANENT without READONLY?

Happy to either remove PERMANENT or add READONLY. Which would you prefer? The latter is probably better, I guess...

> Nit: I think *_retval is guaranteed to be true on entry to this function.

Ok, I'll MOZ_ASSERT it.
(In reply to Bobby Holley (:bholley) from comment #56)
> Do you happen to know what that might be?

MOZ_ARRAY_LENGTH appears to be the way to go.

> Happy to either remove PERMANENT or add READONLY. Which would you prefer?
> The latter is probably better, I guess...

Yeah, I'd prefer adding READONLY.
(In reply to Blake Kaplan (:mrbkap) from comment #57)
> (In reply to Bobby Holley (:bholley) from comment #56)
> > Do you happen to know what that might be?
> 
> MOZ_ARRAY_LENGTH appears to be the way to go.

Looks like mozilla::ArrayLength is even better :-)

https://tbpl.mozilla.org/?tree=Try&rev=98dce9438d21
Looks like that test had a load ordering race condition which manifested only on try. Fixed that, and pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/29acf1494fed
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/48bc6259ca24
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ed0f76ecfefd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4fdc85753f3a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2a94932a1fa7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d6eef579bea7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5aabc5770767
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8e59146e161e

Note that I'm landing this prefed off to make sure the infrastructure sticks, and will land a followup patch to twiddle the pref. I'm going to do them in the same bug to avoid confusion, so marking [leave open] for now.
Whiteboard: [leave open]
Try push for flipping the pref:

https://tbpl.mozilla.org/?tree=Try&rev=dfabfc7a0681
(In reply to Bobby Holley (:bholley) from comment #58)
> https://tbpl.mozilla.org/?tree=Try&rev=98dce9438d21

In a flabbergasting state of affairs, the test which failed on your push to try also failed when you pushed to inbound. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8695e3fd2d34
btw, had you included Android, you would have found that test failing in mochitest-8, if that proves to be useful information.
(In reply to Blake Kaplan from comment #55)
> > +const uint32_t kInterfaceShimCount =
> > +  sizeof(kInterfaceShimMap) / sizeof(kInterfaceShimMap[0]);
> 
> I'm sure there's either a macro or a template function that does this for us.

Except on compilers that support constexpr, the template function will cause a static initaliser here, so the macro is preferred.

Also you probably want static const so it's a true constant and not just a variable that happens to live in read-only memory.
Depends on: 856127
Depends on: 856257
Hm, this appears to be on m-c now. Should it be marked FIXED?
Flags: needinfo?(ryanvm)
Doesn't look like comment #67 made it over to m-c yet...
Flags: needinfo?(ryanvm)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #69)
> Doesn't look like comment #67 made it over to m-c yet...

Ah, right! I thought bug 856257 was a regression from flipping the pref, but it was actually a regression from one of the patches in the first set. Sorry for the confusion.
Blocks: 856424
Depends on: 858400
Release noted for FF22 as "For user privacy, the |Components| object is no longer accessible from web content".
I think we should display the warning when the Components shim is used.
https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp?rev=e1cb74a38196#4480
Currently only telemetry is accumulated.
(In reply to Masatoshi Kimura [:emk] from comment #75)
> I think we should display the warning when the Components shim is used.
> https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.
> cpp?rev=e1cb74a38196#4480
> Currently only telemetry is accumulated.

Yeah, that's probably a good idea. I'm unlikely to get to it in the near term, but I'd take a patch.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: