448584 - Don't save invalid uri nodes while doing a JSON backup (don't stop while saving data)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: broken json backup file

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/2008073003 Minefield/3.1a2pre ID:2008073003 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2pre) Gecko/2008072704 GranParadiso/3.0.2pre ID:2008072704

Today I tried to re-import all my bookmarks in using the JSON backup feature. I made a backup but failed while restoring the bookmarks with the following error message: Unable to process the backup file.

I checked what happens and found that the root cause of this is bug 429350. I have some tags left which don't have any children. Making a backup of such a places file the backup script will stop immediately while writing the data to the json file. Have a look at the attached json file.

While doing the backup following error is shown in the Error Console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: file:///Users/henrik/Desktop/Minefield.app/Contents/MacOS/modules/utils.js :: PU__uri :: line 154"  data: no]

The backup script shouldn't fail in such cases when no child nodes are available, no URIs are set or something else. We should always make sure that we write correct backup files otherwise probably lost bookmarks cannot be restored. Perhaps we could also run a syntax check after the creation of the json file?

This is a major issue where a lot of people can run into. All of them could run into this dataloss bug and won't be able to restore their bookmarks.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Affected places.sqlite file

This places.sqlite file will have some tags left which don't have a children. You can reproduce this issue with following steps:

1. Copy places.sqlite in your test profile while Firefox is closed
2. Start Firefox and open the library
3. Run a json backup of the bookmarks
4. Try to restore the bookmarks from the created json file

=> The json backup file is only half-written and cannot be used to restore the bookmarks.

Comment 2

[Samuel Sidler (old account; do not CC)]

Not blocking 1.9.0.2. We'll revisit for 1.9.0.3 if this gets some work done.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dietrich, any status update available?

Comment 4

[Marco Bonardo [:mak]]

stealing bug patch coming, hope to push before 3.1beta1 freeze

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch

with unit test, not exactly a politically correct test, but this should prevent us from failing to backup when something bad happens, so i had to do something bad (using other bugs would stop working when they'll be fixed, while this will always be bad!)

I'll start investigating on how we can finish up with a bookmark without a place (since is what i see in the attached database), but i think that is due to 3.0 alpha stages where we were allowing doing bad things.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 341017 [details] [diff] [review]
patch


>+/*
>+  This test is:
>+    - don't try to add invalid uri noded to a JSON backup

spelling nit: s/noded/node/

>+  // populate db
>+  tests.forEach(function(aTest) {
>+    aTest.populate();
>+    // sanity
>+    aTest.validate(2);
>+    // Something in the code went wrong and we finish up loosing the place, so

spelling nit: s/loosing/losing/

r=me, thanks

Comment 7

[Marco Bonardo [:mak]]

Attachment: for checkin

Comment 8

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/7d2bb64668b0

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 341077 [details] [diff] [review]
for checkin

asking approval for branch

Comment 10

[Tracy Walker [:tracy]]

I am unable to verify this with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b1pre) Gecko/20081003 Minefield/3.1b1pre

Henrik, please run your test cases when you have a chance, thanks.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sorry Marco but the backup json file still contains errors. It cannot be restored even in a fresh profile. Means we probably have incomplete data which results in dataloss. Please take my testcase (attachment 331784 [details]) and do a backup of the data. You will see that it cannot be restored. There is no error shown during the backup.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I was able to find the cause of the remaining issue and I'm not sure that this is still covered by this bug due to the change of the summary. Marco, should I better file a new bug?

So why the backup cannot be restored is due to bug 429350. Tags which cannot be deleted makes the JSON backup file unusable for a restore action. A lot of users have already complained about that so I believe anyone of them don't have any working backup of their bookmark. We should handle this with high priority.

Comment 13

[Marco Bonardo [:mak]]

(In reply to comment #12)
> So why the backup cannot be restored is due to bug 429350. Tags which cannot be
> deleted makes the JSON backup file unusable for a restore action.

Do you mean we are unable to delete the folder when restoring, so everything fails, or that we are saving bad tag children to the backup file, so it's invalid? The latter would me more dangerous, the former could probably be mitigated restoring the backup in a new profile (does that work?).
I'll check the cause, but i think we could file a new bug to see if we should not block the backup/restore at that point and provide a unit test. Instead tag containers cleanup should be done by preventive maintainance patch in bug 431558 (that patch needs a total revise since we are moving toward using async queries, i'll try to finish it in the next days).

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #13)
> Do you mean we are unable to delete the folder when restoring, so everything
> fails, or that we are saving bad tag children to the backup file, so it's
> invalid? The latter would me more dangerous, the former could probably be
> mitigated restoring the backup in a new profile (does that work?).

Yes, its the latter one. All the tags were saved to the JSON file. Even that ones who cannot be manually deleted and persist after all the bookmarks which uses these tags were deleted. Probably the JSON file is correct but contains data which let us fail when restoring. As what I've written this also occurs by trying to restore the JSON file in a fresh profile.

> I'll check the cause, but i think we could file a new bug to see if we should
> not block the backup/restore at that point and provide a unit test. Instead tag
> containers cleanup should be done by preventive maintainance patch in bug
> 431558 (that patch needs a total revise since we are moving toward using async
> queries, i'll try to finish it in the next days).

So creating a new bug and setting it as depends on bug 431558 would be ok? I could file it later today.

What I have to do to verify this bug? I would try to do this with a new profile without using my places.sqlite testcase. What do I have to modify to test the fix? Just removing the URL from the bookmarks table?

Comment 15

[Marco Bonardo [:mak]]

so, this is caused by having separators/containers in tag containers, so for sure the preventive maintainance will solve this. However we could add an additional layer of check that could avoid to write bogus tag children or avoid restoring bogus tag children.

i've filed bug 458683, remarking this as fixed since bogus uri nodes are correctly skipped, thanks.

i think that to verify this the test unit is enough, however you should corrupt an uri, that you can do by:

1. go to moz_places and remove a place for a bookmarked item (so take an fk from moz_bookmarks and do DELETE FROM moz_places WHERE id = your_fk), in the test i'm simply replacing the fk with a non existant one that's the same

1b. do the same but instead of deleting it change the uri to an invalid value, (so take an fk from moz_bookmarks and do UPDATE moz_places SET uri = "foo" WHERE id = your_fk)

2. backup & restore (clearly don't use a profile with bogus tag containers)

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified with the given steps (at least with a small change - used url instead of uri in step 1b) by using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081016 Minefield/3.1b2pre.

Thanks Marco! The backup and restore feature works without an error. No way to find another issue right now.

Comment 17

[Samuel Sidler (old account; do not CC)]

Not blocking. mconnor will look at the approval request, but not for this release.

Comment 18

[Mike Connor [:mconnor]]

Comment on attachment 341077 [details] [diff] [review]
for checkin

Let's get this into 3.0.5, it's baked and has tests.

Comment 19

[Shawn Wilsher :sdwilsh]

I had to modify how we got the db in the test file, but otherwise this patch applied just fine.

Checking in toolkit/components/places/src/utils.js;
new revision: 1.22; previous revision: 1.21
Checking in toolkit/components/places/tests/bookmarks/test_448584.js;
initial revision: 1.1

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Verified that this is fixed in 1.9.0.5 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008120105 GranParadiso/3.0.5pre.

Comment 24

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv