sailfishos-patches / patchmanager

Patchmanager for SailfishOS
https://openrepos.net/content/patchmanager/patchmanager
Other
21 stars 22 forks source link

Should categories which involve a booster trigger a booster restart? #418

Open nephros opened 1 year ago

nephros commented 1 year ago

Some Apps, (Browser and Email) are tied to a browser booster service.

PM currently only restarts the app itself, not the booster.

Should PM also trigger a service restart of the booster?

$ systemctl --user list-units booster-browser*
UNIT                                         LOAD   ACTIVE SUB     DESCRIPTION
booster-browser.service                      loaded active running Application launch booster for Sailfish Browser
booster-browser@jolla-email.service          loaded active running Application launch booster for Sailfish Browser (sandboxed)
booster-browser@sailfish-browser.service     loaded active running Application launch booster for Sailfish Browser (sandboxed)
Olf0 commented 1 year ago

My first, intuitive notion was "yes", but I have never analysed what the booster does in detail (some preloading of libraries, IIRC), hence I cannot tell if this is necessary. As I assume restarting the booster is costly (CPU and run-time), I think we should know for sure if this is needed.

Asked the other way around: Is there some indication (ether technically or by testing) that things do not work well without restarting the specific booster service of Jolla's email or browser app?

nephros commented 1 year ago

Bootser-browser is specific for apps using the WebView, which currently apart from the browser proper is Email. The exact working are hidden from us, but it somehow keeps resources in memory so that WebView launch is much improved.

(One can check that by building a WebView-using application, and starting a booster-browser@my-app.service templated booster, and measuring WebView initialization times. It's much improved, at the expense of (quite some) memory.)

I would assume that there is a case when relaunching the booster@ service is necessary: If a patch modifies anything the booster process has preloaded. Which would basically be things in the Mozilla profile directory.
It's unlikely many patches will use that, however I have one: https://coderus.openrepos.net/pm2/project/patch-email-darker which modifies the userCSS for the Email webview component. This CSS file is read at Browser Launch and then never again.

Which means there is one moment when a booster restart would be necessary: If the EMail booster is running (which it normally is), and none of its files have ever been patched, and then my abovementioned patch is installed.
This is because in this state, the preload library loaded into the booster does not yet know that it should work its magic on the CSS file.

If the booster has been restarted once, activating and deactivating (or modifying the contents of) the patch does not need a restart.

And in theory, upon uninstallation of the patch one could restart the booster, but not doing so has no effect because the patch has been deactivated and does not exist any more.

Olf0 commented 1 year ago

If the booster has been restarted once, activating and deactivating (or modifying the contents of) the patch does not need a restart.

… but that is only because you implemented that cleanly, I assume: You patch a library to use your CSS file and … activating / deactivating only changes the CSS file? While writing this, I ask myself: How is this achieved? It is only a single unified diff (or not in this case?), so how can one alter two files upon installation (or enabling?), but only one when activating / deactivating?

But no need to explain in detail: Give me a pointer to source at Git* and I will read, if there is no public repo then just do not care, because …

And in theory, upon uninstallation of the patch one could restart the booster, but not doing so has no effect because the patch has been deactivated and does not exist any more.

Sounds as if that again is due to a fail-safe implementation by you. Others might implement in a "quick & dirty" manner, hence I believe that restarting the booster for these two categories makes sense, both upon enabling and disabling such a Patch.

nephros commented 1 year ago

… but that is only because you implemented that cleanly, I assume: You patch a library to use your CSS file and … activating / deactivating only changes the CSS file? While writing this, I ask myself: How is this achieved? It is only a single unified diff (or not in this case?), so how can one alter two files upon installation (or enabling?), but only one when activating / deactivating?

Nono, the library I mean is the Patchmanager preload library.

It goes somewhat like this:

  1. Booster starts
  2. PM library is loaded into booster
  3. PM library reads list of enabled patches, figures out which files are affected, and hijacks/redirects syscalls on those files
  4. Lets say in our case it watches file locations a, b, and c.
  5. Now a new patch is installed and activated (e.g. my patch to the CSS file), which affects a file in location d
  6. Normally, for any non-boosted app, it is enough to restart that app because then it will again trigger steps 2 and 3, and that instance of the preload library knows about all four file locations
  7. In the case of the Email app, it can be restarted, but the booster process in the background still has the lib loaded and configured from step 3, so it's not affected by a patch at location d
  8. Lets restart the booster
  9. New load of the library, new configuration with all four locations, all is fine
  10. At this point, as the loaded library from 9. knows about my CSS patch, I can activate and deactivate, even update it, the library intercept the calls and the patch works
  11. To compete the picture, lets say we de-activate and uninstall the patch. Now our library from 9. still knows all four locations, d doesn't exist anymore, but that's harmless because the patch got deactivated before uninstalling.
  12. We could restart the booster, but it wouldn't make a real difference.
Olf0 commented 1 year ago

I took my time, because it appears that you want another mind to check these considerations for correctness.

  1. I think that "PM library", "preload library", "the lib", "the library" and "our library" do all mean the same, correct? (I am pretty sure about that.)
  2. The first statement I fail to comprehend is the first half of step 6: "… for any non-boosted app, it is enough to restart that app because then it will again trigger steps 2 and 3, …". How does an app (or any other software component), which does not know anything about PM and its preload library "trigger steps 2 and 3", i.e. (step 2), the boosting of the PM preload library?

    I would have expected, that the description of step 7 (app using boosted software components) applies to step 6 (app which does not utilise boosted software components), too. The description of step 7 does make sense, but misses to point out why the fact that an app uses a boosted software component makes a difference for PM's boosted preload library.

  3. For step 10 I perceive (nitpickingly) that it becomes untrue AFAIU, when an updated Patch patches a different set of files (and maybe even different locations / callable functions in the same set of files) than before. Right?
  4. While I believe to understand the logic of steps 10 and 11, I have a strange gut feeling, that the PM library still holding a reference to location d to intercept calls to it does not matter; doesn't the PM preload library redirect the intercepted calls to a patched copy (with d in it) on tmpfs of the original software component? Or does PM preload library double-check if that patched copy does still exist and if not redirects the calls to the original software component on internal mass storage?

I reread the steps 1 to 11 multiple times, also trying to imagine that the many "it"s do not reference the last occurring noun or subject, but I still fail to comprehend them. Sorry.

nephros commented 1 year ago

2. How does an app (or any other software component), which does not know anything about PM and its preload library "trigger steps 2 and 3",

We are forcing all processes to preload the library through ld.so.preload. So in fact all are made to "know about" PM (at least subconsciously).

nephros commented 1 year ago
  • For step 10 I perceive (nitpickingly) that it becomes untrue AFAIU, when an updated Patch patches a different set of files (and maybe even different locations / callable functions in the same set of files) than before. Right?

  • While I believe to understand the logic of steps 10 and 11, I have a strange gut feeling, that the PM library still holding a reference to location d to intercept calls to it does not matter; doesn't the PM preload library redirect the intercepted calls to a patched copy (with d in it) on tmpfs of the original software component? Or does PM preload library double-check if that patched copy does still exist and if not redirects the calls to the original software component on internal mass storage?

I'm currently not sure how exactly the communication between the daemon and the library works.

It is clear that doPrepareCache in the daemon does check for file existence, dangling symlinks etc. And that pm_name (from the library) gets the name it should redirect to from a connection to the daemon, and does no checking or managing of lists of existing files itself.

nephros commented 1 year ago

We definitely have an issue with boosted apps.

I frequently see patches not being in effect (despite being activated/applied) for Browser and Email.

Patches like:

'randomly' stop working.

Possibly after they, or their boosters, are restarted, killed, or OOM killed. Not sure when it triggers.

Olf0 commented 1 year ago

Sorry that I cannot contribute to this, not even the discussion: It is beyond my current know-how and I lack the time to become more knowledgeable on this topic.

nephros commented 1 year ago

No sweat at all.

I'll open a separate issue about this soon, just so we have something to point to as a known issue, and so we have something to track.

b100dian commented 1 year ago

(To add a couple of other things that don't work, not booster related - changing the number of cameras in patch-five-cameras-tucana also requires a dconf update too :)

To the topic: randomly stopping working means that we need to add systemd dependencies on patchmanager service for those booster services I guess.

But "stopping working" on a running system is not something I have an explanation for either.


Regarding communication preload library <--> daemon, it's /tmp/patchmanager-socket based. The protocol is that the preload library writes a path, and the daemon returns the redirected path (or the same one).