1129529 - Bookmark backup may not be restorable

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[dfgsdfgsdfg]

Attachment: just 1 three.json

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150122214805

Steps to reproduce:

- Add a tag to a bookmark, which is more than 100 characters long
- backup bookmarks to JSON
- restore bookmarks


Actual results:

JSON-Backup of bookmarks performs correctly (automated as well as manual backup), but restoring the backup causes error "backup-file cannnot be processed" (may be different, this is a 1:1 translation from the German locale).
All bookmarks in the backup-file before the one with the long tag are restored.
The bookmark with the long tag is restored, but without the tag.
All other bookmarks are not restored.



Expected results:

The restore process should just be able to handle more than 100 characters in the "tags" field - or Firefox should not allow to enter more than 100 chars in the tag field in the first place (the alter one is a bad solution in my opinion)

Comment 1

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

oh yes, this is terrible.

The fact is that tagURI throws if one tries to set a tag that is longer than 100 chars, we basically need to catch that exception.

The fix would require adding a try catch aroung tagURI here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/BookmarkJSONUtils.jsm#454

451           // TODO (bug 967196) the tagging service should trim by itself.
452           let tags = aData.tags.split(",").map(tag => tag.trim());
453           if (tags.length)
454             PlacesUtils.tagging.tagURI(NetUtil.newURI(aData.uri), tags);
455         }

It would also require writing an xpcshell-test, it should try to import a json with a bookmark having a tag longer than 100 chars and verify that everything is imported, except the tags for that bookmark.

Comment 2

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

as bonus points, we could filter the tags array and remove the invalid tag, so that other tags are still set.

Comment 3

[canaar]

Hi,

I am new to open source and I would like to work on this bug as my first bug. How should I proceed?

Thanks!

- abhilash

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

@ Abhilash Mhaisne - speak to Marco Bonardo [::mak] as he's the mentor of this bug

Comment 5

[canaar]

Mr. Bonardo, I'd like to work on this bug.

Comment 6

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

Hi, no worries.

First of all, I suggest you to go through this small introduction, if you didn't yet
https://developer.mozilla.org/en-US/docs/Introduction
Ensure you have a working simple Firefox build and that your mercurial is properly setup https://developer.mozilla.org/en-US/docs/Installing_Mercurial
Once you are there, you can start by comment 1, create a new patch addressing the code as explained there, then attach it and ask for feedback, then we'll look together into writing a test case.

Feel free to ask questions here or in the #introduction channel on irc.mozilla.org

Comment 7

[Ravi Shankar [:waffles]]

Hey Marco, I'm new to Mozilla. I've already installed the prerequisites (including Mercurial). And, I've chosen this bug as a starter. It's been told that the tag cannot be restored from the backup. When I checked it on my Firefox (developer edition), the tag space itself doesn't accept more than 100 chars. If we try to put more than 100 chars, it just disappears from the text area. So, I guess it's a problem in adding/modifying the bookmarks.

Comment 8

[Toby Daigneault]

Attachment: bug1129529_bookmarktag_length.diff

Comment 9

[Toby Daigneault]

Comment on attachment 8571517 [details] [diff] [review]
bug1129529_bookmarktag_length.diff

I removed the check from the method that converts the array of tags to an array of "tag object" which threw an uncaught expection.

I put the check in the method that actually add the tags to a bookmark in order to skip any tags that were longer than Ci.nsITaggingService.MAX_TAG_LENGTH.

This way  all other acceptable tags would still be applied and only invalid tags would be skipped.

Comment 10

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

Comment on attachment 8571517 [details] [diff] [review]
bug1129529_bookmarktag_length.diff

Review of attachment 8571517 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsTaggingService.js
@@ +147,5 @@
>          {
> +          if (tag.name.length > Ci.nsITaggingService.MAX_TAG_LENGTH) {
> +            // Tag name is too long, we are skipping this tag.
> +            return;
> +          }

So, your solution works, but the problem is that a developer that is using this API to set tags, and that sets too long tags, would never notice he is doing something wrong, for him the API would have worked.
For example, suppose he is setting a single tag, 101 long, after the API call, since he didn't get an exception, he'd think everything worked.

For this I can only see 2 solutions:
1. we keep throwing and we fix the callers so they filter out the tags BEFORE passing them to tagURI
2. we truncate the long tags to 100 chars

2 doesn't seem very nice, since it could generate a meaningless value, so I'd vote for 1.
There are 2 calls to tagURI in BookmarkJSONUtils.jsm
http://mxr.mozilla.org/mozilla-central/search?string=tagURI%28&find=BookmarkJSONUtils.jsm

The only meaningful one for this bug is this one (the other one sets only a single tag and already catches:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/BookmarkJSONUtils.jsm#461
459           let tags = aData.tags.split(",").map(tag => tag.trim());
460           if (tags.length)
461             PlacesUtils.tagging.tagURI(NetUtil.newURI(aData.uri), tags);

As you can see, line 459 that generates the tags array, seems like the perfect point where to add a .filter() that will skip too long tags, something like
let tags = aData.tags.split(",")
                     .map(tag => tag.trim())
                     .filter(tag => ... );
It would also be nice to add a comment above it, stating we skip too long tags.

Comment 11

[Toby Daigneault]

Thanks for the feedback Marc. I agree. I'll get on this tomorrow and submit a new patch.

Comment 12

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

thanks. If after this bug you are looking for something else, bug 967196 could be good for you, I noticed that tag.trim() here and we could remove it with that fix.

It is just a bit more complex involving a test (but should be easy to write).

Comment 13

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

Hi toby, any news about this bug?

Comment 14

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch

This adds the try/catch block and a test.
The test feeds a dummy bookmarks tree to importFromURL (using a data uri) and then checks that all the bookmarks are imported correctly, except for the tags on the one with the tag that is more than 100 characters.

Comment 15

[Drew Willcoxon :adw]

Comment on attachment 8607700 [details] [diff] [review]
Patch

Review of attachment 8607700 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Nihanth!  Looks OK to me.

Comment 16

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v1.1

This no longer base64 encodes the blob.

Comment 17

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

Comment on attachment 8607703 [details] [diff] [review]
Patch v1.1

Review of attachment 8607703 [details] [diff] [review]:
-----------------------------------------------------------------

in comment 10 I suggested to rather use a .filter and filter out too long tags, this way we preserve most information we can. You can use Ci.nsITaggingService.MAX_TAG_LENGTH for that.
Changing the test should be simple, define 2 tags for guid2, tag2 and the long tag, and in the for loop check tags = tag+i for all bookmarks (the long tag should not be imported, the short one should be)

::: toolkit/components/places/tests/bookmarks/test_1129529.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

You can clearly use MPL2 if you wish, but tests don't need anymore a license header, if you don't provide any header the default license will be Public Domain.

@@ +4,5 @@
> +
> +
> +function run_test() {
> +  run_next_test();
> +}

This is no more needed, if you use add_task in xpcshell and you don't provide a run_test, the tests harness will make one for you.

@@ +14,5 @@
> +    guid: "root________",
> +    index: 0,
> +    id: 1,
> +    type: "text/x-moz-place-container",
> +    dateAdded: 1000,

using a real value would be nicer :) you can
let now = Date.now() * 1000; and then use now++ all around

@@ +61,5 @@
> +          uri: "http://test2.com/"
> +        }
> +      ]
> +    }]
> +  }

nit: add a newline before this line, for readability

@@ +65,5 @@
> +  }
> +  let contentType = "application/json";
> +  let uri = "data:" + contentType + "," + JSON.stringify(aData);
> +  yield BookmarkJSONUtils.importFromURL(uri, false);
> +  let [bookmarks, count] = yield PlacesBackups.getBookmarksTree();

nit: add a newline before this line, for readability

@@ +67,5 @@
> +  let uri = "data:" + contentType + "," + JSON.stringify(aData);
> +  yield BookmarkJSONUtils.importFromURL(uri, false);
> +  let [bookmarks, count] = yield PlacesBackups.getBookmarksTree();
> +  let unsortedBookmarks = bookmarks.children[2].children;
> +  equal(unsortedBookmarks.length, 3);

In Places test code we decided to keep the Assert. prefix to clarify where the method comes from, so Assert.equal
please fix all instances

@@ +69,5 @@
> +  let [bookmarks, count] = yield PlacesBackups.getBookmarksTree();
> +  let unsortedBookmarks = bookmarks.children[2].children;
> +  equal(unsortedBookmarks.length, 3);
> +  for (let i = 0; i < unsortedBookmarks.length; ++i) {
> +    let bookmark = unsortedBookmarks[i];

for (let bookmark of unsortedBookmarks) {

@@ +77,5 @@
> +    equal(bookmark.uri, "http://test" + i + ".com/");
> +    if (i != 1)
> +      equal(bookmark.tags, "tag" + i);
> +    else
> +      ok(!Object.prototype.hasOwnProperty.call(bookmark, "tag"));

why not just bookmark.hasOwnProperty("tag")?

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2

Addressed review comments.

Comment 19

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

Comment on attachment 8608323 [details] [diff] [review]
Patch v2

Review of attachment 8608323 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thank you

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +467,5 @@
> +          if (tags.length) {
> +            try {
> +              PlacesUtils.tagging.tagURI(NetUtil.newURI(aData.uri), tags);
> +            } catch (ex) {
> +              // Invalid tag child, skip it.

please also Cu.reportError(`Unable to set tags "${tags.join(", ")}" for ${aData.uri}: ${ex}`);

Comment 20

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2.1

Added the Cu.reportError. Carrying forward the r+.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a8087a77b8ca

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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