Fix Dropbox on FW 4.26.16704

[shermp]

This PR fixes #107

The previous code simply passed nullptr to the MoreController::dropbox method, instead of a pointer to a valid 'this' pointer. The new code creates a valid MoreController object to pass to the dropbox method.

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...

[NiLuJe]

Yup, worksforme over here (on 4.26) :).

[shermp]

Note, I'm not entirely sure if this is going to leak memory or not, or if it even matters.

[pgaskin]

not entirely sure if this is going to leak memory or not

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 you're stack-allocating the object itself (edit: I misread the diff, you used calloc, not alloca).

[shermp]

Actually, I've heap allocated with calloc, can switch to alloca if you prefer.

[pgaskin]

Actually, I've heap allocated with calloc, can switch to alloca if you prefer.

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 _ZN14MoreControllerD0 on the pointer after you launch dropbox.

[shermp]

Oops, appeared 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 _ZN14MoreControllerD0 on the pointer after you launch dropbox.

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 free after calling the destructor. is this correct? (Obviously C++ new and delete do this automatically here)

[pgaskin]

I'm a bit hazy on C++ destructors.

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 operator new instead if you want.

[shermp]

Alright, hopefully no more leaky memory.