Closed Bug 448584 Opened 12 years ago Closed 12 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3.1b1

People

(Reporter: whimboo, Assigned: mak)

References

Details

(Keywords: dataloss, testcase, verified1.9.0.5)

Attachments

(3 files, 1 obsolete 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.
Flags: blocking1.9.0.2?
Flags: blocking-firefox3.1?
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.
Keywords: testcase
Assignee: nobody → dietrich
Priority: -- → P1
Target Milestone: --- → Firefox 3.1a2
Not blocking 1.9.0.2. We'll revisit for 1.9.0.3 if this gets some work done.
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.3?
Flags: blocking1.9.0.2?
Flags: blocking1.9.0.2-
Dietrich, any status update available?
Blocks: 384370
No longer depends on: 429350
Whiteboard: [needs status update dietrich]
stealing bug patch coming, hope to push before 3.1beta1 freeze
Assignee: dietrich → mak77
Status: NEW → ASSIGNED
Target Milestone: Firefox 3.1a2 → Firefox 3.1b1
Summary: Make JSON backup less error-prone (don't stop while saving data) → Don't save invalid uri nodes while doing a JSON backup (don't stop while saving data)
Whiteboard: [needs status update dietrich]
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #341017 - Flags: review?(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
Attachment #341017 - Flags: review?(dietrich) → review+
Attached patch for checkinSplinter Review
Attachment #341017 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/7d2bb64668b0
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #341077 - Flags: approval1.9.0.4?
Comment on attachment 341077 [details] [diff] [review]
for checkin

asking approval for branch
Flags: blocking-firefox3.1? → in-testsuite+
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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(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).
(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?
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)
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Not blocking. mconnor will look at the approval request, but not for this release.
Flags: blocking1.9.0.4? → blocking1.9.0.4-
Attachment #341077 - Flags: approval1.9.0.4? → approval1.9.0.5+
Comment on attachment 341077 [details] [diff] [review]
for checkin

Let's get this into 3.0.5, it's baked and has tests.
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
Keywords: fixed1.9.0.5
Blocks: 435204
Blocks: 441672
Blocks: 463812
Duplicate of this bug: 463812
No longer blocks: 463812
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.
Blocks: 465684
Blocks: 468383
Duplicate of this bug: 469155
Duplicate of this bug: 474526
Depends on: 512273
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.