1348330 - PlacesUtils.history.remove should implement chunking to avoid SQL stack size issues

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Mark Banner (:standard8)]

Per bug 1087255 comment 47 - 51, relating to History.jsm / remove (aka PlacesUtils.history.remove):

> I think we need to file a follow-up, because remove is using a plain IN
> clause and it could easily reach the SQL length limit. Especially because it
> puts urls into the IN clause, and urls can be large. I'm fine with removing
> the chunking here, though we have to add it directly into the API
> implementation (even simple chunked calls to the internal remove() would do).
> Without that, there's concrete risk that people will try to remove thousands
> entries, and remove will just throw on SQL limit.

Comment 1

[Lina Butler [:lina]]

Do you think it's worth adding a Sqlite.jsm helper to build a chunked "IN" clause? Sync fetches history visits in chunks, too, and the bookmarks engine could use it for bulk deletes.

Comment 2

[Marco Bonardo [:mak]]

(In reply to Kit Cambridge [:kitcambridge] from comment #1)
> Do you think it's worth adding a Sqlite.jsm helper to build a chunked "IN"
> clause? Sync fetches history visits in chunks, too, and the bookmarks engine
> could use it for bulk deletes.

Do you have some specific API in mind to suggest?
Actually, it could also be feasible to have Sqlite.jsm itself detect when a query is very long, and if so check if it can be rewritten by splitting an IN clause. Though this may transparently break consumer assumptions that the query runs in one-shot.

The other fact is that in general we should not use large IN clauses, instead we should implement bug 483318, then Sqlite.jsm would just take a query with an " IN carray(?, ?)" and an array param, and do the binding through a carray (it would invoke a special method in mozStorage to convert the js array into a C array and bind the C array address in the query).
Due to that, I was not much prone to add an API for chunking that "ideally" we should not use.

That said, since we have no idea when bug 483318 will happen, and XUL add-ons are going away, we can definitely have an helper and a plan to remove it.

Comment 3

[Lina Butler [:lina]]

Neat, I didn't know `carray` existed. Since bug 483318 will eventually solve this, I don't think we need a generic helper in the meantime. It makes sense to have `remove` and friends implement their own chunking for now, and we can convert them once we support binding arrays.

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1348330 - PlacesUtils.history.remove should implement chunking to avoid SQL stack size issues.

Review commit: https://reviewboard.mozilla.org/r/134102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134102/

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8862126 [details]
Bug 1348330 - PlacesUtils.history.remove should implement chunking to avoid SQL stack size issues.

https://reviewboard.mozilla.org/r/134102/#review137318

Thanks. You should run this through try debug artifact builds, and retrigger some times to ensure we don't hit a timeout.
Something like:
./mach try -b d -p linux,linux64,macosx64,win32,win64 -u xpcshell -t none --artifact --rebuild 10

::: toolkit/components/places/History.jsm:319
(Diff revision 1)
> -    return PlacesUtils.withConnectionWrapper("History.jsm: remove",
> -      db => remove(db, normalizedPages, onResult));
> +    return Task.spawn(function* () {
> +      let removedPages = false;
> +      let count = 0;
> +      while (guids.length || urls.length) {
> +        if (count == 2) {
> +          count = 0;

can we use the modulo operator here instead of resetting the counter?
(count && count %2 == 0) should do

::: toolkit/components/places/tests/history/test_removeMany.js:9
(Diff revision 1)
> +// Tests for `History.remove` with removing many urls, as implemented in
> +// History.jsm.
> +
> +"use strict";
> +
> +Cu.importGlobalProperties(["URL"]);

hm, head_common.js does this, it should not be needed.

::: toolkit/components/places/tests/history/test_removeMany.js:97
(Diff revision 1)
> +    onFrecencyChanged(aURI) {
> +      let origin = pages.find(x => x.uri.spec == aURI.spec);
> +      Assert.ok(origin);
> +      Assert.ok(origin.hasBookmark, "Observing onFrecencyChanged on a page with a bookmark");
> +      origin.onFrecencyChangedCalled = true;
> +      // We do not make sure that `origin.onFrecencyChangedCalled` is `false`, as

this comment can be removed, looks like a typo (incomplete).

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8862126 [details]
Bug 1348330 - PlacesUtils.history.remove should implement chunking to avoid SQL stack size issues.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134102/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70664c1a78fd
PlacesUtils.history.remove should implement chunking to avoid SQL stack size issues. r=mak

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70664c1a78fd