Differential D28647

Fix KIO::Scheduler::emitReparseSlaveConfiguration() to work if called twice in same process
Needs ReviewPublic
Actions

Authored by marten on Apr 7 2020, 9:14 AM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Summary

I've been looking at porting Konqueror's User Agent Changer plugin to current KF5. The GUI is ported and working, but trying to change the user agent more than once in any given invocation of the browser does not seem to work.

After changing the user agent string in kio_httprc, the plugin calls KIO::Scheduler::emitReparseSlaveConfiguration() to inform all running ioslaves of the change. This first of all calls slotReparseSlaveConfiguration() directly to update in the current process, and then sets m_ignoreConfigReparse to true and emits the reparseSlaveConfiguration() signal. The signal calls slotReparseSlaveConfiguration() via DBus; when activated in the same process slotReparseSlaveConfiguration() ignores the signal because m_ignoreConfigReparse is set, it is reset to false and simply returns.

However, it appears that the signal does not get looped back to the current process; in other words, slotReparseSlaveConfiguration() is not called via the DBus signal. This means that m_ignoreConfigReparse is never reset to false and, the next time that KIO::Scheduler::emitReparseSlaveConfiguration() is called it has no effect. This can be confirmed by uncommenting the "Ignoring signal sent by myself" debug line in slotReparseSlaveConfiguration(), the message is never printed.

The change fixes this by explicitly setting m_ignoreConfigReparse to false before the direct call of slotReparseSlaveConfiguration(), then to true before the DBus call. This inhibits the loopback signal in case it does happen, but ensures that the direct call is not ignored.

Test Plan

Tested with https://kluge.in-chemnitz.de/tools/browser.php to show the user agent as sent.
Observed that, with this fix, the user agent can be changed as many times as required.

Diff Detail

Repository
R241 KIO
Lint
Lint Skipped
Unit
Unit Tests Skipped
marten created this revision.Apr 7 2020, 9:14 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 7 2020, 9:14 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
marten requested review of this revision.Apr 7 2020, 9:14 AM
dfaure accepted this revision.Apr 11 2020, 8:28 AM

The new code structure screams for killing the bool and just extracting the code (all of slotReparseSlaveConfiguration) to a different method, called directly from emitReparseSlaveConfiguration().

Accepting if you want to move on, but it would really be refactor that way.

This revision is now accepted and ready to land.Apr 11 2020, 8:28 AM
marten added a comment.Apr 16 2020, 10:27 AM

I'm happy to work on the refactoring if you think it's the right thing to do.

Do you mean splitting SchedulerPrivate::slotReparseSlaveConfiguration() up into two halves, the first part (KProtocolManager::reparseConfiguration through to NetRC::self()->reload - reparsing the configuration in the current process) being called directly and by the DBus signal, while the second half (check that 'proto' is applicable then iterate through the allSlaves() list) being called only from emitReparseSlaveConfiguration? Something like:

void Scheduler::emitReparseSlaveConfiguration()
{
  schedulerPrivate()->slotReparseSlaveConfiguration(...);
  schedulerPrivate()->reparseOtherSlaves();
}

void SchedulerPrivate::slotReparseSlaveConfiguration(...)
{
  KProtocolManager::reparseConfiguration();
  ,,,
  NetRC::self()->reload();
}

void SchedulerPrivate::reparseOtherSlaves()
{
  check protocol, return if not applicable
  iterate over allSlaves()
  {
    slave->send(CMD_REPARSECONFIGURATION); slave->resetHost();
  }
}
dfaure added a comment.Apr 18 2020, 11:51 AM

Something like that, but in your pseudo-code I'm missing the call to

emit self()->reparseSlaveConfiguration(QString());

"Other slaves" confuses me. This is about broadcasting to "other processes" (applications), and that each process/application (this one, and then all others) talk to their slaves.

And I wasn't clear either. There isn't really any splitting to do. Just removing the bool and the bool check, based on your assessment that "slotReparseSlaveConfiguration() is not called via the DBus signal".

dfaure resigned from this revision.Apr 18 2020, 11:51 AM
This revision now requires review to proceed.Apr 18 2020, 11:51 AM
marten added a comment.Apr 29 2020, 10:05 AM

I've thought about this a bit more and can't find any explanation as to why the DBus signal does not loop back to the sending process as well as all other listeners. It may be an unspecified detail of the DBus implementation that could possibly change at any time and start happening. So maybe it would be better to retain the boolean flag and fix the ordering - in other words, keep things simple and implement the change as in the original diff.

Revision Contents
Changeset List

PathPackages
Msrc/core/scheduler.cpp (7 lines)

Diff 79557

View Options

src/core/scheduler.cpp

Loading...