-
Notifications
You must be signed in to change notification settings - Fork 565
NullReferenceException when accessing "steamLogin" cookie #197
base: master
Are you sure you want to change the base?
Conversation
+1, should definitely merge @jshackles |
This works, thanks! |
By checking the cookie, not the value for a null you do have a single lookup. |
@treloret I think what you've done in #243 is just fine. I didn't go check the Item[String] exception behavior when implementing my solution and I prefer to "look before you leap" or use "defensive programming" when handling user input which is what I consider cookies. But Item[String] will only raise an ArgumentNullException and the argument is a static string.
I would argue that your implementation is not "less brute force" than mine because of how the Item[String] accessor is implemented. |
When the steamLogin cookie was not in the collection a NullReferenceException was caught and propagated out which prevented the badge page from ever loading.
I updated this PR with a new fix. Instead of iterating the cookie collection directly one lookup is performed and cached locally. The cached instance is tested for null (the original problem) and then for the value "deleted" (the original goal). It worked for me locally. |
@opello can you compile? |
Works nicely |
The latest Idle Master was never able to load the badge page for me, but it seemed like everything should be working according to a Wireshark capture. Then I looked in
error.log
:I did some additional debugging and came up with the fix I'm proposing here. It's too bad that the test isn't a single lookup, but I don't think there will be a large number of cookies to iterate through. If this proposed change is undesirable for that reason I think larger changes to get access to a
System.Web.HttpCookieCollection
would be required. I also thought about catching theNullReferenceException
on the access of theCookieCollection
but this seemed cleaner.