By: TwitterButtons.com

Recent blog posts

User login

Home | Blogs | Stephane Eyskens's blog

My top 10 sanity checks when performing SharePoint code review

1. Checking for memory leaks

Memory leaks are the big evil but most of the times, it is important to have a pragmatic approach when hunting them .

First, a short reminder of what a memory leak is: a memory leak is a space in memory that was not released after use. Before hunting them, it is important to focus on the parent process that executes them. Sometimes, I see people trying to find memory leaks in PowerShell scripts…While it is always a best practice to release the memory as soon as you are done with it, this is not an absolute necessity when dealing with scripts since they are most of the time executed during a short amount of time and when PowerShell.exe stops running, the memory associated with the process gets released automatically.

So, I would definitely not spend time in a large-scale SharePoint implementation on chasing memory leaks in PowerShell scripts.

Here are the top 3 components that must be monitored closely according to me:

The Timer Jobs: since they are executed every xx minute/hours/days/weeks, they can, therefore, regularly cause new leaks. Moreover, they are executed by OWSTIMER, a permanent process running on the server(s), thus not releasing memory until it stops or until you explicitly dispose of your objects (simplification here…).

Farm deployed WebParts/WebControls: and basically all the controls that are handled by w3wp.exe for the same reason as the above. Although w3wp.exe are usually recycled automatically by IIS, it’s important to make sure they are not recycled unexpectedly because of your components.

SandBoxed Solutions:again, they are executed by permanent processes running on the server(s). With SandBoxes, SharePoint already controls the resources used but it’s still necessary to review the code.

Of course, to find those memory leaks, it’s always handy to have a tool and this tool is quite famous now in the SharePoint world and is called SPDisposeCheck.

http://archive.msdn.microsoft.com/SPDisposeCheck


2. Checking the number of SPSite and SPWeb objects that are instantiated

As you probably know, instantiating SPSite & SPWeb objects require a lot of resources, therefore, I tend to instantiate them only when absolutely required. So, I’m looking for patterns like this one:

class Utils  
{  
  public string GetWebTitle(string url)  
  {  
    using(SPSite Site = new SPSite(url))  
    {  
      using(SPWeb Web=Site.OpenWeb())  
      {  
       return Web.Title;  
      }
    }  
  }  
  public string GetWebList(string url, Guid listid)  
  {  
     using(…)//same as above  
  }   
} 

This is typically the type of code blocks that should be avoided because that kind of Utils class will cause too many instantiations. Rather than passing the web url in parameter, I would pass the SPWeb object or pass it to the constructor of the Utils class and reuse it in all the methods so that it gets instantiated by the caller only once per class use.

Also, as a rule of thumb, I privilege the usage of contextual objects such as SPContext.Current.Site/Web/etc… and site/web from properties objects in event handlers, features etc…


3. Looking for SPSecurity.RunWithElevatedPrivileges calls

While this is sometimes the only solution (along with impersonating with the site collection admin user token), I want to make sure that the application layer really needs to perform this impersonation.

I see, too often, developers using that statement just to make sure they will not encounter security issues. First, they might still face security issues because, this call does nothing but take the application pool’s identity which does not have entire power over the farm (otherwise it’s scary)…Secondly, I also see that kind of calls in Console Apps or feature receivers where this should absolutely be avoided because Console Apps have no web context and because feature receivers don't always have a web context (when activated from PowerShell for instance).

A good way to avoid impersonation is to scope your features correctly (see next section).

As you will notice, impersonation is often misunderstood and not always used appropriately. This causes not only a potential security hole but also a performance overhead.


4. Checking that the features are correctly scoped

Do not ask a site collection feature to perform operations at the web application level. In that case, scope your feature to the webapp directly…This way, you can reasonably assume that the identity that will activate the feature has the required permission to do so.

To me, all the operations that target the SPWebApplication and SPFarm objects should be controlled by admin/infra guys to make sure you do not destroy everything with your custom solution.


5. Checking that features are PowerShell aware
Nowadays SharePoint gets more and more use within the Enterprise. Thanks to that, a lot of companies start having good industrialization processes and more and more, the admin/infra teams emit a series of exigencies when they receive a solution package.

Most of the times, they will build some kind of PowerShell frameworks that will automate the deployment process and thus, the feature activation. In that case, if your features are not PowerShell aware, it could just result in a failure.

What do I mean by PowerShell aware? For example, say you have a feature with a feature receiver. Inside that feature receiver, you execute that line of code:

SPServiceContext.Current

because you need it to access the User Profiles or whatever…This will work when the feature is activated from the web interface but this will typically fail when activated from PowerShell since this object will be null…


6. Web.config changes
SharePoint is not ASP.NET so just stop using the AppSettings section of the web.config file. To me, web.config files should only be modified by SharePoint solution packages.

You can store your custom application settings in property bags, SPPersistedObject ( http://msdn.microsoft.com/en-us/library/microsoft.sharepoint.administrat...
)…so no need to fiddle with web.config files.

If you absolutely need to change the web.config for operations such as adding a HTTPModule, then you should discuss with the admin guys to see how to tackle it. Either you’ll use the SharePoint framework (SPWebConfigModification), or you’ll document the change and let the admin do it if they prefer.


7. Checking that no SharePoint root file gets overwritten
One solution package can just destroy everything. So, it’s important to make sure you do not overwrite some OOTB SharePoint files living in the SharePoint root. Sometimes, you are forced to do so, such as when you want to hide some site definitions (webtemp modif). If you face such as situation, just make sure to ask your Microsoft representative whether your solution will switch your farm from supported to unsupported mode.

Whatever the answer is, you should always consider that kind of change seriously since they might break standard functionality and they might be broken by a service pack/CU…I already had requests in the past to modify the simple.master master page and I always found good arguments not to do so…(nor to implement a HTTP module to change its behavior since it is a really crappy solution).


8. Checking that components are Farm aware
Most of the times, developers (me the first) develop on a standalone machine or on a farm containing two servers: 1 SharePoint & 1 SQL. Of course, when your solution gets to the next step (acceptance/prod), the topology is likely going to change dramatically and your components will be executed in a totally different environment and thus, might behave unexpectedly.

A good example of that is the timer job. Say you create a timer job with locktype set to none. As you know, this locktype makes the job run on every server. When you are in your dev environment with only one server, you will not see the difference between having locktype set to none or set to job.

But this could typically have a very big impact if you deployment goes to production and if you didn’t really use the locktype property correctly.


9. Checking for superfluity DLLs and deployment targets
I don’t know if you already noticed but with SharePoint 2007, people were used to working with WSPBuilder and now, they usually use Visual Studio 2010 to build the solution packages. By default, WSPBuilder & Visual Studio do produce a project DLL even if your project only contains artifacts.

I’ve seen many WSPs containing that superfluity DLLs and getting deployed to acceptance/prod. While this does not have a major impact, this DLL is just pure pollution and should not be included in the solution package.

Another aspect regarding DLLS is where they are deployed, meaning either in GAC, or in BIN.

When talking about security, you often see two types of people: 1) those who do not care and deploy everything to GAC whatever the real need is. 2) those who want to deploy everything to BIN along with custom CAS policies…I try to be just in the middle since CAS policy is a true pain in…but fortunately we now have SandBoxed solutions.

I’ve even seen DLLS containing event handlers deployed to BIN and then people complaining that sometimes that event handler was not working…OMG why? May be just because once again the web context was not available…Say you have an event handler on ItemAdding, you embed it in a DLL that you deploy to the BIN because you are a very secure guy. Say that on ItemAdding you check something and refuse to add the item if one business condition is not met. While this will work when adding items from the web interfaces, it will once again not work when adding it from PowerShell, a scheduled task or whatever process not aware of the IIS context.

So, when reviewing DLL deployment targets, before thinking security, I’m thinking usability and contexts in order to avoid being too permissive or too restrictive.


10. Checking the deployment procedure
This one is easy: just make sure there is no batch file to execute or any other poor deployment technique…The only acceptable delivery is made up of SharePoint solution packages only!

Don’t hesitate to add your own sanity checks since there are many more.

Happy Coding!