Opened 6 years ago

Last modified 3 weeks ago

#74 new change

Make sure ABP engine closes gracefully.

Reported by: trev Assignee:
Priority: P3 Milestone:
Module: Adblock-Plus-for-Internet-Explorer Keywords:
Cc: oleksandr Blocked By: #1652
Blocking: #1195, #1202 Platform: Internet Explorer
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by oleksandr)

Background

See #58.

There were reports of a corrupt (not full) patterns.ini file a few times, which is pretty bad since it doesn't get updated until the next update interval and so user gets unpredictable blocking behavior.

What to change

Make sure that whenever we have received a signal to exit from AdblockPlusEngine we are not interrupting any I/O operations.
Also we need to make sure that the engine does not shut down not having replied anything to the browser.

Essentially this TODO has to be addressed: https://github.com/abby-sergz/adblockplusie/commit/6caf0e7281c3c4e5955cc6a63e3adf85bdd523a5#diff-cb4693d37bb412be109c3776d5c985d8R423

Change History (24)

comment:1 Changed 6 years ago by philll

  • Blocking 58 added

comment:2 Changed 6 years ago by trev

  • Reporter changed from philll to trev

comment:3 Changed 6 years ago by fhd

  • Priority changed from Unknown to P3
  • Status changed from new to accepted

comment:4 Changed 6 years ago by fhd

  • Owner oleksandr deleted
  • Status changed from accepted to assigned

comment:5 Changed 6 years ago by fhd

  • Status changed from assigned to accepted

comment:6 Changed 6 years ago by oleksandr

  • Owner set to oleksandr
  • Status changed from accepted to assigned

comment:7 Changed 5 years ago by philll

  • Status changed from assigned to new

The assigned state will be dropped by #403

comment:8 Changed 5 years ago by oleksandr

  • Owner changed from oleksandr to sergz
  • Platform set to Unknown
  • Ready unset

comment:9 Changed 5 years ago by oleksandr

This means that we need to make sure we are not interrupting any I/O operations with closing of AdblockPlusEngine.

comment:10 Changed 5 years ago by oleksandr

  • Platform changed from Unknown to Internet Explorer
  • Ready set

This means that we need to make sure we are not interrupting any I/O operations with closing of AdblockPlusEngine.

comment:11 Changed 5 years ago by trev

  • Cc oleksandr added

While I am supposedly the reporter of this, I don't really understand what it is about. What's the problem with interrupting downloads on shut down? We do it on every other platform, the engine will simply restart the download when it is started later.

comment:12 Changed 5 years ago by oleksandr

  • Description modified (diff)

comment:13 Changed 5 years ago by oleksandr

  • Description modified (diff)
  • Summary changed from [ABP IE 1.1 cleanup] Make sure ABP engine handles current filterlist download gracefully when shutting down. to [ABP IE 1.1 cleanup] Make sure ABP engine closes gracefully.

Modified the ticket description to be more precise.

comment:14 Changed 5 years ago by philll

@trev: looks like this is one of the issues that I migrated from trello in your name, as the trello card was from you.

comment:15 Changed 5 years ago by trev

@philll: It actually sounds like one of my review comments but the context is missing.

comment:16 Changed 5 years ago by eric@…

I think I was the one that first raised this issue, because it arose originally as an installation issue.

When IE shuts down, it shuts down the BHO as well. That part works just fine. The problem is that the engine does not shut down when IE does. In all cases, this is a defect; if IE isn't running, then no part of ABP should be either. Ordinarily, though, it's in the class of minor defects; it just sits there taking up a process entry and some swap space and might occasionally wake up for updates. You can kill the process manually in this state with no adverse affects. So this is the first aspect of the issue: to shut down the engine when IE terminates.

During installation, though, it causes more serious problems. To avoid having Windows Installer schedule a reboot, it's necessary to have the both BHO DLL and the engine closed at the time of installation. (All that would be needed would be to reboot IE itself rather than the whole system, but Windows Installer has no way of specifying such a restricted dependency; it's the whole system or nothing.) The custom action inside the installer sends WM_ENDSESSION to windows of both IE and the engine process, and if those don't work WM_CLOSE. Now ordinarily IE will shut down just fine and take the plugin with it. The engine, however, doesn't have a way of separately honoring this window message, because the only time it has a window is for the update dialog box. So the second aspect of this issue is to have a hidden window associated with the engine that will honor WM_ENDSESSION.

The third aspect is the question of pending downloads at exit. During installation, it would be better to terminate everything immediately to reduce installation time. This would include interrupting any pending download at the least. (Perhaps we also want to enter a "terminating" state and short-circuit any pipe queries.) In ordinary use, we could complete the download normally in the background and then (silently) exit. Minimally, though, we need to do either one of these and ensure that the filter lists are in a consistent state on disk. And that's the third part of the issue, to ensure that no matter what happens, we leave things in a consistent state.

comment:17 Changed 5 years ago by sergz

The problem is that the engine does not shut down when IE does.

The Engine should not be running when there are no clients and I think it is how it works now, the number of clients is zero => exit

to shut down the engine when IE terminates.

Not sure about it, I would like to avoid sticking to the IE, we can have other clients and it's not a problem for the Engine, it should not know anything about the clients.

All that would be needed would be to reboot IE itself rather than the whole system, but Windows Installer has no way of specifying such a restricted dependency; it's the whole system or nothing.

Could restart manager http://msdn.microsoft.com/en-us/library/windows/desktop/cc948910(v=vs.85).aspx help here?

During installation, it would be better to terminate everything immediately to reduce installation time. This would include interrupting any pending download at the least.

I would like to make the downloader interruptable. It's very easy, just pass delegate like bool IsCanceling() to the downloader/updater and call it when it's safe to stop the downloading process from the downloader/updater. I think it does not cause a big delay and users won't mind about it, especially if we can provide them with some progress indicator.

The value returned by the delegate can be based on the current number of active clients as well as we can have a hidden window (only for message loop) to be notified by the installer.

Minimally, though, we need to do either one of these and ensure that the filter lists are in a consistent state on disk. And that's the third part of the issue, to ensure that no matter what happens, we leave things in a consistent state.

It should be checked when the engine is starting, and fixed by the enegine if it's necessary, for example, by downloading a fresh list.

comment:18 Changed 5 years ago by eric@…

  • Blocking 1195 added

comment:19 Changed 5 years ago by eric@…

  • Blocking 1202 added

comment:20 Changed 5 years ago by eric@…

  • Blocked By 1652 added

comment:21 Changed 5 years ago by sergz

  • Owner sergz deleted

comment:22 Changed 4 years ago by oleksandr

  • Blocking 58 removed
  • Tester set to Unknown

comment:23 Changed 4 years ago by oleksandr

  • Summary changed from [ABP IE 1.1 cleanup] Make sure ABP engine closes gracefully. to Make sure ABP engine closes gracefully.

comment:24 Changed 3 weeks ago by Elegantitservices

Nice Post

Click here to Know more about [Salesforce Training in Bangalore]https://www.elegantitservices.com/Best-Salesforce-Training-Institutes-in-Bangalore.html

Note: See TracTickets for help on using tickets.