Controller.ControllerContext property null in AuthorizeAttributeAclModule

[rahulbpatel]

One of our custom AuthorizeAttribute implementations accesses the AuthorizationContext.Controller.ValueProvider property which is causing an ArgumentNullException inside the AuthorizeAttributeAclModule because Controller.ControllerContext property is null:

I believe either MvcContextFactory.CreateControllerContext or AuthorizeAttributeAclModule.CreateControllerContext methods should set Controller.ControllerContext property to the created ControllerContext.

Example:

public class ControllerContextSettingMvcContextFactory : MvcContextFactory {

public override ControllerContext CreateControllerContext(RequestContext requestContext, ControllerBase controller) {
            var result = base.CreateControllerContext(requestContext, controller);
            if (controller.ControllerContext == null && result != null) {
                controller.ControllerContext = result;
            }
            return result;
        }
}

[NightOwl888]

Thanks for the report.

Your solution fixes the original problem, but also causes a memory leak from the circular reference. It was actually a bit of surprise to me that MVC didn't handle this detail until I realized why.

I didn't put this change in the MvcContextFactory because that would cause a memory leak whenever the CreateControllerContext() method is reused somewhere else. Instead, I localized the change in the AuthorizeAttributeAclModule.VerifyContoller() method, where it could be placed in the pre-existing try-finally block to ensure it is always cleaned up. I think it is better to forget to set this property in the future than to forget to set it to null somewhere when reusing the factory and have it leak memory unknowingly.

Anyway, I pushed the change to the dev branch. Please check this out for your use case to make sure it works as expected. If so, I should be able to push the patch release to NuGet tomorrow.

If you need me to push it to MyGet for you so you can install it via NuGet to your project rather than building the NuGet package yourself (with build.bat - see the build instructions on the home page), let me know.

[rahulbpatel]

Hi,

Thanks for the quick response. I missed the memory leak. The easiest
thing for me currently is to inherit from AuthorizeAttributeAclModule and
override method VerifyController as per your implementation and use that
instead of the default implementation with my DI config. I will make a
note to remove it whenever you get a chance to release again.

Thanks!

On Tue, Feb 4, 2014 at 2:51 PM, NightOwl888 [email protected]:

Thanks for the report.

Your solution fixes the original problem, but also causes a memory leak
from the circular reference. It was actually a bit of surprise to me that
MVC didn't handle this detail until I realized why.

I didn't put this change in the MvcContextFactory because that would cause
a memory leak whenever the CreateControllerContext() method is reused
somewhere else. Instead, I localized the change in the
AuthorizeAttributeAclModule.VerifyContoller() method, where it could be
placed in the pre-existing try-finally block to ensure it is always cleaned
up. I think it is better to forget to set this property in the future than
to forget to set it to null somewhere when reusing the factory and have it
leak memory unknowingly.

Anyway, I pushed the change to the dev branchhttps://github.com/maartenba/MvcSiteMapProvider/tree/dev.
Please check this out for your use case to make sure it works as expected.
If so, I should be able to push the patch release to NuGet tomorrow.

If you need me to push it to MyGet for you so you can install it via NuGet
to your project rather than building the NuGet package yourself (with
build.bat - see the build instructions on the home pagehttps://github.com/maartenba/MvcSiteMapProvider),
let me know.

Reply to this email directly or view it on GitHubhttps://github.com//issues/271#issuecomment-34111424
.

---

Rahul B. Patel
Parsus Solutions, LLC
14358 N. Frank Lloyd Wright Blvd., #15
Scottsdale, AZ 85260
480.614.9000 (office)
480.650.2445 (cell)
866.708.6847 (fax)

[rahulbpatel]

Hi,

Thinking about it further, it seems that the GC can handle circular references just fine:

http://stackoverflow.com/questions/400706/circular-references-cause-memory-leak

Am I missing something in this case why the reference can't be set in MvcContextFactory?

[NightOwl888]

Hmm... I think I am getting old. I swear I read some documentation somewhere that specifically said that the CLR couldn't deal with circular references automatically. Maybe that was from an earlier version of .NET and they have since fixed it since then, I don't know.

I did an experiment on the SiteMap > Node > SiteMap circular reference by removing the SiteMap.Clear() call in the SiteMapLoader and adding another 100KB of buffer onto the SiteMapNode. I also set the CacheDuration to 0. Then I watched the memory consumed by the application carefully and have confirmed that the garbage collection is working in that case - I was never able to make the MvcMusicStore consume more than about 175MB and then it would drop back down to about 100MB after doing several reloads of the SiteMap (even with the ridiculous payload I added to it). The good news is you just helped me fix the second issue in #236 (and the first issue I have fixed already and will be in the v4.5 release) because there is no reason to call the Clear() method in the first place.

Anyway, I still have yet to confirm explicitly that there is no memory leak in the controller > controllerContext > controller circular reference, but I suspect my results will be the same. Thanks for pointing that out. So indeed putting this into the MvcContextFactory seems like the best choice.

But it is still a puzzle why when creating a new ControllerContext object and passing it a controller why MVC doesn't automatically set the controller's ContollerContext for you. I guess that is a puzzle I don't need to solve.

[NightOwl888]

FYI - The fix is now live in v4.4.10.

[rahulbpatel]

Hi,

Thanks for all your help on this. I am going to upgrade to v4.4.10 now.

I was wondering the same thing about when ControllerContext gets set in MVC
framework so I looked into the code. It gets set in a call to
ControllerBase.Initialize(RequestContext) which is called by either
ControllerBase.Execute(RequestContext) or
Controller.BeginExecute(RequestContext).

On Wed, Feb 5, 2014 at 2:08 AM, NightOwl888 [email protected]:

Hmm... I think I am getting old. I swear I read some documentation
somewhere that specifically said that the CLR couldn't deal with circular
references automatically. Maybe that was from an earlier version of .NET
and they have since fixed it since then, I don't know.

I did an experiment on the SiteMap > Node > SiteMap circular reference by
removing the SiteMap.Clear() call in the SiteMapLoader and adding another
100KB of buffer onto the SiteMapNode. I also set the CacheDuration to 0.
Then I watched the memory consumed by the application carefully and have
confirmed that the garbage collection is working in that case - I was never
able to make the MvcMusicStore consume more than about 175MB and then it
would drop back down to about 100MB after doing several reloads of the
SiteMap (even with the ridiculous payload I added to it). The good news is
you just helped me fix the second issue in #236#236 the first issue I have fixed already and will be in the v4.5 release)
because there is no reason to call the Clear() method in the first place.

Anyway, I still have yet to confirm explicitly that there is no memory
leak in the controller > controllerContext > controller circular reference,
but I suspect my results will be the same. Thanks for pointing that out. So
indeed putting this into the MvcContextFactory seems like the best choice.

But it is still a puzzle why when creating a new ControllerContext object
and passing it a controller why MVC doesn't automatically set the
controller's ContollerContext for you. I guess that is a puzzle I don't
need to solve.

Reply to this email directly or view it on GitHubhttps://github.com//issues/271#issuecomment-34148538
.

---

Rahul B. Patel
Parsus Solutions, LLC
14358 N. Frank Lloyd Wright Blvd., #15
Scottsdale, AZ 85260
480.614.9000 (office)
480.650.2445 (cell)
866.708.6847 (fax)