Closed Bug 1129529 Opened 5 years ago Closed 5 years ago

Bookmark backup may not be restorable

Categories

(Firefox :: Bookmarks & History, defect)

35 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- affected
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: dfgsdfgsdfg, Assigned: nhnt11, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files, 4 obsolete files)

Attached file 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)
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.
Mentor: mak77
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][lang=js]
as bonus points, we could filter the tags array and remove the invalid tag, so that other tags are still set.
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
@ Abhilash Mhaisne - speak to Marco Bonardo [::mak] as he's the mentor of this bug
Mr. Bonardo, I'd like to work on this bug.
Flags: needinfo?(mak77)
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
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
Attachment #8571517 - Flags: review?(mak77)
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 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.
Attachment #8571517 - Flags: review?(mak77)
Assignee: nobody → toby.daigneault
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
Thanks for the feedback Marc. I agree. I'll get on this tomorrow and submit a new patch.
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).
Hi toby, any news about this bug?
Flags: needinfo?(toby.daigneault)
Assignee: nobody → nhnt11
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8607700 - Flags: review?(mak77)
Comment on attachment 8607700 [details] [diff] [review]
Patch

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

Thanks Nihanth!  Looks OK to me.
Attachment #8607700 - Flags: feedback+
Attached patch Patch v1.1 (obsolete) — Splinter Review
This no longer base64 encodes the blob.
Attachment #8607700 - Attachment is obsolete: true
Attachment #8607700 - Flags: review?(mak77)
Attachment #8607703 - Flags: review?(mak77)
Flags: needinfo?(toby.daigneault)
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")?
Attachment #8607703 - Flags: review?(mak77) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
Addressed review comments.
Attachment #8607703 - Attachment is obsolete: true
Attachment #8608323 - Flags: review?(mak77)
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}`);
Attachment #8608323 - Flags: review?(mak77) → review+
Attachment #8571517 - Attachment is obsolete: true
Attached patch Patch v2.1Splinter Review
Added the Cu.reportError. Carrying forward the r+.
Attachment #8608323 - Attachment is obsolete: true
Attachment #8608956 - Flags: review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/a8087a77b8ca
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.