Crash while deleting a folder which has more than SQLITE_MAX_VARIABLE_NUMBER child folders on any of the tree levels

NEW
Assigned to

Status

()

Firefox for Android
Data Providers
3 months ago
16 days ago

People

(Reporter: Grisha, Assigned: jwu)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

3 months ago
We don't perform "chunking" while traversing through the bookmark tree in [0], which will result in a crash (failing to prepare sql statement, "too many variables") for any structure which has more than SQLITE_max_variable_number folders on any tree level. Example:

root
-> folder
--> bookmark
--> folder-1
--> folder-2
...
--> folder-1000

'folder' has one bookmark and 1000 folders in it. If we were to delete guid='folder', we'll make a list of all folders whose parent='folder', and perform a SQL statement which looks like "SELECT ... WHERE parent IN (?, ?, ?... for each child folder..., ?, ?)".

This will crash because number of child folders (1000) exceeds SQL's max_variable_number (999 by default).

[0] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java#2364
Comment hidden (mozreview-request)
(Reporter)

Comment 2

3 months ago
I'll push a patch with a fix later this week, unless jwu can get to it faster.
Component: General → Data Providers
Summary: Crash while deleting a folder which has more than SQLITE_max_variable_number children folders on any of the tree levels → Crash while deleting a folder which has more than SQLITE_MAX_VARIABLE_NUMBER child folders on any of the tree levels
(Assignee)

Comment 3

3 months ago
Created attachment 8890658 [details] [diff] [review]
Query folders iteratively to prevent exceeding maximum variable count in a clause

Sorry for the incorrect use about database query. A patch is attached for reviewing. 

Also I made a little change in the test case to meet the requirement, hope my understanding is correct.
Assignee: nobody → topwu.tw
Attachment #8890658 - Flags: review?(gkruglov)
(Reporter)

Comment 4

3 months ago
Comment on attachment 8890658 [details] [diff] [review]
Query folders iteratively to prevent exceeding maximum variable count in a clause

Review of attachment 8890658 [details] [diff] [review]:
-----------------------------------------------------------------

Before landing this, can you add tests around boundary conditions of the chunking logic: triggering one under, at, and one above the variable limit cases.
Attachment #8890658 - Flags: review?(gkruglov) → review+
You need to log in before you can comment on or make changes to this bug.