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

RESOLVED FIXED in Firefox 66

Status

()

defect
P2
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox65 wontfix, firefox66 fixed, firefox67 fixed)

Details

(Whiteboard: [qa-66b-p2])

Attachments

(1 attachment)

Assignee

Description

5 months ago

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 2

5 months ago
Pushed by mbanner@mozilla.com:
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

5 months ago
bugherder
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Can we uplift this?

Flags: needinfo?(standard8)
Assignee

Comment 5

5 months ago

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

Flags: needinfo?(standard8)
Attachment #9039848 - Flags: approval-mozilla-beta?

When did the regression start? With 64?

Flags: needinfo?(standard8)
Assignee

Comment 7

5 months ago

(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.

Flags: needinfo?(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

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

Attachment #9039848 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [qa-66b-p2]
You need to log in before you can comment on or make changes to this bug.