[4.5.0.16] Browser "forgets" patched files - probably patchmanager issue

lolek · 4 February 2023 14:47

REPRODUCIBILITY: 100%
OS VERSION: 4.5.0.16
HARDWARE: Xperia 10
UI LANGUAGE: PL
REGRESSION: Yes

DESCRIPTION:

After conv with @vlagged I came to conclusion that this could be patchmanager issue.

This may sound odd and crazy but somehow the SailfishBrowser “forgets” that it’s patched.

PRECONDITIONS:

patchmanager installed

STEPS TO REPRODUCE:

reboot phone (so we sure everything is clear)
go to patchmanager ui and apply my patch Browser toolbar always visible.
Start up browser and go to the settings, check if you have “Behavior” section at the bottom, if not, reboot the phone.
after reboot check again if you have the option,
play with the browser until it crash
once browser reload, go to settings, observe the “Behavior” section is gone even that browser.qml and SettingsPage.qml of the browser still contains the patch.
go to terminal, kill /usr/libexec/mapplauncherd/booster-browser --application=sailfish-browser
start up browser, go to settings, observe the “Behavior” section is back again

EXPECTED RESULT:

After browser reload the patch should still be there, the Behavior section shouldn’t be gone

ACTUAL RESULT:

Even that browser qml files still contain the patch, the Behavior section is gone.

MODIFICATIONS:

Only this one patch

ADDITIONAL INFORMATION:

miau · 4 February 2023 14:59

The same happened with other browser and mail patches in 4.4 releases.

lolek · 4 February 2023 15:01

odd, with 4.4.0.72 I didn’t spot this. Anyway I wonder what’s the problem. The files has the modifications, so how come suddenly the same binary reading from the same files “doesn’t see them”?

throwaway69 · 4 February 2023 15:07

I think the booster has somehow cached original qml, had similar weird behaviour when playing with the dialler app and my changes would half of the time not be reflected in the app when launched from launcher, see if it consistently shows up when starting from cli

lolek · 4 February 2023 15:08

it’s enough to kill booster and start app from the menu. So yeah it’s definitively booster, I mentioned this in the bug. The question is how can I clear booster cache?

pherjung · 4 February 2023 15:22

Sounds more a patchmanager bug.

lolek · 4 February 2023 15:23

I don’t think so. I checked the browser files as mentioned in the bug report and my changes are still there.
Or you think that somehow patchmanager suddenly disable and enable the patch again?

throwaway69 · 4 February 2023 15:24

Doesn’t seem like it as it applies correctly but changes are ignored when booster is involved, don’t know enough about boosters to help though

lolek · 4 February 2023 15:25

@throwaway69 @pherjung we need someone with Booster knowledge to be able to debug this more somehow. For now it’s very annoying

attah · 4 February 2023 15:49

Since patching can hardly count as “supported”, surely this can’t be called a bug.
(I.e. that some other functionality interferes with it)

lolek · 4 February 2023 15:51

Ok then let Jolla make all the patches from patchamanger options built into the system.
If not, this is how we’re adding missing functionalities.

vlagged · 5 February 2023 13:32

Hi @lolek,

My intuition says that browser-booster may start before patchmanager because there may be nothing preventing it from doing so.

For example, to be able to launch before lipstick, patchmanager installation makes this link in its rpm spec

mkdir -p %{buildroot}/%{_userunitdir}/lipstick.service.wants/
ln -s ../lipstick-patchmanager.service %{buildroot}/%{_userunitdir}/lipstick.service.wants/

We could include the equivalent for the browser-booster user service if you can confirm that this is the case when the patches do not work: the browser-booster starts before patchmanager.

With something like journalctl -b | grep -E booster\|patchmanage you can see if the services started in which order (or you could install systemd-analyze --user)

lolek · 5 February 2023 13:37

Ok I’ll try to debug the logs and get back to you with some information.
For now I’m trying to understand.
So you’re trying to say that all the patches managed by patchmanager are applied every time on system reboot?
That would be really odd and then your solution with order of service starting would made sense. However, if the “patch is forgotten”, it’s enough to reboot. I.e. it always works after reboot. So that’s probably something else, but of course I’ll dig in the logs and try to find something there!

vlagged · 5 February 2023 13:54

Oh, so it always works correctly after reboot, but doesn’t work after a browser crash. That may be different indeed.

For your other question: patchmanager uses a preload lib to redirect patched files to their patched version. The way that works is by the preloaded library talking to the patchmanager service.

vlagged · 5 February 2023 14:06

I assume there will be logs when the browser crashes, if you run devel-su journalctl -f when that happens - ideally from a computer - and then start the browser, you may have bug-worthy material…

lolek · 5 February 2023 19:36

ok this is something I’m trying hard to understand. I think it’s because of this part:

lib to redirect patched files to their patched version

Could you please drop a bit more light onto this?

In the mean time it seems crash is not the only activator of this behavior. I closed browser, locked phone, then after some time unlocked it, started browser and boom, browser doesn’t “see patch”. I was on the go I didn’t want to check if the patch is still in the .qml files but I have no reason to believe it’s not there. Of course killing booster-browser fix the problem.

vlagged · 5 February 2023 19:56

Sorry I don’t know the level of details you need this, I’ll try with “all”.

For example open

github.com/sailfishos-patches/patchmanager

src/preload/src/preloadpatchmanager.c

master


      
          
                  fprintf(stderr, "[open] pid: %d, path: %s (%s), dir: %s, flags: %d, mode: %d, no_preload: %d, validate_uid: %d, validate_flags: %d, validate_name: %d\n",
                          getpid(), new_name, pathname, dir_name, flags, mode, d_no_preload, d_pm_validate_uid, d_pm_validate_flags, d_pm_validate_name);
              }
          
              if (!d_no_preload && d_pm_validate_uid && d_pm_validate_flags && d_pm_validate_name) {
                  pm_name(new_name);
                  if (debug_output()) {
                      fprintf(stderr, "[open] new_name: %s\n", new_name);
                  }
                  return orig_open(new_name, flags, mode);
              }
          
          #endif // #ifndef NO_INTERCEPT
          
              return orig_open(pathname, flags, mode);
          }

Calls pm_name with new_name obtained from realpath. If pm_name can get a ‘redirected’ name from the socket, then that file is opened instead.

(You’ve replied to @lolek 3 times, did you know you could send them a personal message instead? haha discourse)

lolek · 5 February 2023 20:09

ok thank you, but I can’t find the connection between this and qml files. Right now my patch is just a simple diff against qml file which is applied by patch -p1 < unified_diff.patch. So, where’s redirect you’re mention in this involved? I’m unable to put this pieces together. It’s clear for so, hence the LD_PRELOAD etc but for diffs against qml?

yeah had the same in the community post

vlagged · 5 February 2023 20:20

I assumed this was done through patchmanager, which, for the qml files, will look them up by this preload + overloading open + asking a service for the actual file to respond with.
If you are patching directly the filesystem and the crash or browser close still malfunctions I am out of words how that can happen (assuming patchmanager has bugs was doable, otherwise I don’t understand either).
(Remember, patchmanager doesn’t actually patch the files, but makes copies of them + redirects them there through the LD_PRELOAD mechanism).

So maybe it’s time you share some more details how you reproduce this, through patchmanager or by changing the filesystem with patch command?

vlagged · 5 February 2023 20:22

Sorry, maybe this was not explained. You generate a patch as an unified diff, but patchmanager doesn’t just patch it in place, but goes through that LD_PRELOAD redirection.