Closed Bug 1384696 Opened 4 years ago Closed 5 months 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 Graveyard :: Data Providers, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: Grisha, Assigned: jwu, NeedInfo)

References

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)
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.