Closed Bug 767640 Opened 12 years ago Closed 6 years ago

Define Ci, Cr, Cc, and Cu whenever Components is defined for a chrome scope

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Dolske, Assigned: mccr8)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 6 obsolete files)

General idea from bug 406371 comment 5...

We use Cc/Ci/Cu/Cr "shortcut" variables all over the place in chrome code because Components.classes, Components.interfaces, etc are too tedious to type. (My fingers are already le tired from just those 2).

Could we just have a little xpconnect magic to create these globals along with Components? That would make 406371 much easier to untangle, and ensure that these shortcuts work all the time.
Yes, we should! Tricky part would be compatability issues, of course. We'd want to isolate this to chrome-code-only, for sure, and then also figure out a way to have it not clobber on existing variables that may use these names.
An easy way to do the latter would be to define them as non-permanent, so later redefinitions would overwrite the original.
Blocks: 406371
Blocks: 1230369
I started looking at this a little, just for Ci. The code isn't too complicated, but it is a little gross. For compatibility, you don't really want the JSPROP_PERMANENT, but it helped me find places that were defining it. I also wrote a script to automatically remove definitions of Ci. That removes around 1300 instances of people defining Ci. The other patch fixes a few places I noticed that get hit during startup that the script misses. Ideally you'd probably need an eslint to detect that. Once addons are no longer supported, we could then add PERMANENT, I assume.
Attached file script to remove definitions of Ci (obsolete) —
Here's the script I wrote to remove a few common ways people define Ci. You run it on a directory, with --fix if you want it to actually change the files. If you run this yourself, you'll want to edit the bit of code down near "Hacky way to not process files in the objdir." to match your objdirs.
Andrew, if you're interested in continuing the js part of this, Florian and myself would be happy to help with removing the existing definitions (eslint/script/whatever).
Flags: needinfo?(continuation)
Ok, thanks. I haven't any time to think about the best way to do this. I don't know if there's some webidl for JSM globals that could have Ci added to or if my approach of writing some gross JSAPI is the best we can do.
Flags: needinfo?(continuation)
(In reply to Andrew McCreight [:mccr8] from comment #8)
> Ok, thanks. I haven't any time to think about the best way to do this. I
> don't know if there's some webidl for JSM globals that could have Ci added
> to or if my approach of writing some gross JSAPI is the best we can do.

https://dxr.mozilla.org/mozilla-central/rev/e4107773cffb1baefd5446666fce22c4d6eb0517/dom/webidl/ChromeUtils.webidl#7-13 seems like it would be OK for this stuff too?

I don't know how gross the JSAPI thing is and if there are/aren't other (dis)advantages to either approach.

Regardless, I think it'd be a good thing if we exposed C[iucr] (and Services, XPCOMUtils and AppConstants, ideally, though that can also be a separate bug) on all chrome scopes without any imports. As Mark said, frontend folks will be *very* happy to help with automated rewriting.

Andrew, are you in a position to push this over the line?
Flags: needinfo?(continuation)
Sure, I can work on this.

Boris, is there some better WebIDL-ish way to define Cc etc. on all chrome scopes than what I did in my patch here (write some JSAPI code in XPCWrappedNativeScope::AttachComponentsObject()?
Flags: needinfo?(continuation) → needinfo?(bzbarsky)
Assignee: nobody → continuation
If we want to define Ci/Cc in exactly the cases when AttachComponentsObject happens, then doing it via webidl machinery is a bit annoying, because we can AttachComponentsObject to all sorts of globals (e.g. Window globals and opted-in sandboxes).

Furthermore, some of the globals involved where we definitely want Ci/Cc aren't even WebIDL globals (e.g. JSM globals)...  So we can't just hook this up to some webidl machinery and have it work.

You _can_ use ToJSValue instead of manually doing xpcObjectHelper and NativeInterface2JSObject.  

If you don't care about the sandbox and Window cases, you could perhaps do the work lazily in dom::SystemGlobalResolve or something.
Flags: needinfo?(bzbarsky)
I haven't really tested this, because the browser doesn't start with it, so you need to fix it up everywhere at the same time. Maybe I can drop the READONLY part.

MozReview-Commit-ID: 6FV9ahLeqUb
Attachment #8859333 - Attachment is obsolete: true
Attachment #8859334 - Attachment is obsolete: true
I got it at least semi working with my patch, and with some removals. I had to remove READONLY so that the various existing places that do define it will work.

There are also a few nuances to removal. First, I gave up on doing anything for .js files, because some are run as content (I guess). For JSMs, there's at least one place where it defines Cu or whatever inside a framescript or something, so you can't delete a definition of Cu etc. if it has any white space before it. Thirdly, testing/specialpowers/content/MockFilePicker.jsm broke if I removed the definitions. I don't understand why. Maybe it runs as content? Anyways, this is at least something to work with.
MozReview-Commit-ID: 6FV9ahLeqUb
Attachment #8943808 - Attachment is obsolete: true
MozReview-Commit-ID: 4PyMh0WeA4r
Comment on attachment 8943808 [details] [diff] [review]
Put Ci, Cr, Cc, and Cu on chrome contexts.

You probably don't want JSPROP_RESOLVING there.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #16)
> You probably don't want JSPROP_RESOLVING there.

Thanks, I removed that. I just copied the list of props from what was being done for Components and have been slowly removing them...

One odd thing is that the use of Cu by formautofill.xml seems to break, even without the removal of any existing definitions of Cu. Somehow defining the property must interfere with defining it as a var in the xul file it derives from. There's some weirdness around Components and whether you have high or low privilege access (utils is on the high privilege part), which maybe the JSPROP_READONLY | JSPROP_RESOLVING stuff that is done for Components (but that I don't do for Cu because it breaks things) interferes with it. Chromeutils.import also works.

I could obviously just fix this where I find it by not using the shorthand but it seems like a bad idea to mysteriously change the behavior of Cu like this without any understanding of the reason.
Here's the try run (without any removal of existing Cu, etc.): https://treeherder.mozilla.org/#/jobs?repo=try&revision=811865ec0d156d9f6a9e88abc1f4c8173d12468e
It looks like only form fill and some Marionette thing are broken.
(In reply to Andrew McCreight [:mccr8] from comment #18)
> Here's the try run (without any removal of existing Cu, etc.):
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=811865ec0d156d9f6a9e88abc1f4c8173d12468e
> It looks like only form fill and some Marionette thing are broken.

Matt or Jared, do you know why form fill would be specifically impacted by this bug? See also the earlier comments.
Flags: needinfo?(jaws)
Flags: needinfo?(MattN+bmo)
Depends on: 1432576
Depends on: 1432578
My patch appears to not actually work correctly. That may be somehow causing the problems with form fill and Marionette. I'll figure out why the basic functionality isn't working before worrying about the other weirdness.

Anyways, for some reason, Cu.import is not defined until I evaluate Components.utils. I'm poking around in XPConnect code right now but I'm not sure exactly why the behavior is like this.

This is the result of doing a series of dump() statements:
  Cu.import = undefined
  Cu = [object nsXPCComponents_Utils]
  Components.utils = [object nsXPCComponents_Utils]
  Cu.import = function import() {
      [native code]
  }

At first, Cu.import is undefined, even though Cu is a nsXPCComponents_Utils as expected. But then after evaluating Components.utils, suddenly Cu.import works as expected.
Flags: needinfo?(jaws)
Flags: needinfo?(MattN+bmo)
> At first, Cu.import is undefined, even though Cu is a nsXPCComponents_Utils as expected.

Oh.  I can tell you why that is, now that I think about it.  Cu doesn't have classinfo, so doesn't get interface flattening.  That means that to get the things from nsIXPCComponents_Utils defined on it, you have to JS-wrapt it as an nsIXPCComponents_Utils.

This means a few things:

1)  Using ToJSValue will not work.
2)  You can use NativeInterface2JSObject, or dom::WrapObject or various other
    things (dom::WrapObject may be the one with the simplest signature), but
    you will need to pass the right IID for the iid argument.  Specifically,
    the IID for nsIXPCComponents_Utils.
Attachment #8859333 - Attachment is obsolete: true
Attachment #8859334 - Attachment is obsolete: true
Attachment #8859333 - Attachment is obsolete: false
Attachment #8859334 - Attachment is obsolete: false
Attachment #8943819 - Attachment is obsolete: true
This seems to work, at least with what part of my try run has run so far.

I just noticed the part of comment 9 about also wanting Services, XPCOMUtils and AppConstants, and I wonder if maybe a better approach would be to set up a JS script to be run any time we create a chrome JS global. I'm not sure how that would work, but it would be nice because then it could be updated by people who are writing chrome JS, rather than requiring some gross XPConnect C++ code. It might be slower, but at least we create less globals now due to the shared JSM loading.
(In reply to Andrew McCreight [:mccr8] from comment #26)
> This seems to work, at least with what part of my try run has run so far.
> 
> I just noticed the part of comment 9 about also wanting Services, XPCOMUtils
> and AppConstants, and I wonder if maybe a better approach would be to set up
> a JS script to be run any time we create a chrome JS global. I'm not sure
> how that would work, but it would be nice because then it could be updated
> by people who are writing chrome JS, rather than requiring some gross
> XPConnect C++ code. It might be slower, but at least we create less globals
> now due to the shared JSM loading.

I think for perf reasons I would still prefer a native/webidl/js-engine solution, but if it'd be much easier/faster to implement this with an "all privileged globals get this script" solution then I'd still love to have that purely for the resulting chrome JS code cleanup, and we could potentially convert that solution to a compiled one in a separate bug if we deemed that would net us some perf improvement.

I don't know what order of magnitude our global creation is up to at this point what with shared-globals for JSMs. I guess I'd expect some additional ones for content processes and so on, but I think JS components still get individual globals, and so perhaps it'd still be "more than we might like", especially once we get up to 5+ processes in total.
In case you find it easier to review this as a splinter patch (though please mark an r+ in mozreview so I can autoland it). The interesting parts are probably only the linter stuff.
Summary: Define Cc/Ci/etc whenever Components is defined for a chrome scope → Define Ci, Cr, Cc, and Cu whenever Components is defined for a chrome scope
Attachment #8859337 - Attachment is obsolete: true
Attachment #8859337 - Attachment mime type: text/x-python → text/plain
Blocks: 1432992
Comment on attachment 8859333 [details]
Bug 767640, part 1 - Put Ci, Cr, Cc, and Cu on chrome contexts.

https://reviewboard.mozilla.org/r/131362/#review221220

Thanks for doing this! r+ because I don't want to block this and it looks OK but I'm wondering if it wouldn't be a better idea to land here only the eslint changes, and do a smarter script in bug 1432992 for the mass cleanup.

The things I think we can do better:
- handle all files (including .js, .xul) For example https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/browser/components/places/content/placesOverlay.xul#23-26 looks like it really wants to be removed :-).
- the current patch leaves 2 consecutive empty lines in many files, I think we could avoid this with a smarter script.
- handle the const {classes: Cc, utils: Cu, interfaces: Ci} = Components; form.
- Mass replace the remaining Components.interface/Components.utils uses with Ci/Cu (I'm hesitant about Components.classes -> Cc because I'm not sure we have a standard way to deal with the indent). And after doing this, covering it with eslint would make sense.

I can help if you would like to do this with an xpcshell based script rather than one based on regexps.

FYI, your script-generated patch will bitrot significantly with bug 1431533 which is likely to reland soon.
Attachment #8859333 - Flags: review+
(In reply to Florian Quèze [:florian] from comment #32)
> Thanks for doing this! r+ because I don't want to block this and it looks OK
> but I'm wondering if it wouldn't be a better idea to land here only the
> eslint changes, and do a smarter script in bug 1432992 for the mass cleanup.

Yeah, I was thinking that myself. I actually only fixed some cases because I was trying to make part 2 not as big. I wanted to make sure that the initial landing would actually use the new things it defined, but the breakage I saw from an incorrect version suggests that it is being tested somewhat even without it.

> The things I think we can do better:
> - handle all files (including .js, .xul)
Yes, this is easy. I only did .jsms just to make the patch a little smaller for the initial landing.

> - the current patch leaves 2 consecutive empty lines in many files, I think
> we could avoid this with a smarter script.
That's a good point. eslint was complaining about multiple blank lines in a few places, and I used eslint --fix to clear that up, but I see that there are still a lot of them.

> - handle the const {classes: Cc, utils: Cu, interfaces: Ci} = Components;
> form.
Yeah, my script actually does deal with that.

> - Mass replace the remaining Components.interface/Components.utils uses with
> Ci/Cu (I'm hesitant about Components.classes -> Cc because I'm not sure we
> have a standard way to deal with the indent). And after doing this, covering
> it with eslint would make sense.
That's a good idea, though it seems like something you'd want to do after the other stuff.

> I can help if you would like to do this with an xpcshell based script rather
> than one based on regexps.
The current one works well enough. What do you mean by an "xpcshell base script"?

> FYI, your script-generated patch will bitrot significantly with bug 1431533
> which is likely to reland soon.

Ah, right. I'll wait for that to settle out then before landing any fixes.
(In reply to Andrew McCreight [:mccr8] from comment #33)

> > I can help if you would like to do this with an xpcshell based script rather
> > than one based on regexps.
> The current one works well enough. What do you mean by an "xpcshell base
> script"?

I mean something using our actual JS parser rather than regexps. I have plenty of examples in https://bitbucket.org/fqueze/xpcshell-rewrites/src (used for eg. bug 1421992 or bug 1374282)
Comment on attachment 8859334 [details]
Bug 767640, part 2 - Remove definitions of Ci, Cr, Cc and Cu from the top level of .jsms.

https://reviewboard.mozilla.org/r/131364/#review221268

Looks like my r+ in comment 32 got attached to the wrong patch. Sorry about that.
Attachment #8859334 - Flags: review?(florian) → review+
Attachment #8859333 - Flags: review+
Attachment #8945267 - Attachment is obsolete: true
Attachment #8945268 - Attachment is obsolete: true
Attachment #8945268 - Attachment mime type: text/x-python → text/plain
Attachment #8859334 - Attachment is obsolete: true
Sorry for the noise here. I dropped part 2, then realized we still needed the eslint changes, and I guess mozreview forgot about the r+....
Comment on attachment 8945524 [details]
Bug 767640, part 2 - Update eslint rules to take into account the definitions of Ci, Cr, Cc and Cu.

https://reviewboard.mozilla.org/r/215672/#review221346
Attachment #8945524 - Flags: review?(florian) → review+
Comment on attachment 8859333 [details]
Bug 767640, part 1 - Put Ci, Cr, Cc, and Cu on chrome contexts.

https://reviewboard.mozilla.org/r/131362/#review221534

r=me woth the nits.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp:231
(Diff revision 5)
>  
> +static bool
> +DefineSubcomponentProperty(JSContext* aCx,
> +                           HandleObject aGlobal,
> +                           nsISupports* aSubcomponent,
> +                           const nsID* aIid,

aIID is the usual spelling.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp:241
(Diff revision 5)
> +    if (!XPCConvert::NativeInterface2JSObject(&subcompVal, helper,
> +                                              aIid, false, nullptr))
> +        return false;
> +    if (NS_WARN_IF(!subcompVal.isObject()))
> +        return false;
> +    RootedObject subcompObj(aCx, &subcompVal.toObject());

Why do you need subcompObj?  You should be able to pass the RootedValue to JS_DefinePropertyById.
Attachment #8859333 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #41)
> aIID is the usual spelling.
Ah, right. I thought aIid looked horrible.

> Why do you need subcompObj?  You should be able to pass the RootedValue to
> JS_DefinePropertyById.
I didn't think about how an overload might exist. I'll fix that.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6bc5ccfb1bd3
part 1 - Put Ci, Cr, Cc, and Cu on chrome contexts. r=bz,florian
https://hg.mozilla.org/integration/autoland/rev/073aecb9bee5
part 2 - Update eslint rules to take into account the definitions of Ci, Cr, Cc and Cu. r=florian
Oops. I saw some talos failures in my try run with the removal included, but I didn't think that it would affect the version without removal. Talos is apparently the last code that uses enableUniversalXPConnect, and then it rebinds Components, so maybe that's related.
What is happening here is that EnableUniversalXPConnect causes us to call XPCWrappedNativeScope::AttachComponentsObject() on a global a second time. When this happens, Ci is already defined. When trying to call JS_DefinePropertyById for Ci a second time, it fails for some reason. For the components object, it sets the JSPROP_RESOLVING attr, and when I do that for my new Ci, etc. things at least the one Talos test seems to pass. I tried reading the comment for JSPROP_RESOLVING, but I don't understand what it means. Boris, is it reasonable for me to pass in this flag for the Ci, Cc, Cu, Cr properties?
Flags: needinfo?(bzbarsky)
The only point of JSPROP_RESOLVING is to skip the resolve hook.

It _should't_ matter in cases when you're not under a resolve hook to start with.  

If passing JSPROP_RESOLVING does make things work, I'd really like to find out why (and in particular why the version without JSPROP_RESOLVING fails).
Flags: needinfo?(bzbarsky)
enableUniversalXPConnect seems to work just fine in a plain Mochitest. I need to figure out what Talos is doing differently that is causing it to fail.
I managed to get a full error message finally, by popping open the browser console while it was stalled out. For some reason, the browser output was squelched. Anyways, I saw this there:
  TypeError: can't redefine non-configurable property "Ci"  MozillaFileLogger.js:35:1

where line 35 is this:
netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

which is followed by:
if (Cc === undefined) {
  var Cc = Components.classes;
  var Ci = Components.interfaces;
}

If I delete the whole if block, then it works.

I guess the "var" declarations of Cc and Ci are being hoisted to the top level, and run before enablePrivilege. Ah, I see from the MDN page on var that "Declared variables are a non-configurable property of their execution context", so I guess it all makes sense... I guess if I pass in JSPROP_RESOLVING it is somehow missing the var...? I guess this is only a problem in Talos because Talos is the only place that uses enablePrivilege, so it is the only place that attempts to overwrite the var. In other places, I guess it is somehow okay to overwrite the result of JS_DefinePropertyById() with var.

I think the fix here is just to go through and clear out all of these definitions of Cc etc. that happen after enablePrivilege.
> I guess the "var" declarations of Cc and Ci are being hoisted to the top level

Yes.

> and run before enablePrivilege

A var at global scope (as opposed to function scope; if blocks do not create a scope for var) defines the property at script evaluation time, before any line of the script is actually run.  The property value assignment happens at the point where the assignment appears.

In other words, when line 35 is running, "Cc" is a non-configurable property on the global, but its value at that point is `undefined`.

Moving the enablePrivilege call to after those lines won't help.  Eliminating those var lines altogether would.

> I guess it is somehow okay to overwrite the result of JS_DefinePropertyById() with var.

"var" with a property name that is already defined simply doesn't define it again; the assignment happens when the line with the "= whatever" is reached....

One thing we could try doing is using the JS::PropertyDescriptor version of JS_DefinePropertyById, with a descriptor that does what Object.defineProperty with no attrs specified would do... (that is, use the existing property attributes but change the value).   But I'm not seeing an obvious way to set up such a descriptor.
Flags: needinfo?(jwalden+bmo)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #52)
> "var" with a property name that is already defined simply doesn't define it
> again; the assignment happens when the line with the "= whatever" is
> reached....
Ah, that makes sense. Thanks for the explanations.

> One thing we could try doing is using the JS::PropertyDescriptor version of
> JS_DefinePropertyById, with a descriptor that does what
> Object.defineProperty with no attrs specified would do... (that is, use the
> existing property attributes but change the value).   But I'm not seeing an
> obvious way to set up such a descriptor.

I think this is only an issue with enablePrivilege("UniversalXPConnect"), because normal these properties are defined before script evaluation time, so I don't know if it is worth worrying about. From my code inspection, this one file is the only place it is a problem. Talos is the only thing using UniversalXPConnect to any substantial extent.
This is green with the deletion of var Cc and var Ci:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4ecba556b86482ce7583cfd24d9f03bad72b7bb

Boris, are you okay with me working around the one place we hit this problem, or would you prefer I pursue a more thorough solution along the lines of what you discussed at the end of comment 52? Like I said, I think this is only a problem when using UniversalXPConnect.
Flags: needinfo?(bzbarsky)
Working around is fine, esp. since we are trying to kill off enablePrivilege("UniversalXPConnect") anyway.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(jwalden+bmo)
Joel, please review the Talos changes, as trivial as they are.
Comment on attachment 8859333 [details]
Bug 767640, part 1 - Put Ci, Cr, Cc, and Cu on chrome contexts.

https://reviewboard.mozilla.org/r/131362/#review223072

talos changes look good
Attachment #8859333 - Flags: review?(jmaher) → review+
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a74c56caa0dc
part 1 - Put Ci, Cr, Cc, and Cu on chrome contexts. r=bz,florian,jmaher
https://hg.mozilla.org/integration/autoland/rev/e51fa5f76367
part 2 - Update eslint rules to take into account the definitions of Ci, Cr, Cc and Cu. r=florian
https://hg.mozilla.org/mozilla-central/rev/a74c56caa0dc
https://hg.mozilla.org/mozilla-central/rev/e51fa5f76367
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Is this a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=1135220?  Or is this subtly different?
Flags: needinfo?(continuation)
Looks the same to me.
Flags: needinfo?(continuation)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #52)
> One thing we could try doing is using the JS::PropertyDescriptor version of
> JS_DefinePropertyById, with a descriptor that does what
> Object.defineProperty with no attrs specified would do... (that is, use the
> existing property attributes but change the value).   But I'm not seeing an
> obvious way to set up such a descriptor.

JS_DefinePropertyById with an appropriately set up JS::Handle<JS::PropertyDescriptor> argument should let you do this, if you end up wanting to.  Given this bug is closed, I'm guessing nothing I say in answer here is going to exactly be important.

Worth noting, tho -- if C[ciru] are defined as global constants, *and* they are defined as such in the same manner as |const| variables, then there are complicated interactions as to how such things can be redefined (lexical variables live in a separate object from the global object where vars live).  Off the top of my head, I don't quite remember just what you can do as far as redefining-while-only-changing-value goes, for such beasts.
> with an appropriately set up
> JS::Handle<JS::PropertyDescriptor> argument should let you do this

Fwiw, I tried to figure out how, and the only way I see is explicitly going against what the API documentation says and hoping that it actually works...
Depends on: 1437242
See Also: → 1442006
Blocks: 1442047
You need to log in before you can comment on or make changes to this bug.