ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
15.37k stars 2.29k forks source link

Crash when loading user with no avatar in social browser #2435

Closed Piotrekol closed 6 years ago

Piotrekol commented 6 years ago

It is crashing with Forbidden. result.

Relevant output:

[network:verbose] Performing request osu.Game.Online.API.Requests.GetFriendsRequest
[network:verbose] Request to https://osu.ppy.sh/api/v2/friends successfully completed!
[network:verbose] Request to https://osu.ppy.sh/images/headers/profile-covers/c8.jpg successfully completed!
[network:verbose] Request to https://osu.ppy.sh/images/headers/profile-covers/c1.jpg successfully completed!
[network:verbose] Request to https://a.ppy.sh/519024 successfully completed!
[.. some more successfull completions later]
[network:verbose] Request to https://a.ppy.sh/442979 failed with System.Net.WebException: Forbidden.
[network:verbose] Request to https://a.ppy.sh/579171 failed with System.Net.WebException: Forbidden.
xWTyrone commented 6 years ago

First, the output you gave is not the only relevant output. Are you running osu! under the debugger? If so, VS caught this exception since osu! did not handle it. If you press Continue inside VS, it'll unfreeze osu! and run...

At least, that is what happens to me...

Switching VS debugger off will make osu! run fine. (and the exception is thrown anyway...)

Piotrekol commented 6 years ago

I did run it under debugger. Fair enough, I haven't even considered pressing continue, assuming that it wouldn't be caught anywhere up the chain anymore. If that's intentional behavior then this issue can be closed.

Frontear commented 6 years ago

@xWTyrone if the issue occurs in the debugger, that means it will occur with similar conditions under release, except the user can't continue their program. I don't believe this is intentional behavior, however, since I know nothing about osu API, I can't confirm. I don't believe this issue directly relates to the user, but to the avatars (given a.ppy.sh/rng). On visiting the forbidden sites, we are presented with the default avatar (question mark). Considering any random numbers you type beyond the avatar url leads to the default, this may not even be a user, or a user who removed their profile picture, maybe git restricted. Regardless, this definitely shouldn't throw an exception, assuming what I think is the truth.

xWTyrone commented 6 years ago

@Frontear

if the issue occurs in the debugger, that means it will occur with similar conditions under release, except the user can't continue their program.

Well, you are right there. The thing is the issue does occur under release. The exception is thrown and unhandled anyway (swallowed), but that doesn't interfere with the game. (Default Avatar is loaded)

osu!Framework's WebRequest class should not be ignoring 403 Forbidden errors. Good coding practice says no exception swallowing.

What actually happens is since osu! sees the request as Complete (since the server returns with 403 but sends the image anyway), the exception is ignored.

As to why it is an issue in VS, that is because it is set to check if any and all exceptions are handled.

TL;DR: Is it an issue? No. Could it be an issue if WebRequest gets used on a different context? Yes.

EDIT: Spelling.

peppy commented 6 years ago

Where are you seeing the exception swallowed? Could you please point me in the direction of the code? As far as I can tell it should still trigger firing the Failed action.

xWTyrone commented 6 years ago

VS breaks at this point (before line executes): https://github.com/ppy/osu-framework/blob/61e676094d25436bb9e8858946f65c43d15d8e01/osu.Framework/IO/Network/WebRequest.cs#L358

This is where it complains about WebException...

In the Avatar loading case, the catch block on OnlineStore.cs (https://github.com/ppy/osu-framework/blob/61e676094d25436bb9e8858946f65c43d15d8e01/osu.Framework/IO/Stores/OnlineStore.cs#L24) returns null regardless of the exception (VS thinks this is swallowing? Then why not break here?).

I have no idea why VS complains. No failed action is triggered though. And default avatar is loaded, and thus execution resumes normally.