Bug 1054630 added parser telemetry for usage of SpiderMonkey's deprecated language extensions (like legacy generators) in real-world web content. I tried to avoid collecting parser telemetry for add-ons and chrome JS because we know they use these language extensions. Telemetry numbers for the first couple days [1] show a few hundred page-views of expression closures (shorthand function syntax). evilpie suspects some addons might be loading JS as HTTP resources, so we should attribute that JS usage to addons, not web content. Will JS loaded as an HTTP resource by an addon still have an addon compartment? I haven't actually seen a case of an addon loading JS as an HTTP resource; this is just evilpie's hypothesis. [1] http://telemetry.mozilla.org/#filter=nightly%2F34%2FJS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT&aggregates=multiselect-all!Submissions&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph

Comment on attachment 8481412 [details] [diff] [review] skip-addons.patch Review of attachment 8481412 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +7558,5 @@ > const char* filename = getFilename(); > if (!filename) > return; > > + bool isAddon = cx->compartment()->addonId != nullptr; Please remove the != nullptr bit. Also, until bug 1030420 lands, this code will miss some add-ons (i.e., isAddon will be false even though it's add-on code).

> Will JS loaded as an HTTP resource by an addon still have an addon compartment? I haven't > actually seen a case of an addon loading JS as an HTTP resource; this is just evilpie's > hypothesis. No, stuff loaded via HTTP won't be identified as part of an add-on. I would be surprised if our AMO policy allowed this to happen though. It seems extremely dangerous. Couldn't the few hundred page views just be people experimenting with new features? Nightly is a pretty bleeding-edge audience.

(In reply to Bill McCloskey (:billm) from comment #1) > > + bool isAddon = cx->compartment()->addonId != nullptr; > > Please remove the != nullptr bit. addonId is a JSAddonId pointer, so we would get warnings about assigning a pointer to a bool. (I split the isAddon logic into its own local variable because I have a later telemetry patch that uses isAddon in a more complicated expression where isAddon makes that code clearer.) > No, stuff loaded via HTTP won't be identified as part of an add-on. oh. That means invalidates this whole patch. :( Is there another way for Parser to determine that it is parsing JS code on behalf of a (non-AMO) addon that might have loaded JS via HTTP? > I would be surprised if our AMO policy allowed this to happen though. It seems > extremely dangerous. You are correct. AMO reviewers confirmed that AMO policy does not allow this (but non-AMO addons might try). > Couldn't the few hundred page views just be people experimenting with new > features? Nightly is a pretty bleeding-edge audience. Yeah, that's what we're trying to find out. The JS team is eager to remove these non-standard language features. :)

> oh. That means invalidates this whole patch. :( Is there another way for Parser to determine > that it is parsing JS code on behalf of a (non-AMO) addon that might have loaded JS via HTTP? I don't know of any way to do that. However, you could write a custom telemetry analysis that tried to correlate JS feature usage with add-ons installed. We already have all the data, and I think it's pretty easy to write map/reduce jobs over the telemetry data now. I think Mark Reid is the person to ask.

(In reply to Bill McCloskey (:billm) from comment #2) > No, stuff loaded via HTTP won't be identified as part of an add-on. I tested the following assertion in Parser and it blows up from the mochitest add-on test runner. Maybe I misunderstood what you meant by "stuff loaded via HTTP won't be identified as part of an add-on". Is the mochitest add-on a special case? bool isAddon = cx->compartment()->addonId != nullptr; bool isHTTP = strncmp(filename, "http://", 7) == 0 || strncmp(filename, "https://", 8) == 0; MOZ_ASSERT(!(isAddon && isHTTP), "JS loaded via HTTP should not be associated with an add-on");

(In reply to Chris Peterson (:cpeterson) from comment #5) > (In reply to Bill McCloskey (:billm) from comment #2) > > No, stuff loaded via HTTP won't be identified as part of an add-on. > > I tested the following assertion in Parser and it blows up from the > mochitest add-on test runner. Maybe I misunderstood what you meant by "stuff > loaded via HTTP won't be identified as part of an add-on". Is the mochitest > add-on a special case? > > bool isAddon = cx->compartment()->addonId != nullptr; > bool isHTTP = strncmp(filename, "http://", 7) == 0 || strncmp(filename, > "https://", 8) == 0; > MOZ_ASSERT(!(isAddon && isHTTP), "JS loaded via HTTP should not be > associated with an add-on"); I guess it depends on what the add-on is doing. If it does Cu.import("http://whatever"), then that code will not be identified as part of the add-on. However, if it loads the script via a <script> tag or loadSubScript (which may be what the test harness is doing), then the code will be loaded into the same compartment as the importing code, so it will be identified as part of the add-on. If you do decide to land this, please change the code to: bool isAddon = !!cx->compartment()->addonId; I don't think that any compilers will warn about this without the !! (I checked gcc and clang), but it can't hurt.