Free library even if proc lookup fails#108312
Free library even if proc lookup fails#108312auto-submit[bot] merged 1 commit intoflutter:masterfrom verath:patch-1
Conversation
flutter-dashboard
bot
added
the
tool
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
cc @loic-sharma can you review this? As a non C++ dev, this looks like something we'd want :) |
loic-sharma
requested review from
a-wallen,
cbracken,
gspencergoog,
stuartmorgan-g and
yaakovschectman
|
@cbracken Should we get a test exemption for this? We'd need to somehow check if there's a memory leak when creating a window on a machine that doesn't have |
|
Alternately you could wrap the Win32 APIs with a DI-able object, and then test that you get the expected calls when you simulate various return values (using a mock version of that API wrapper). That's how we test the clipboard, for instance, and also how some of our Windows plugin unit tests work. |
|
Thanks for looking at this! Let me know about how you want to handle the test cases. I can give adding one a it a try, if you can give me some more concrete pointers in the right direction. I did this PR via the Github web-editor, and only because I happened to run across the example file in another project. In other words, I am far from fluent in this code base :). |
|
Here's a very simple example of that pattern in one of the Windows plugins:
|
There was a problem hiding this comment.
Thanks!
Re: testability. Since this is just proactively cleaning up a DLL we're no longer using, it likely qualifies for a test exemption. Arguably it's somewhat harmless to leave it loaded, especially if we plan to use anything else from that DLL in the future -- as far as I know we don't in the template.
Personally, I'd prefer that we use an RAII pattern here since that'll guarantee cleanup no matter how many additional conditional paths we add to this function.
HMODULE is just a void* so a simple unique_ptr with a custom deleter should work fine. Usage would look something like how we use fml::UniqueObject. This might go somewhere like the utils.h header. Something like:
// utils.h
// Custom deleter that unloads the library pointed to by LibraryPtr.
struct LibraryDeleter {
typedef HMODULE pointer;
void operator()(HMODULE h) {
FreeLibrary(h);
}
};
// A smart pointer to a library loaded via LoadLibraryA.
using LibraryPtr = std::unique_ptr<std::remove_pointer_t<HMODULE>, LibraryDeleter>;// win32_window.cpp
// ...
void EnableFullDpiSupportIfAvailable(HWND hwnd) {
LibraryPtr user32_module(LoadLibraryA("User32.dll"));
// No matter how many paths are possible to bail out of this function
// they'll all result in the library getting freed.
}If we wanted to go a little fancier, we could create a type that takes a string (library name) as a constructor parameter, then does the LoadLibraryA in the constructor and FreeLibrary in the destructor. We do this for ImmContext in the engine, for example. To be honest that's probably overkill here though.
|
fwiw I don't think we need to do the above refactor in this PR. I'm fine landing as-is to get the library cleanup in. It would be straightforward to do the cleanup later, since it might involve some bikeshedding on our part :) |
|
re tests: Thanks for the pointers, looks very reasonable! Let me know when you decide if a test is needed or not. I might need some additional help in where to place the tests if so. Agreed this change is probably not strictly necessary, leaving the dll loaded is likely not a problem in practice. I still did this PR because this is part of sample code (as far as I understood), so there is some value in "doing the right thing", I think. Also because this was a very easy fix 😄. I also think a RAII wrapper is better. I did not do that initially because that would be a much bigger change, which I would feel uncomfortable doing as a first contribution. Will happily leave this to you people to clean up further! One more thing. There seems to be multiple |
No worries at all. There's also a lot of value in keeping the code as simple and readable as possible. The RAII solution is something we'd definitely do in the engine, but I can definitely see the case for keeping it as a single FreeLibrary in the template. :)
Good catch! Those are clones of this one. As part of our integration testing, we have a few instantiated apps checked into the repo. The code in those apps should but updated with updates to the template, like this patch does. |
|
I made the same change to all |
christopherfujino
added
the
autosubmit
6 participants
Makes sure to call
FreeLibraryeven ifGetProcAddressfails.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.