This started out as a project to fix the read-only service test I just
added. Initializing two history service objects in a row was racy,
because I needed the first history service to be initialized before
creating the second one, but there was no way to ensure that. This was
only an issue for this one test, though; real Epiphany browser mode of
course only creates one history service, so I assumed it was not a big
problem.
Fix this first issue using a condition variable to ensure the GObject
initialization doesn't complete until after the history service has
actually created the SQLite database.
In doing this, I discovered a second bug. The use of the condition
variable altered the timing slightly, and caused the history filename
property to not be set in time when entering the history service thread.
In fact, it's kind of amazing that the history service ever worked at
all, because there is absolutely nothing here to guarantee that the
filename and read-only properties have been initialized prior to
starting the history service thread. So the database filename could be
NULL when opening the database, which is a great way to lose all your
history. Also, it could also be in read-only mode here even if it is
supposed to be read/write mode, which is going to cause failures after
today's commits. Fix this by adding a constructed function and starting
the history thread from there, instead of doing it in init. This means
that the history thread will not be started until after properties have
been set. Note that, while I could not reproduce this bug on my machine
until after adding the condition variable to fix the first bug, that was
just due to timing and luck; it was already broken before.
history-service: Ensure thread member is initialized before use
We have assertions to ensure that several functions are only ever called
on the history thread. But the first such assertion, at the top of
run_history_service_thread, sometimes fails when running the tests. It
is racy. Use a mutex to fix this.
These assertions are actually executed at runtime for end users, so it's
surprising that nobody has ever reported a bug about this.
We also need to be sure to initialize the async queue before running the
history service thread. The mutex is needed as a memory barrier here, so
it's not possible to remove the mutex by removing the assertions except
in debug mode, which is something I considered.
Fix search provider horribly breaking history service
If the search provider is running, all database transactions will fail
because the search provider will take a write lock on the database.
Ouch! This is worth a good string of profanities....
Notably, this causes opening the database to fail if you searched for
anything in the shell overview in the minute prior to starting Epiphany.
(One minute is our search provider timeout.) Then no history will ever
get saved, ever. I think. Something like that.
So, although our history service has read-only mode, it's enforced at
the history service level, not the SQLite connection level. SQLite
actually has a read-only mode, which we are not using, and which we need
to use in the search provider if we want to have any chance of reliably
saving history.
Accordingly, give EphySQLiteConnection a mode property, to indicate
whether it is in writable mode or read-only mode. Teach all callers to
set it properly. Use it, rather than a boolean, when creating the
EphyHistoryService, since boolean parameters are hard to read at call
sites. And actually put the underlying SQLite connection in read-only
mode when set.
Don't open transactions or ever attempt to rollback in read-only mode,
because that doesn't make any sense. This should never have been
happening due to the history service level read-only checks, but it
should be enforced at the SQLite connection level now, too.
Avoid initializing tables when opening the database in read-only mode.
This is obviously writing to the database, and now that we really have a
read-only SQLite connection it fails. As it should.
SQLite connection creation will now fail in case the connection is
read-only and the database does not yet exist; it will no longer be
created anyway. So handle this case gracefully. It's fine for the
history service to return nothing in this case. This has the small
advantage that the history thread will quit immediately after it's
created in this case, so it's not constantly running if there's no
history in incognito mode anymore. To check for this condition, we
expose the underlying SQLite error; previously, only the error message
was exposed outside of EphySQLiteConnection. Exposing the error isn't
really necessary or sufficient, though, since it's super generic and we
have to check if the file actually exists on disk anyway.
Test it. Ensure that a read/write history service functions properly if
it's running at the same time as a read-only history service. Using two
read/write services here fails very badly, but when one of the services
is read-only it works fine.
Also, remove the original read-only service test. It only ever tested
that creating a read-only history service with an empty history database
would succeed. And, as I just explained, that fails now.
Lastly, stop running a second history service for the search provider.
It needed its own once upon a time when the search provider did not run
an EphyShell instance. That changed when we stopped using the default
web context, because nothing works without EphyEmbedShell now, as all
sorts of places need it to get the embed's web context. And since
EphyEmbedShell runs its own history service, the search provider can
just use that now instead of running its own separate one.
sqlite-connection: Do not ignore errors when executing commands
This file is so careful to handle errors properly everywhere EXCEPT the
point where actual SQLite commands are executed. The history database is
pretty much totally broken right now; having error messages would be
helpful thank you!
uri-tester: Ensure regexps are properly constructed
This adds a few more cases to the escaping done when converting an AdBlock
non-regepx "simple pattern" from a rule into a GRegex. This patch does the
following:
- Adds escaping to some of the regexp metacharacters which were not being
handled: (){}+.|\
- Adds support for using a vertical bar at the end of a pattern to anchor the
match at the end.
- Adds support for using ^ to match a "separator character" (a non-letter,
non-number, or one of _-.%).
This also adds as much comment lines as code, which in this particular case
is probably a good thing, so reading the code in the future does not need
checking each case against the GRegex documentation.
