Closed Bug 675014 Opened 14 years ago Closed 13 years ago

Statically analyze extension and Firefox code for bad chrome->content access patterns

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

Product:
Developer Infrastructure
Component:
Source Code Analysis
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: benjamin, Assigned: dherman)

Details

This is the static analysis analog of bug 666713: we're analyzing for patterns of chrome->content script access which will no longer be possible once we have content processes.
Status update: We've got the tool running on some addons and providing plausible results. We were going to do a test run on all addons today for the first time but ran out of time. One limitation: we aren't going to be able to handle E4X or sharp-literals, so the analysis won't get results for addons that use them. But we've implemented all the other SpiderMonkey extensions. We'll be doing our first trial runs across the full set of addons early next week, and I'll post an update when we see what the results look like. Dave
Status update: We ran the tool last night on the ~6k addons we found on AMO. It crashed on ~1k of them, so we'll focus first on those problems (many of them look like pretty silly and easily fixable errors). Of the ~5k it succeeded on, it found ~3k to be "definitely" safe for content processes. (This isn't an absolute guarantee of safety, but the tool is fairly conservative.) That sounds bad, but it's just the first run-through. We'll look for what kinds of obvious patterns it's being over-conservative on, and see if we can push that number higher. Dave
FWIW, a lower bound of 50% e10s compatibility out of the gate doesn't sound as bad to me as it does to you. :) Adding gavin since he'll be coordinating front end e10s work
> FWIW, a lower bound of 50% e10s compatibility out of the gate doesn't sound > as bad to me as it does to you. :) Just keep in mind the lower bound is not absolutely certain: false negatives are a possibility, although we expect them to be pretty rare. I'm running the tool again overnight tonight with a few issues fixed. I don't expect the numbers to be hugely different, but hopefully this will fix a couple hundred of the previously failing analyses. I'll post a link tomorrow where people can download the current results. Dave
I've posted the results of last night's run (which only fixed a few dozen of the failing cases, unfortunately) at http://people.mozilla.org/~dherman/e10s/e10s-results-2011-08-18.tar.bz2 Each subdirectory contains five files: * xpi - text file containing URL to the XPI file in question * evts - the summary of the analysis * result - indication of whether the analysis succeeded, failed with an error, or timed out * all.js - text file containing concatenation of all the JS source code extracted from the XPI file (this is what the analysis ran over) * coords.json - source coordinate map linking the sections of all.js back to the portions of the individual files in the XPI file where that JS code came from The one you'll care about is evts. It's currently in a **** format that's human-readable but not particularly machine-readable. Soon I'm going to modify it to generate JSON instead. Dave
FYI, the next main tasks on my list are: - also download and analyze JAR files (can anyone help me understand the naming conventions for these files? I thought before that *-fx.xpi was sufficient, but I'm seeing e.g. http://ftp.mozilla.org/pub/mozilla.org/addons/7/qute-4.1.1-fx-linux.jar so apparently *-fx-$ARCH.jar is also possible) - partition the addons into three sets to get more useful statistics: * those that contain no JS code at all * those that contain JS with features we can't handle (E4X and sharp literals) -- I can pretty easily use Reflect.parse to detect these * those that have JS code and that we successfully analyze * those that have JS code but where the analysis crashed - reformat the analysis results as JSON - produce source location information in the analysis results, appropriately mapped back to files in the XPI (as opposed to the line number in all.js) - try to cut down on the number of crashes Dave
(In reply to Dave Herman [:dherman] from comment #5) > I've posted the results of last night's run (which only fixed a few dozen of > the failing cases, unfortunately) at > > http://people.mozilla.org/~dherman/e10s/e10s-results-2011-08-18.tar.bz2 I decided to check out my results for Flagfox. It seems that you're running your tests on extremely out-of-date addon versions. "results/5791/xpi" shows that it ran on Flagfox version 3.3.5 (December 4, 2008). The current Flagfox version is 4.1.5 (July 7, 2011). That's 3 years and two major versions back, including a complete rewrite from scratch for Flagfox 4. You seem to have a problem in whatever you're using to pull down the XPIs from AMO. If you're running these tests on lots of very old versions like this then you're going to get very skewed results.
I've got two guesses on what might be causing the old versions to be used: 1) Flagfox 3.3.5 was the last version before I added SeaMonkey 2 support. As a result, the filename format used by AMO changed from "flagfox-version-fx.xpi" to "flagfox-version-fx+sm.xpi" and more recently "flagfox-version-sm+fx.xpi" for some odd reason. Maybe you're running a check on the filenames for Firefox support and it's getting tripped up on this? 2) Due to some AMO bug that I forget the specifics of, all old Flagfox versions from the first on AMO, 3.0.1, to 3.3.5 ended up with an updated last modified timestamp of 08-Jan-2009 12:56 when viewed on FTP. All newer are correct, though the next two versions have older dates than that listed. (I forget how they mangled this) Everything since February 2009 looks normal.
> You seem to have a problem in whatever you're using to pull down the XPIs > from AMO. Indeed. I was having trouble getting information about what exactly the format of the filenames was, but I've just gotten help from fligtar and jbalogh. I was filtering for Firefox-only versions, i.e., matching only: /-fx\.xpi$/ and then picking the most recent one. But the flaw there was for addons like Flagfox that don't have a Firefox-only version, or that stopped doing a Firefox-only version at some point. So I think the best thing to do is grep for: /-[^-]*fx[^-]*(-[a-z0-9]+)?\.(xpi|jar)$/ \----------/\-----------/\---------/ apps platform extension and then pick the most recent one. Thanks for raising this issue, Dave PS For reference, the format is described in the AMO code here: https://github.com/jbalogh/zamboni/blob/master/apps/files/models.py#L163
Fresh results, which include all JAR files in addition to XPIs, are uploading as I speak to: http://people.mozilla.org/~dherman/e10s/e10s-results-2011-08-21.tar.bz2 I haven't run the numbers yet but there are now 7493 total addons in the analysis and 6386 succeeded to produce an analysis. Dave
Oh, and JFYI I've added the download script to source control: https://github.com/mozilla/doctorjs/blob/master/bin/download-addons Dave
Taking a look at my evt file, I see that the events declared safe are the ones that are both "Attached on" and "Came from" chrome, however most of the events listed there list "any" for both columns and thus register as unsafe. All are addEventListener used on a XUL widget in chrome, however, so am I correct in saying that these are all false positives or does the event used matter?
> All are addEventListener used on a XUL widget in chrome, however, so am I > correct in saying that these are all false positives or does the event used > matter? The events do matter sometimes, but only for identifying uses of addEventListener that are always safe (e.g., "popupshowing"). There are false positives showing up in yours (thanks again for the bug report!) but in this case it looks like it might be a bug in the static analysis engine. The newFlagInstance function is only ever being called on a chrome window and DoctorJS ought to have figured that out. We'll investigate. Thanks! Dave
(In reply to Dave Herman [:dherman] from comment #13) > There are > false positives showing up in yours (thanks again for the bug report!) but > in this case it looks like it might be a bug in the static analysis engine. > The newFlagInstance function is only ever being called on a chrome window > and DoctorJS ought to have figured that out. We'll investigate. My guess is there are also a few misses as well. Flagfox polls for change of the 'window.content.document' object for each chrome window to detect when the content changes which I suspect would be problematic (a while back I tried to switch to an event-based system but there were bugs, at least in older versions, so I just ended up keeping the old method) and I access the content document object in a few other places as well (the var 'contentDoc' in flagfox.jsm is always a reference to the current 'window.content.document').
(In reply to Dave Garrett from comment #14) > (In reply to Dave Herman [:dherman] from comment #13) > > There are > > false positives showing up in yours (thanks again for the bug report!) but > > in this case it looks like it might be a bug in the static analysis engine. > > The newFlagInstance function is only ever being called on a chrome window > > and DoctorJS ought to have figured that out. We'll investigate. > > My guess is there are also a few misses as well. Flagfox polls for change of > the 'window.content.document' object for each chrome window to detect when > the content changes which I suspect would be problematic (a while back I > tried to switch to an event-based system but there were bugs, at least in > older versions, so I just ended up keeping the old method) and I access the > content document object in a few other places as well (the var 'contentDoc' > in flagfox.jsm is always a reference to the current > 'window.content.document'). As far as I have seen, all the eventListeners are attached to the chrome objects. Like window, icon and menu in flagfox.jsm module; where the last two of them correspond to the XUL elements. Currently the analysis only look at the objects to which the eventListeners have been attached. Since no eventListeners have been attached to the 'contentDoc' or any HTML elements of the contentDoc, we are not taking the usage of that into consideration. If the FlagFox polls for the changes in the content instead of relying on events, I think that should not be problematic (based on our current heuristics). However someone more deeply involved with e10s might be able to give an accurate answer.
(In reply to Rezwana Karim from comment #15) > If the FlagFox polls for the changes in the content instead of relying on > events, I think that should not be problematic (based on our current > heuristics). However someone more deeply involved with e10s might be able to > give an accurate answer. Just to be clear, you're saying that this is not currently flagged, but will be later, yes? Because that's definitely broken in the e10s model.
You don't need to worry about themes (which is what you mean by JARs, I think). I'll be running this on testcases today, but I'm a little worried that all the distinctions in the original problem statement aren't being caught.
> Just to be clear, you're saying that this is not currently flagged, but will > be later, yes? Because that's definitely broken in the e10s model. The scope of the project is more limited: we're not implementing a general-purpose tool that proves compatibility with e10s; we're building a tool that checks for calls to addEventListener that could be incompatible with e10s. Dave
What about expanding the scope to also cover direct calls to content DOM that would be incompatible? (possibly in a followup bug if need be) That seems within the broad scope of the initial goal here, at least.
The original scope of both the dynamic and static analysis was: We want to identify: non message-manager scripts which touch a "content object" or implement nsIWebProgressListener Definitions: Non message-manager script is any chrome script that isn't run through nsIChromeFrameMessageManager.loadFrameScript. Content objects are at least the following: Any DOM object (window, document, or node) Docshell or docshell derivative nsIWebProgress nsIDOMWindowUtils After some IRC discussion, the current static analysis doesn't currently distinguish between message manager scripts and "normal" chrome scripts: in order to use this tool as a feedback loop for addon authors we'll need to refine that.
> You don't need to worry about themes (which is what you mean by JARs, I > think). I haven't been able to get a clear story on what the .jar files in ftp.mozilla.org/pub/mozilla.org/addons are (some older format of addons? themes?), or whether that repo contains themes or not. But I think it's not a big deal because the analysis tool extracts all the JS code in each archive and if it doesn't find any, we'll just exclude it from the final results. Or are there some archives that *do* contain JS but shouldn't be analyzed? > The original scope of both the dynamic and static analysis was: > > We want to identify: > > non message-manager scripts > which touch a "content object" > or implement nsIWebProgressListener > > Definitions: > > Non message-manager script is any chrome script that isn't run through > nsIChromeFrameMessageManager.loadFrameScript. I did a grep over all addons and only found 13 instances of loadFrameScript: 1865 207602 4664 324976 15093 200733 161778 199790 52490 10229 8542 7036 161674 This does not seem particularly important relative to other sources of false positives. > After some IRC discussion, the current static analysis doesn't currently > distinguish between message manager scripts and "normal" chrome scripts: in > order to use this tool as a feedback loop for addon authors we'll need to > refine that. We have only ever talked about using this for internal metrics; I was not aware anyone was considering deploying this tool to authors. Dave
> What about expanding the scope to also cover direct calls to content DOM > that would be incompatible? (possibly in a followup bug if need be) That > seems within the broad scope of the initial goal here, at least. This is feasible in theory, but should be broken out into a separate bug. This bug could be turned into a tracking bug. Dave
(In reply to Dave Herman [:dherman] from comment #13) > > All are addEventListener used on a XUL widget in chrome, however, so am I > > correct in saying that these are all false positives or does the event used > > matter? > > The events do matter sometimes, but only for identifying uses of > addEventListener that are always safe (e.g., "popupshowing"). There are > false positives showing up in yours (thanks again for the bug report!) but > in this case it looks like it might be a bug in the static analysis engine. > The newFlagInstance function is only ever being called on a chrome window > and DoctorJS ought to have figured that out. We'll investigate. > > Thanks! > > Dave I think we can't do better in Flagfox than we currently do. There is only one call to newFlagInstance from inside Flagfox.init. To find that the argument of newFlagInstance is chrome, the analysis must find that the argument to Flagfox.init is chrome. There is only one call to Flagfox.init: scope.Flagfox.init(window); However, the analysis can't figure out that the Flagfox property of scope is the correct object, because the property seems to be assigned somewhere outside the addon's code. Therefore, the analysis can't assume anything about the argument to Flagfox.init, so it can't find that it's chrome.
(In reply to Dimitris Vardoulakis from comment #23) > I think we can't do better in Flagfox than we currently do. > > There is only one call to newFlagInstance from inside Flagfox.init. To find > that the argument of newFlagInstance is chrome, the analysis must find that > the argument to Flagfox.init is chrome. There is only one call to > Flagfox.init: > scope.Flagfox.init(window); > > However, the analysis can't figure out that the Flagfox property of scope is > the correct object, because the property seems to be assigned somewhere > outside the addon's code. Therefore, the analysis can't assume anything > about the argument to Flagfox.init, so it can't find that it's chrome. Oh, so you mean this queue to start up in each chrome window in overlay.xul is confusing it: <script type="application/javascript"> window.addEventListener("load", Flagfox_loadForThisWindow, false); function Flagfox_loadForThisWindow() { window.removeEventListener("load", Flagfox_loadForThisWindow, false); let scope = {}; Components.utils.import("resource://flagfox/flagfox.jsm",scope); scope.Flagfox.init(window); } </script> Rather than just dumping the 'Flagfox' object into the window's global scope I load it to a local variable just for the moment I need it. Components.utils.import() doesn't have a way to import locally only; it's either global or into a specified object.
This is now irrelevant.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
Product: Core → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.