-
Notifications
You must be signed in to change notification settings - Fork 67
Fix Dropbox on FW 4.26.16704 #108
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
Conversation
It may not have been necessary as of FW 15505, but required in later versions
|
Yup, worksforme over here (on 4.26) :). |
|
Note, I'm not entirely sure if this is going to leak memory or not, or if it even matters. |
In that case, I usually look at the callers to determine what they do. Note that even though in this situation, no functions appear to reference the PLT entry (which calls the main symbol entry) directly, there is a reference to the "bx pc" instruction above it. This is used to switch between ARM/Thumb when tail-calling the function. Currently, the only reference detected is a switch table, which is referenced by the Qt metacall. Thus, it's probably used in another widget by a signal connection. Now, we look for references to the MoreController constructor, and can figure out how it's used by inspecting its callers. In short, this leads us to the MainWindowController, which we can see deletes views after they are popped. This isn't directly relevant to us, but it tells us that any views handled by the MainWindowController must be eap-allocated. This is confirmed the other reference to the MoreController constructor (the Activity view's back button). Next, we inspect the MoreController's construction itself to see if it allocates anything which could come back to bite us later (by causing memory leaks or referencing itself later). From that, it's pretty clear that it's just a plain QObject (which doesn't need to be properly destructed, we can do what we currently do with it on the stack) and some *NavMixins allocated on the MoreController's memory, which do not have anything relevant in their constructor. Finally, just to make sure there aren't any other references outside the function call (we don't expect it since MWC is usually in charge of the MC's lifetime, and it can pretty much delete it whenever it wants), we can check the Dropbox function. It is as we expect. Based on this, it's now clear that at least in this version, it's safe to either stack-allocate the memory used for the MoreController, or heap-allocate it and tell the QObject to delete itself later or call D0. In this context, both are just as likely to break in the future if Kobo decides to start using the MoreController later, so I'd go for the first one because it's simpler. If they did, the problem would show up as a crash when the memory is attempted to be freed later either stack corruption. Because of the location and the purpose, it should fail relatively soon after in this situation, and it should be relatively easy to diagnose. If it wasn't, I'd use the heap-allocation option to make it easier to track down. To answer your question directly, it won't leak memory directly since it doesn't allocate any within the object, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but it'd be nice if someone could test this on an older version to be sure (I'll do it if nobody else does). This would just be to ensure it doesn't cause a regression (it doesn't need to support Dropbox -- it'll show a blank screen in this case, but it shouldn't cause a crash).
|
Actually, I've heap allocated with |
Oops, appears I misread that while doing this from my phone. In that case, it does indeed leak memory, but you should be safe just deleting it. But now that I think about it, we might as well do this properly. Call |
Yeah, was thinking that calling the destructor was probably the right thing to do, just wasn't sure if the MoreController would end up belonging to another QObject somewhere along the way and be deleted elsewhere. I'm a bit hazy on C++ destructors. I THINK I will need to call |
The C++ ABI document is your friend. D0 will delete it for you, and D2 will let you delete it yourself. Note that D2 is essentially a no-op right now (which is why it's currently safe to stack allocate it), and D0 is just D2+delete. Also, it's up to the compiler to decide which ones it needs to generate, so not all objects will have a D0. So, no, you don't need to free it if calling D0. Also, technically, it's undefined behavior for it to delete malloc'd memory (one is C, the other is C++), but they should be identical in practice. To be safe, you could allocate it with |
To avoid UB, use ::operator new() to allocate memory
|
Alright, hopefully no more leaky memory. |
This PR fixes #107
The previous code simply passed
nullptrto theMoreController::dropboxmethod, instead of a pointer to a valid 'this' pointer. The new code creates a validMoreControllerobject to pass to thedropboxmethod.Tested on my Forma with fw 4.26.16704.
@NiLuJe can you please double check it works on your Forma?
Has anyone got fw 4.23.15505 they could test with? It should work, but probably wouldn't hurt to test...