Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Look for qbittorrent.pdb in installation directory #14505

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

glassez
Copy link
Member

@glassez glassez commented Mar 7, 2021

Pass application directory as PDB search path in SymInitialize.
Otherwise it searches in application working directory so when you run qBittorrent with working directory other than its installation one it can't find qbittorent.pdb file and produces broken stacktrace.

Pass application directory as PDB search path in SymInitialize.
Otherwise it searches in application working directory so when you
run qBittorrent with working directory other than its installation
one it can't find qbittorent.pdb file and produces broken stacktrace.
@glassez glassez requested review from Chocobo1 and a team March 7, 2021 15:28
@glassez glassez added Core OS: Windows Issues specific to Windows labels Mar 7, 2021
@glassez glassez added this to the 4.3.4 milestone Mar 7, 2021
Copy link
Member

@sledgehammer999 sledgehammer999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, even if this doesn't solve the problem, it certainly can hurt it even more.

@glassez
Copy link
Member Author

glassez commented Mar 7, 2021

FYI, there is also path to .pdb file that is hardcoded in .exe. It points to the original .pdb file path on the developer's machine where the binary was built. You can change it by setting /PDBALTPATH linker option, so you can, for example, set it to C:\Program Files\qBittorrent\qbittorrent.pdb there. Unfortunately, this is useless if the user install qBittorrent to a path other than the default one.

@FranciscoPombal
Copy link
Member

FranciscoPombal commented Mar 7, 2021

@glassez

FYI, there is also path to .pdb file that is hardcoded in .exe. It points to the original .pdb file path on the developer's machine where the binary was built. You can change it by setting /PDBALTPATH linker option, so you can, for example, set it to C:\Program Files\qBittorrent\qbittorrent.pdb there. Unfortunately, this is useless if the user install qBittorrent to a path other than the default one.

Considering the proposed fix, it's no surprise that we were seeing broken stacktraces until now. My question now is why did it work for some people, but not others? I don't understand why it wasn't broken for everyone. For example, it has always worked for me (I always install it to default location on Windows, which I think is under Program Files). Like I said in the team post, I can reproduce crashes with stacktraces reliably with 4.3.0, but I can't reproduce broken stacktraces on my machines, if that's what you're after.

@glassez
Copy link
Member Author

glassez commented Mar 8, 2021

My question now is why did it work for some people, but not others? I don't understand why it wasn't broken for everyone.

Sorry, did you read the first post, or just the second? Here is the main thing:

it searches in application working directory so when you run qBittorrent with working directory other than its installation one it can't find qbittorent.pdb file and produces broken stacktrace.

There is no problem when qBittorrent is s launched directly from its installation directory or using link created by installer (since it sets correct working directory).

Like I said in the team post, I can reproduce crashes with stacktraces reliably with 4.3.0, but I can't reproduce broken stacktraces on my machines

Did you try to test all the cases proposed in team post?

@xavier2k6
Copy link
Member

@glassez What about the scenario No. 4 from #13774 (comment)

Overwrite/update the qBittorrent executable but leave the pdb file from the previously installed version.

This PR doesn't help in this case correct? (User runs in portable mode via profile folder)

@glassez
Copy link
Member Author

glassez commented Mar 8, 2021

Overwrite/update the qBittorrent executable but leave the pdb file from the previously installed version.

This PR doesn't help in this case correct?

I don't think there is a way to do anything in the case of a broken installation. We shouldn't even care about it.

@xavier2k6
Copy link
Member

We shouldn't even care about it.

Ok, so in scenarios like that we continue as we were & close/label as invalid with a small description of why.

@FranciscoPombal
Copy link
Member

FranciscoPombal commented Mar 8, 2021

@glassez

My question now is why did it work for some people, but not others? I don't understand why it wasn't broken for everyone.

Sorry, did you read the first post, or just the second? Here is the main thing:

it searches in application working directory so when you run qBittorrent with working directory other than its installation one it can't find qbittorent.pdb file and produces broken stacktrace.

There is no problem when qBittorrent is s launched directly from its installation directory or using link created by installer (since it sets correct working directory).

👍

Like I said in the team post, I can reproduce crashes with stacktraces reliably with 4.3.0, but I can't reproduce broken stacktraces on my machines

Did you try to test all the cases proposed in team post?

My bad, I misunderstood something. Anyway, in my tests I was always starting qBittorrent by double-clicking the executable after extracting it (and the pdb) from the installer to a temp folder (case 2). I now confirm your results for the remaining cases; case 1 is also fine (starting from the installed shortcuts), but 3 and 4 are problematic (starting from the browser or a torrent file outside of the executable directory).

I haven't have time to test the fix in these cases as well, but I assume you did and the fix "looks" like to me. I agree with:

IMO, even if this doesn't solve the problem, it certainly can hurt it even more.

@xavier2k6

From my testing - there's a few scenarios that cause the inadequate stack traces

  1. Rename the pdb file
  2. Delete the pdb file
  3. Run the qBittorrent executable in portable mode via profile & don't include the pdb file
  4. Overwrite/update the qBittorrent executable but leave the pdb file from the previously installed version. (manually)

These are all scenarios where it is inevitable to have broken stacktraces and there is nothing we could do about it. Cases 1., 2. and 3. are functionally the same. As for case 4., the pdb from the previous version will contain the wrong symbols for the current one, so it is also not useful.


@qbittorrent/frequent-contributors

Since apparently quite a few users make use of portable mode, should we prevent qBittorrent from starting if the pdb is not present in the same directory as the executable (only applicable on Windows, of course)? I suspect that when many users prepare a "portable" installation (e.g. in a USB drive), they only copy over the executable, not the pdb. Thus, forcing users to hold on to the pdb would result in usable crash reports from them as well.
The check can be really simple: just check if there is a file with the expected name in the same directory; if not, display an error message and exit.

EDIT: moved this to a proper discussion page, as it is another matter entirely: #14513

@glassez
Copy link
Member Author

glassez commented Mar 8, 2021

should we prevent qBittorrent from starting if the pdb is not present in the same directory as the executable

I don't think we should do that. To be radical, I'd rather get rid of the portable mode. But if someone likes it, let them be responsible themselves. We will just ignore the broken stacktraces as suggested above.

@sledgehammer999
Copy link
Member

should we prevent qBittorrent from starting if the pdb is not present in the same directory as the executable

IMO, this is a heavy handed approach and I vote no. At worst, the stacktrace report could indicate if the pdb was missing from the dir so we can close the issue if there isn't other info that helps reproduce the crash. (not a PR requirement)

@xavier2k6
Copy link
Member

These are all scenarios where it is inevitable to have broken stacktraces and there is nothing we could do about it. Cases 1., 2. and 3. are functionally the same.

I am aware of that, that was for the time we were investigating how we were getting the "empty stack traces"

Hence

All these scenarios are user intervention - If installed via installer or copied correctly, there doesn't seem to be any problem. (Correct pdb associated with correct qBittorrent executable)


The check can be really simple: just check if there is a file with the expected name in the same directory; if not, display an error message and exit.

This achieves nothing, it's not checking the files contents/file size/checksums etc.

To circumvent this, a user just has to create the missing pdb by say renaming a .txt file correctly or as I've said previously leave the previous version.

@glassez glassez merged commit 0bf36ad into qbittorrent:master Mar 8, 2021
@glassez glassez deleted the stacktrace branch March 8, 2021 11:57
@FranciscoPombal
Copy link
Member

@xavier2k6

The check can be really simple: just check if there is a file with the expected name in the same directory; if not, display an error message and exit.

This achieves nothing, it's not checking the files contents/file size/checksums etc.

To circumvent this, a user just has to create the missing pdb by say renaming a .txt file correctly or as I've said previously leave the previous version.

Why would a user do this? This would never reasonably happen. At most, maybe due to some rare occurrence, the old pdb could be left behind. But I would expect the pdb to contain some sort of version metadata, so we could parse that to rule out that particular situation in addition to simply checking for existance. But all other "counter-measures" are out-of-scope IMO. Why should we bother checking if the user renamed the extension to something else, zeroed out the file, etc? If they did that, it must be part of some strange experiment that we don't care about.

@xavier2k6
Copy link
Member

Why would a user do this?

Annoyance of nagging......hey this is the wrong version....
space limitations....

Who really knows & we really don't care at this stage....lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core OS: Windows Issues specific to Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants