458683 - Backup only uri nodes into tag containers

Status: VERIFIED FIXED

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

Description

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

When restoring tag container's children we simply call tagURI, but if the tag container has some bogus child (for example a separator or a container) the call to _uri(node.uri) will throw.

Cleaning up tag children should be done in bug 431558, but still we should try to not stop when doing backup and restore, so we should add a couple checks to skip those bogus nodes.

critical, because we must provide reliable backups to users.

Comment 1

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

(In reply to comment #0)
> critical, because we must provide reliable backups to users.

Which also means we have dataloss here.

Comment 2

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

Attachment: wip 0.1

first attempt, avoid writing bogus children, avoid restoring bogus items, don't stop if we can't remove a tag container. needs unit test.

Comment 3

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

Attachment: patch

with unit test

Comment 4

[Dietrich Ayala (:dietrich)]

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

>diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js
>--- a/toolkit/components/places/src/utils.js
>+++ b/toolkit/components/places/src/utils.js
>@@ -1072,17 +1072,25 @@ var PlacesUtils = {
> 
>           for (var i = 0; i < childIds.length; i++) {
>             var rootItemId = childIds[i];
>             if (rootItemId == this._utils.tagsFolderId) {
>               // remove tags via the tagging service
>               var tags = this._utils.tagging.allTags;
>               var uris = [];
>               for (let i in tags) {
>-                var tagURIs = this._utils.tagging.getURIsForTag(tags[i]);
>+                try {
>+                  var tagURIs = this._utils.tagging.getURIsForTag(tags[i]);
>+                } catch (ex) {

hm, does that really throw? seems like it'd be better for getURIsForTag to return empty array instead, since it's not really a program error to have no URIs with the specified tag. please fix the tag service in this patch, or file a followup.


>@@ -1440,36 +1452,54 @@ var PlacesUtils = {
>       // set index in order received
>       // XXX handy shortcut, but are there cases where we don't want
>       // to export using the sorting provided by the query?
>       if (aIndex)
>         node.index = aIndex;
> 
>       addGenericProperties(bNode, node);
> 
>+      var parent = bNode.parent;
>+      var grandParent = parent ? parent.parent : null;
>+
>       if (self.nodeIsURI(bNode)) {
>+         // Tag root accept only folder nodes
>+         if (parent && parent.itemId == self.tagsFolderId)
>+          return false;

indent is wrong in a couple of places here

r=me otherwise

Comment 5

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

Attachment: patch

(In reply to comment #4)
> (From update of attachment 342277 [details] [diff] [review])
> >diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js
> >--- a/toolkit/components/places/src/utils.js
> >+++ b/toolkit/components/places/src/utils.js
> >@@ -1072,17 +1072,25 @@ var PlacesUtils = {
> > 
> >           for (var i = 0; i < childIds.length; i++) {
> >             var rootItemId = childIds[i];
> >             if (rootItemId == this._utils.tagsFolderId) {
> >               // remove tags via the tagging service
> >               var tags = this._utils.tagging.allTags;
> >               var uris = [];
> >               for (let i in tags) {
> >-                var tagURIs = this._utils.tagging.getURIsForTag(tags[i]);
> >+                try {
> >+                  var tagURIs = this._utils.tagging.getURIsForTag(tags[i]);
> >+                } catch (ex) {
> 
> hm, does that really throw? seems like it'd be better for getURIsForTag to
> return empty array instead, since it's not really a program error to have no
> URIs with the specified tag. please fix the tag service in this patch, or file
> a followup.

the problem is that we are not throwing because we don't have any uri, instead we are throwing here:

  getURIsForTag: function TS_getURIsForTag(aTag) {
    if (aTag.length == 0)
      throw Cr.NS_ERROR_INVALID_ARG;

    var uris = [];
    var tagNode = this._getTagNode(aTag);
    if (tagNode) {
      tagNode.QueryInterface(Ci.nsINavHistoryContainerResultNode);
      tagNode.containerOpen = true;
      var cc = tagNode.childCount;
      for (var i=0; i < cc; i++)
        uris.push(gIoService.newURI(tagNode.getChild(i).uri, null, null));
      tagNode.containerOpen = false;
    }

gIoService.newURI(tagNode.getChild(i).uri, null, null) throws because the node is not an uri node, and this is an edge corruption case due to us allowing users to put containers/separators inside tag containers in previous versions.
We would not throw for an empty tag container.

i moved the try catch to tagging service since a bogus child would also throw and stop from getting all other valid uri nodes, asking additional review on the change (i also fixed those wrong indent)

Comment 6

[Dietrich Ayala (:dietrich)]

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

r=me, thanks

Comment 7

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

Attachment: patch

fixed a minor issue due to possible receiving empty tags (for example a separator is seen as an empty tag), ready for pushing

Comment 8

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

http://hg.mozilla.org/mozilla-central/rev/71ac681c2d80

Comment 9

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

Works fantastic. Thanks Marco!

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081016 Minefield/3.1b2pre and test case on bug 448584.

Marco, do the automated tests cover all issue or do we need a Litmus test?

Comment 10

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

I believe this is something we should consider for 1.9.0.4.

Comment 11

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

(In reply to comment #9)
> Marco, do the automated tests cover all issue or do we need a Litmus test?

i really doubt we could test this with litmus, causes of bogus tag containers are gone (we don't allow anymore users to add/drag something inside them) and testing requires direct api calls. The test ensures that if something bad is inserted into them we are still able to backup and restore, that should be enough imho.

Comment 12

[Daniel Veditz [:dveditz]]

Not blocking, no evidence this is a major issue worth taking the code risk on. Feel free to request approval from mconnor and justify the risk on the patch if you disagree.

Comment 13

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

Marco, as we got approval for 1.9.0.5 on bug 448584 what's the risk of your patch here? It's a real dataloss bug where users wont be able to restore their bookmarks at all. I think it's worth thinking about.

Comment 14

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

bug 448584 was much smaller, this is changing some JSON backup behaviour so risk is a bit higher than there. On the other side comes with a unit test and is tested on trunk nightlies. Moreover we have evidence of many users unable to restore bookmarks due to this, and bookmarks restore safety is quite important.

I would ask approval, but i think Dietrich should approve first.

Comment 15

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

(In reply to comment #14)
> I would ask approval, but i think Dietrich should approve first.

Sure. Does the latest patch apply cleanly on branch or do you have to update it? Please don't forget to set the approval flag then. Thanks.

Comment 16

[Dietrich Ayala (:dietrich)]

talked w/ marco on irc: bug 448584 doesn't ensure that tag containers can't have non-URI children, so we must have this fix on branch as well to ensure full backup and restore for users that have polluted tag containers.

Comment 17

[Dietrich Ayala (:dietrich)]

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

branch drivers: see comments 14-16.

Comment 18

[Daniel Veditz [:dveditz]]

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

Approved for 1.9.0.5, a=dveditz

Comment 19

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsTaggingService.js;
/cvsroot/mozilla/toolkit/components/places/src/nsTaggingService.js,v  <--  nsTaggingService.js
new revision: 1.26; previous revision: 1.25
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.24; previous revision: 1.23
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_458683.js,v
done
Checking in toolkit/components/places/tests/bookmarks/test_458683.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_458683.js,v  <--  test_458683.js
initial revision: 1.1
done

Comment 20

[Dietrich Ayala (:dietrich)]

Attachment: test bustage fix for branch

Comment 21

[Dietrich Ayala (:dietrich)]

(In reply to comment #20)
> Created an attachment (id=348796) [details]
> test bustage fix for branch

which i just checked in

Comment 22

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

Verified that this is fixed for 1.9.0.5. Dietrich's test is passing in current 1.9.0 builds.

Comment 24

[Scott]

Don't know if this is the same bug, or a new bug with the same results, but my bookmarks.json files are showing a size of 0. It doesn't matter if they are created automatically or by exporting from "organize bookmarks".
This is occurring in both existing and newly created profiles. There is no problem exporting bookmarks as html and I am not using Norton.
When I try to export bookmarks the error console shows this message, "Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsINavHistoryService.executeQuery]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: file:///Applications/Firefox3.app/Contents/MacOS/modules/utils.js :: PU_backupBookmarksToFile :: line 1554"  data: no]"

I am showing the problem with both 3.2a1 and 3.06
Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20090209 Minefield/3.2a1pre - Build ID: 20090209020504

Comment 25

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

Scott, do you eventually see bug 477739? If yes, please comment there. It would be great to have some reliable STR. At least you shouldn't be affected by this bug. Means if you don't think it's related to bug 477739, please file a new one and cc me please.

Comment 26

[Scott]

I'm seeing bug 477739 in the preexisting profile, not yet seeing that behavior in the new profile

Comment 27

[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