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

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Places
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
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.
Priority: -- → P1
(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.
Flags: needinfo?(kit)
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.
Flags: needinfo?(kit)
(Assignee)

Updated

a year ago
Assignee: nobody → standard8
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
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).
Attachment #8862126 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 7

a year ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70664c1a78fd
PlacesUtils.history.remove should implement chunking to avoid SQL stack size issues. r=mak

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/70664c1a78fd
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.