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

()

P2
normal
a year ago
a year ago

People

(Reporter: Grisha, Assigned: jwu, NeedInfo)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year 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

a year 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

a year 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

a year 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+
(Reporter)

Comment 5

a year ago
NI myself to re-review this and land it.
Flags: needinfo?(gkruglov)

Updated

a year ago
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.