Open Bug 1384696 Opened 3 years ago Updated 1 year ago

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

Categories

(Firefox for Android :: Data Providers, enhancement, P2)

enhancement

Tracking

()

People

(Reporter: Grisha, Assigned: jwu, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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
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
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)
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+
NI myself to re-review this and land it.
Flags: needinfo?(gkruglov)
Priority: -- → P2

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jwu, could you have a look please?

Flags: needinfo?(topwu.tw)
You need to log in before you can comment on or make changes to this bug.