OCC::AbstractNetworkJob::~AbstractNetworkJob

[sentry-io]

Sentry Issue: DESKTOP-WIN-AND-MAC-G08

EXC_BREAKPOINT / EXC_ARM_BREAKPOINT / 0x10552e9f0: Fatal Error: EXC_BREAKPOINT / EXC_ARM_BREAKPOINT / 0x10552e9f0
  File "abstractnetworkjob.cpp", line 290, in OCC::AbstractNetworkJob::~AbstractNetworkJob
  File "qobject.cpp", line 2211, in QObjectPrivate::deleteChildren
  File "qobject.cpp", line 1138, in QObject::~QObject
  File "space.h", line 60, in OCC::GraphApi::Space::~Space
  File "space.h", line 60, in OCC::GraphApi::Space::~Space
...
(56 additional frame(s) were not displayed)

[modSpike]

so far just eyeballing the code I have this braindump:

the SpaceImage instance in the Space creates a ResourceJob on update (Space.cpp:87) passing the Space instance as parent. I am not yet familiar with the logic here but that seems odd to me. As far as I can tell the SpaceImage is just a gui element/icon, right? why should it be creating jobs?! And why is it passing the Space to parent to the job it creates instead of itself?

Anyway, once that job is created it appears that it deletes itself when finished - this should be no problem in theory so perhaps something is going wrong during the "later" aspect of deleteLater. Quick test could be to just replace
AbstractNetworkJob::slotFinished line 236 -> deleteLater; with a straight delete but would have to find a consistent way to reproduce the crash for that change to be verified as good or not.

One aspect of the ResourceJob I didn't get into yet is that it calls "setStoreInCache" in its ctr. A quick look suggests this is a flag that controls some kind of caching in the request but I want to make sure the job isn't being cached and potentially deleted somewhere else.

Also want to investigate what happens with imageChanged() signal - it's emitted from the image when it receives the job's "finished" signal (before deleteLater) but this seems to just be piped out via a dup signal in Space which goes...nowhere? I don't think this has much to do with the crash but it doesn't look healthy so I want to look a little bit deeper.

[modSpike]

midnight thought from the weekend: SpaceImage::update is called in SI ctr and that function relies very heavily on Space. SpaceImage is constructed in the initializer list of Space. This is not particularly safe so I will remove that call to update at least in the ctr - maybe it helps. As far as I can tell it's pointless anyway since the resource path is not available yet, so should be a noop?

[modSpike]

ahhhh and I found this buried in a comment in SpacesManager:
// TODO: leak the job until we fixed the onwership #11203

that issue is marked as fixed but looks awfully similar :/

Drives is ultimately an AbstractNetworkJob which should delete itself as the last step of its finished slot. But we are also calling deleteLater on it in SpacesManager:62 In theory multiple calls to deleteLater is "safe" but this is very convoluted and hard to follow. Looks like a "whack a mole" solution to some previous problem?

[modSpike]

I have some improvements around not calling SpaceImage::update on ctr, and also checking the incoming dir value(s) when Space::setDirectory is called to make it a noop when the dir has not actually changed. I will commit these as soon as I have verified that they don't cause any new problems. Main concern is whether the Space/dir info gets updated when the icon changes - I would expect I can still reduce these refresh calls even in that case by adding an extra check against the icon info for the dir.

As follow up: I will write a more comprehensive refactoring story that moves responsibility for creating the resource request to the Space, slash the bidirectional dependency from SpaceImage to Space and anything else I can identify to reduce unpleasant aromas.