Summary

Fix context leaks in ActionHooks.

Review Request #5762 — Created May 2, 2014 and submitted May 6, 2014, 5:25 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.0.x

Bugs

Depends On

Commit

e3fa7c5...

Reviewers

Groups

reviewboard

People

Description

Context.update() implies a push(), but the ActionHooks weren't popping.

This meant that state would be leaked not only between hooks, but into

the main template as well.
We now do a pop() after rendering the template.

Testing Done

Unit tests pass.

Issues

Description	From	Last Updated
This isn't exception safe. Maybe move the try/except into the for loop and push/pop outside of it?	david	May 6, 2014, 4:57 p.m.
Hmm, this isn't quite right now, especially with the logged error message. Probably there should be two exception handlers, one …	david	May 6, 2014, 5:16 p.m.

reviewboard/extensions/templatetags/rb_extensions.py (Diff revision 1)

The issue has been resolved. Show all issues

This isn't exception safe. Maybe move the try/except into the for loop and push/pop outside of it?

chipx86 May 6, 2014, 4:59 p.m.

I originally did that, and then changed it back, because the templates they're rendering are under our control, and not the extension's. However, we may decide to change that someday, and the old code is more wrong, so fixing.

Change Summary:

Now handles exceptions when rendering a template for an action.

Commit:

53435c67917f2c60b8a454e6dfc8ad72db7eb717

0eb33be1d53c7e074db9acf22fe04692340826bd

Diff:

Revision 2 (+34 -14)

Show changes

	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py

reviewboard/extensions/templatetags/rb_extensions.py (Diff revision 2)

The issue has been resolved. Show all issues

Hmm, this isn't quite right now, especially with the logged error message.

Probably there should be two exception handlers, one around get_actions(), and one around the render call.

chipx86 May 6, 2014, 5:16 p.m.

Ugh yeah. I wasn't paying enough attention.

Change Summary:



Wrapped the whole get_actions() loop in one exception handler (since we still want to respect generators, I didn't want to convert to a list in its own try/except), and the rendering in another exception handler.


Made some fixes to error text.

Commit:

0eb33be1d53c7e074db9acf22fe04692340826bd

e3fa7c578b87e44674ad722bc0a969b4af3d4ed5

Diff:

Revision 3 (+37 -11)

Show changes

	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py

Ship it!

```
Ship It!
```

Status:: Completed