1523665 - Bookmarks.insertTree should take account of invalid bookmark info causing an insertion of zero items

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P2)

Description

[Mark Banner (:standard8)]

In bug 1523096 we have seen that it is possible for insertTree to be called for a root, but to filter out all items to be inserted (as they are invalid), and as a result, it attempts to insert with an empty array.

This causes sqlite to fail, and for example import transactions to abort.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1523665 - Bookmarks.insertTree should take account of invalid bookmark info causing an insertion of zero items. r?mak

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba41803aea53
Bookmarks.insertTree should take account of invalid bookmark info causing an insertion of zero items. r=mak

Comment 3

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ba41803aea53

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Can we uplift this?

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 9039848 [details]
Bug 1523665 - Bookmarks.insertTree should take account of invalid bookmark info causing an insertion of zero items. r?mak

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bookmarks Async APIs

User impact if declined

Users importing bookmarks (or restoring from backups), may have incomplete restoration and warnings that the restore failed. Whilst we attempt to filter out invalid records, if the filtering leads to adding nothing to a top-level folder, then the restore will fail.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Simple patch to skip attempting to insert records when there are no records to insert.

String changes made/needed

None

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

When did the regression start? With 64?

Comment 7

[Mark Banner (:standard8)]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #6)

When did the regression start? With 64?

I think if anything it probably started with bug 1095426 in 56... however you could reproduce similar bugs depending on the exact corruption from before then. We've already fixed bugs handling bad types or bad urls (bug 1373610/bug 1425764), this is the latest step in improving corruption/unexpected formats for handling for backups.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9039848 [details]
Bug 1523665 - Bookmarks.insertTree should take account of invalid bookmark info causing an insertion of zero items. r?mak

Fix for some cases of corrupt data; adds tests.
OK to uplift for beta 4.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6edb580f40bf