Last Comment Bug 790732 - Stop defining |Components| object in content scopes
: Stop defining |Components| object in content scopes
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed, relnote
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
: 848030 (view as bug list)
Depends on: 781689 792036 795275 834697 843711 848998 856127 856257 858400
Blocks: 429070 693733 794943 848030 856424
  Show dependency treegraph
 
Reported: 2012-09-12 13:09 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2013-05-29 09:42 PDT (History)
26 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
22+


Attachments
Bit-rotted WIP. (3.14 KB, patch)
2013-02-11 03:00 PST, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Part 1 - Remove the aTarget parameter from AttachComponentsObject. v1 (2.69 KB, patch)
2013-02-25 10:46 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 2 - Stop attaching Components in InitClasses. v1 (955 bytes, patch)
2013-02-25 10:48 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 3 - Remove Components warning and Telemetry. v1 (15.42 KB, patch)
2013-02-25 10:48 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 4 - Omit Components object for content scopes. v1 (2.68 KB, patch)
2013-02-25 10:49 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 3 - Make Components console warning test pref-aware. v1 (2.34 KB, patch)
2013-03-21 17:01 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 4 - Define a lazily-resolved shim for Components. v1 (5.04 KB, patch)
2013-03-21 17:01 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 5 - Omit Components object for content scopes. v2 (4.66 KB, patch)
2013-03-21 17:02 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 6 - Components shim telemetry. v1 (2.12 KB, patch)
2013-03-21 17:02 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 7 - Components shim tests. v1 (4.91 KB, patch)
2013-03-21 17:02 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-09-12 13:09:56 PDT
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.
Comment 1 Boris Zbarsky [:bz] 2012-09-12 13:56:09 PDT
> 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.
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-09-13 06:45:50 PDT
(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.
Comment 3 Boris Zbarsky [:bz] 2012-09-13 08:27:18 PDT
> 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?
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-09-13 08:36:01 PDT
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
Comment 5 Boris Zbarsky [:bz] 2012-09-13 12:09:55 PDT
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?
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-09-13 14:37:19 PDT
(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.
Comment 7 Boris Zbarsky [:bz] 2012-09-13 14:38:57 PDT
Oh, we're not doing this for chrome XBL?  In that case it might be ok, yeah.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-09-13 14:43:31 PDT
(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).
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-09-14 17:03:03 PDT
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
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2012-09-15 18:08:12 PDT
https://tbpl.mozilla.org/?tree=Try&rev=ca4b26ad9136
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-09-16 03:06:27 PDT
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?
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-09-16 03:08:01 PDT
(also, smaug says he's seen people getting XHR interface constants off of Components.interfaces.nsIXMLHttpRequest).
Comment 13 Andrew McCreight [:mccr8] 2012-09-17 11:17:04 PDT
Bobby, is this blocked by bug 781689? That bug removes some code in content that does QI and getInterface using things in Components.interfaces.
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-09-17 13:42:22 PDT
(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.
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2012-09-17 16:18:11 PDT
tps://tbpl.mozilla.org/?tree=Try&rev=2133df1c1ea8
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2012-09-18 03:31:04 PDT
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?
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2012-09-18 04:18:04 PDT
https://tbpl.mozilla.org/?tree=Try&rev=dcd7c1955e0a
Comment 18 Joel Maher ( :jmaher ) 2012-09-18 06:26:50 PDT
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.
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-09-18 06:42:02 PDT
(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).
Comment 20 Joel Maher ( :jmaher ) 2012-09-18 06:45:17 PDT
since it doesn't work in jsreftests, we might actually make it work for crashtests :)
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-09-18 06:56:35 PDT
(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.
Comment 22 Bobby Holley (:bholley) (busy with Stylo) 2012-09-18 07:29:32 PDT
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.
Comment 23 Boris Zbarsky [:bz] 2012-09-18 21:17:50 PDT
> What kind of communication are we going to need to do for this one?

You want to talk to Jorge, I think.
Comment 24 Bobby Holley (:bholley) (busy with Stylo) 2012-09-19 00:24:16 PDT
(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.
Comment 25 Olli Pettay [:smaug] 2012-09-19 01:41:10 PDT
Could you add a telemetry probe to get some more information?
Comment 26 Bobby Holley (:bholley) (busy with Stylo) 2012-09-19 01:58:29 PDT
(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...
Comment 27 Bobby Holley (:bholley) (busy with Stylo) 2013-01-16 19:50:27 PST
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!
Comment 28 Bobby Holley (:bholley) (busy with Stylo) 2013-02-11 03:00:26 PST
Created attachment 712385 [details] [diff] [review]
Bit-rotted WIP.

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.
Comment 29 Bobby Holley (:bholley) (busy with Stylo) 2013-02-18 15:14:21 PST
Now that we have XBL scopes, we no longer need any extra machinery from the JS engine to do this.
Comment 30 Bobby Holley (:bholley) (busy with Stylo) 2013-02-20 13:21:47 PST
Changing the bug summary, since the XBL scope stuff means that we can just turn it off for content and XBL will still work.
Comment 31 Bobby Holley (:bholley) (busy with Stylo) 2013-02-20 13:23:10 PST
https://tbpl.mozilla.org/?tree=Try&rev=129690661994
Comment 32 Bobby Holley (:bholley) (busy with Stylo) 2013-02-21 07:52:04 PST
Hm, not sure how that happened. Anyway, pushing again:

https://tbpl.mozilla.org/?tree=Try&rev=c6401e6e5dce
Comment 33 Bobby Holley (:bholley) (busy with Stylo) 2013-02-21 11:01:19 PST
https://tbpl.mozilla.org/?tree=Try&rev=79db1993474c
Comment 34 Bobby Holley (:bholley) (busy with Stylo) 2013-02-22 09:22:11 PST
https://tbpl.mozilla.org/?tree=Try&rev=e56bfbf6fbd1
Comment 35 Bobby Holley (:bholley) (busy with Stylo) 2013-02-25 10:46:43 PST
Created attachment 717963 [details] [diff] [review]
Part 1 - Remove the aTarget parameter from AttachComponentsObject. v1

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.
Comment 36 Bobby Holley (:bholley) (busy with Stylo) 2013-02-25 10:48:03 PST
Created attachment 717964 [details] [diff] [review]
Part 2 - Stop attaching Components in InitClasses. v1

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|.
Comment 37 Bobby Holley (:bholley) (busy with Stylo) 2013-02-25 10:48:54 PST
Created attachment 717965 [details] [diff] [review]
Part 3 - Remove Components warning and Telemetry. v1
Comment 38 Bobby Holley (:bholley) (busy with Stylo) 2013-02-25 10:49:12 PST
Created attachment 717966 [details] [diff] [review]
Part 4 - Omit Components object for content scopes. v1

\o/
Comment 39 Bobby Holley (:bholley) (busy with Stylo) 2013-02-25 10:53:35 PST
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.
Comment 40 Anthony Lieuallen 2013-02-25 14:43:18 PST
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.
Comment 41 Bobby Holley (:bholley) (busy with Stylo) 2013-02-25 14:48:03 PST
(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?
Comment 42 Anthony Lieuallen 2013-02-25 14:49:44 PST
Yes-ish.  I mean are (the source of) a user script which contains the string "Components.", not preceded by "//".
Comment 43 Blake Kaplan (:mrbkap) 2013-03-04 14:03:07 PST
Comment on attachment 717966 [details] [diff] [review]
Part 4 - Omit Components object for content scopes. v1

\o/
Comment 44 Bobby Holley (:bholley) (busy with Stylo) 2013-03-04 14:25:18 PST
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.
Comment 45 Bobby Holley (:bholley) (busy with Stylo) 2013-03-05 10:02:27 PST
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
Comment 46 Ryan VanderMeulen [:RyanVM] 2013-03-05 12:45:42 PST
Unfortunately, this appears to have caused talos-other hangs/crashes. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bef7c417aa93
Comment 48 Bobby Holley (:bholley) (busy with Stylo) 2013-03-21 16:55:18 PDT
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.
Comment 49 Bobby Holley (:bholley) (busy with Stylo) 2013-03-21 17:01:41 PDT
Created attachment 727968 [details] [diff] [review]
Part 3 - Make Components console warning test pref-aware. v1
Comment 50 Bobby Holley (:bholley) (busy with Stylo) 2013-03-21 17:01:56 PDT
Created attachment 727969 [details] [diff] [review]
Part 4 - Define a lazily-resolved shim for Components. v1
Comment 51 Bobby Holley (:bholley) (busy with Stylo) 2013-03-21 17:02:13 PDT
Created attachment 727970 [details] [diff] [review]
Part 5 - Omit Components object for content scopes. v2
Comment 52 Bobby Holley (:bholley) (busy with Stylo) 2013-03-21 17:02:26 PDT
Created attachment 727972 [details] [diff] [review]
Part 6 - Components shim telemetry. v1
Comment 53 Bobby Holley (:bholley) (busy with Stylo) 2013-03-21 17:02:41 PDT
Created attachment 727973 [details] [diff] [review]
Part 7 - Components shim tests. v1
Comment 54 Bobby Holley (:bholley) (busy with Stylo) 2013-03-21 17:05:37 PDT
https://tbpl.mozilla.org/?tree=Try&rev=f80126ad2c64
Comment 55 Blake Kaplan (:mrbkap) 2013-03-26 15:29:16 PDT
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.
Comment 56 Bobby Holley (:bholley) (busy with Stylo) 2013-03-26 15:44:19 PDT
(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.
Comment 57 Blake Kaplan (:mrbkap) 2013-03-26 15:49:24 PDT
(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.
Comment 58 Bobby Holley (:bholley) (busy with Stylo) 2013-03-26 16:42:20 PDT
(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
Comment 59 Bobby Holley (:bholley) (busy with Stylo) 2013-03-26 22:19:44 PDT
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.
Comment 60 Bobby Holley (:bholley) (busy with Stylo) 2013-03-26 22:22:20 PDT
Try push for flipping the pref:

https://tbpl.mozilla.org/?tree=Try&rev=dfabfc7a0681
Comment 61 Phil Ringnalda (:philor, back in August) 2013-03-26 23:43:36 PDT
(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
Comment 62 Phil Ringnalda (:philor, back in August) 2013-03-26 23:52:25 PDT
btw, had you included Android, you would have found that test failing in mochitest-8, if that proves to be useful information.
Comment 63 neil@parkwaycc.co.uk 2013-03-27 03:02:46 PDT
(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.
Comment 64 Bobby Holley (:bholley) (busy with Stylo) 2013-03-27 18:29:40 PDT
I thought the test (added in the push) was failing due to a loading race condition, because it didn't reproduce locally. But it looks like it was a path issue that manifested only when running the whole suite (rather than an individual test).

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e5b54db9ddb6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d5dfcbbcd176
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a2998da65e3a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b2f668e489d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a835569488a8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/68842f61d58b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9ca7defc8a0f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ab716af95256
Comment 66 Bobby Holley (:bholley) (busy with Stylo) 2013-03-28 11:17:02 PDT
https://tbpl.mozilla.org/?tree=Try&rev=323749bbcbc2
Comment 68 Bobby Holley (:bholley) (busy with Stylo) 2013-03-30 10:48:15 PDT
Hm, this appears to be on m-c now. Should it be marked FIXED?
Comment 69 Ryan VanderMeulen [:RyanVM] 2013-03-30 13:56:18 PDT
Doesn't look like comment #67 made it over to m-c yet...
Comment 70 Bobby Holley (:bholley) (busy with Stylo) 2013-03-30 15:45:09 PDT
(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.
Comment 72 Bobby Holley (:bholley) (busy with Stylo) 2013-03-31 08:15:07 PDT
*** Bug 848030 has been marked as a duplicate of this bug. ***
Comment 73 Alex Keybl [:akeybl] 2013-05-16 09:15:11 PDT
Release noted for FF22 as "For user privacy, the |Components| object is no longer accessible from web content".
Comment 74 Panos Astithas [:past] (away until 7/21) 2013-05-29 02:51:47 PDT
Can we remove ComponentsWarning now?

http://mxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/dom/dom.properties#118
Comment 75 Masatoshi Kimura [:emk] 2013-05-29 05:46:30 PDT
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.
Comment 76 Bobby Holley (:bholley) (busy with Stylo) 2013-05-29 09:42:23 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.