Not all bookmarks are saved in the backup JSON or HTML

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: morpheus3k+bugzilla, Assigned: mak)

Tracking

({dataloss, regression})

Trunk
Firefox 32
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox29 unaffected, firefox30+ verified, firefox31+ verified, firefox32+ verified)

Details

(Whiteboard: [diamond][good-next-bug][mentor=mak][lang=js] p=3 s=it-32c-31a-30b.2 [qa!])

Attachments

(1 attachment)

Reporter

Description

5 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140406030203

Steps to reproduce:

My bookmark list has grown over the years. Because I wanted to switch to the new FxA based Sync, I've started with a fresh profile and tried to import my bookmarks.

1. Open library
2. click "import and backup your bookmarks"
3. Either choose "backup" or "Export Bookmarks to HTML"
4. Save the JSON or HTML File
5. Start new profile
6. Open library
7. Either click "Restore" or "Import Bookmarks from HTML"


Actual results:

Most of the import bookmarks are missing


Expected results:

All of my bookmarks should have been imported (restored).
Reporter

Updated

5 years ago
Blocks: 992905
Component: Untriaged → Bookmarks & History
(In reply to Morpheus3k from comment #0)
> Actual results:
> Most of the import bookmarks are missing
Couldn't reproduce, tried with 10 bookmarks, 31.0a1 (2014-04-11), Win 7 x64
Reporter

Comment 2

5 years ago
This problem does not exist with a handful bookmarks. My places db has grown over the years. Because I know that old profile creates problems, I did clean my profile now and then. To clean my profile, I did export my bookmarks, created a new profile and imported again. This worked for many years.
Now it is broken.

While the Sync 1.1 works as expected, but neither new export (via JSON or HTML) nor the new Sync 1.5 (which utilize JSON as far I've seen) works. I assume that those two bugs are directly related and could affect a lot of people with old profiles (or places.sqlite files). 

I won't upload my places db to the public, but I can share with one or two QA guys and / or developer if that helps. But I can't debug that error.
Assignee

Comment 3

5 years ago
We recently fixed some bugs related to json/html, so it would be nice if you could try with the current Nightly (last fix was just last week)... fwiw, you should not use html for what you are trying to do, you should rather Backup (will be to a json file) and then Restore. HTML is only useful when sharing with an external service or browser only accepting that format (regardless it should work).
Reporter

Comment 4

5 years ago
I tested the Backup / Restore with the current Nightly, but it still has the same problem. Many bookmarks are missing after restore.
Reporter

Comment 5

5 years ago
I did further test the backup / restore function. Firefox 28 (release) works as expected. Nightly 31.0a1 does not.

What I did:
1. Create new profile with Firefox 28.0
2. Login with Sync 1.1
3. waited until sync was finished
4. Backup the bookmarks (as JSON file)
5. Create new profile with Firefox 31.0a1
6. Restore bookmarks.json (from Firefox 28.0)

This works.

7. Upgrade the profile (Firefox 28.0) to Nightly (31.0a1)
8. Backup the bookarks (as JSON file)
9. Create new profile with Firefox 31.0a1
10. Restore bookmarks.json (from Firefox 31.0a1)

This does not work.
Assignee

Comment 6

5 years ago
if you'd be fine to share your places.sqlite file with me through mail, I may take a look at why some bookmarks don't appear in the json backup. please also point me at which bookmarks are not appearing, in the mail.
Morpheus, did you email to Marco? We'd love help in confirming this or otherwise moving towards resolving it :)
Flags: needinfo?(morpheus3k+bugzilla)
Reporter

Comment 8

5 years ago
Yep, I sent Marco my places DB. 

Marco, were you able to reproduce this bug?
Flags: needinfo?(morpheus3k+bugzilla)
Assignee

Comment 9

5 years ago
didn't have the time yet, but I got the mail.
Reporter

Comment 10

5 years ago
(In reply to Liz Henry :lizzard from comment #7)
> Morpheus, did you email to Marco? We'd love help in confirming this or
> otherwise moving towards resolving it :)

Should I send you my places DB?
Flags: needinfo?(lhenry)
Assignee

Comment 11

5 years ago
please just wait patiently, sorry but this period is quite busy.
Flags: needinfo?(lhenry)
Assignee

Comment 12

5 years ago
This is confirmed and I found the cause, in getBookmarksTree we set the "parent" property to be not enumerable (we should add a comment regarding that there), later, if we found a child before a parent, we copy the properties across objects, unfortunately we don't copy the not enumerable properties...

So this is indeed a real dataloss case and we should write a test for it.

I think we need to come with a more general solution like an array of all of the not enumerable properties, so that we copy all of them correctly (there's just one now but they may increase in future).

See
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesBackups.jsm#623
and
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesBackups.jsm#552
Blocks: 824433
No longer blocks: 992905
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: in-testsuite?
Flags: firefox-backlog?
Keywords: dataloss, regression
Assignee

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
Version: 31 Branch → Trunk
Assignee

Comment 13

5 years ago
and note that likely bug 988070 would also solve this, cause it would visit the tree recursively instead of doing a linear scan. though that's not something I'd backport, so better fixing this, adding a test, and then solve bug 988070 on top of those.
Flags: firefox-backlog? → firefox-backlog+
Morpheus and mak, wow, nice detective work!  :)
Duplicate of this bug: 992905
(In reply to Marco Bonardo [:mak] from comment #12)
> This is confirmed and I found the cause, in getBookmarksTree we set the
> "parent" property to be not enumerable (we should add a comment regarding
> that there), later, if we found a child before a parent, we copy the
> properties across objects, unfortunately we don't copy the not enumerable
> properties...

Why actually is .parent not enumerable? Can't we just make it that?
Assignee

Comment 17

5 years ago
(In reply to Tim Taubert [:ttaubert] from comment #16)
> Why actually is .parent not enumerable? Can't we just make it that?

nope, all properties that we need for internal use but that should not appear in the json are not enumerable. it's a good feature of JSON.stringify.
I see, optionally we could also pass a replacer arg to JSON.stringify() and just filter .parent keys? I feel like it's maybe better to filter when exporting to a certain format than to sacrifice our internal structure?
Assignee

Comment 19

5 years ago
(In reply to Tim Taubert [:ttaubert] from comment #18)
> I see, optionally we could also pass a replacer arg to JSON.stringify() and
> just filter .parent keys? I feel like it's maybe better to filter when
> exporting to a certain format than to sacrifice our internal structure?

it would be much more expensive to filter thousands of entries, rather than relying on stringify enumerable properties filter (which cost has to be payed regardless).
Ok, fair enough. That's a very valid point :)
Assignee

Comment 21

5 years ago
I mean, if this is the only reason, I don't think we should use a filter. On the other side it's possible Mano in rewriting getBookmarksTree will need further filtering, then if we are already paying the price, there would be no reason to not use a json filter. I think for this bug we should stick to the simple change, and we'll see how the API is going to be modified.
Flagging as [diamond] and [good-next-bug], though it seems to have a couple of important moving parts to it. Tim, would you be willing to mentor?
Flags: needinfo?(ttaubert)
Whiteboard: [diamond] [good-next-bug]

Comment 23

5 years ago
(In reply to Marco Bonardo [:mak] from comment #12)
> This is confirmed and I found the cause, in getBookmarksTree we set the
> "parent" property to be not enumerable (we should add a comment regarding
> that there)
Or add explicitly all property attributes. This thing is subtle and fairly recent (ES5) so not everyone knows the default values. The effect of not setting all properties also depends on whether the property has already been defined or not. It's clear in that case, but may not otherwise be.
Setting all properties leaves no ambiguity when it comes to the author intention.

    Object.defineProperty(bookmark, "parent", {
      value: aRow.getResultByName("parent"),
      writable: false,
      enumerable: false,
      configurable: false
    });

> later, if we found a child before a parent, we copy the
> properties across objects, unfortunately we don't copy the not enumerable
> properties...
> 
> So this is indeed a real dataloss case and we should write a test for it.
> 
> I think we need to come with a more general solution like an array of all of
> the not enumerable properties, so that we copy all of them correctly
> (there's just one now but they may increase in future).
Enumerability is a pretty much a dead concept [1] (it was introduced to give author the same sort of expressivity built-in ECMAScript and DOM features have). It doesn't provide any serious guarantee (non-enumerable properties can still be enumerated with Object.getOwnPropertyNames). 
As suggested in [1], for-in loops are super-broken and should be replaced by for-of loops (already implemented by SpiderMonkey). I think that's the good path forward here.

[1] https://mail.mozilla.org/pipermail/es-discuss/2014-January/035638.html
Assignee

Comment 24

5 years ago
yep, we can probably just change for...in to for...of and I take the suggestion to be very explicit setting enumerable: false. The good thing is JSON.stringify behavior regarding not-enumerable properties is in the spec, so it's very unlikely to change.
I can mentor this, the only hard part here is writing a test that hits this specific case, that is having a child being added to the tree before its parent, that depends on db status. I suspect it may be done by having multi-level structure (at least 2 levels) and moving a bookmark created before any folder into the deeper level, TBV.
Flags: needinfo?(ttaubert)
Whiteboard: [diamond] [good-next-bug] → [diamond][good-next-bug][mentor=mak][lang=js]
Whiteboard: [diamond][good-next-bug][mentor=mak][lang=js] → [diamond][good-next-bug][mentor=mak][lang=js] p=3
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Whiteboard: [diamond][good-next-bug][mentor=mak][lang=js] p=3 → [diamond][good-next-bug][mentor=mak][lang=js] p=3 s=it-32c-31a-30b.2 [qa?]
Whiteboard: [diamond][good-next-bug][mentor=mak][lang=js] p=3 s=it-32c-31a-30b.2 [qa?] → [diamond][good-next-bug][mentor=mak][lang=js] p=3 s=it-32c-31a-30b.2 [qa+]
Assignee

Comment 25

5 years ago
Posted patch patch v1Splinter Review
Attachment #8424327 - Flags: review?(mano)
Comment on attachment 8424327 [details] [diff] [review]
patch v1

New getBookmarksTree should land in the next few days, but as it's a dataloss issue, it should be evaluated for aurora & beta.
Attachment #8424327 - Flags: review?(mano) → review+
Assignee

Comment 27

5 years ago
(In reply to Mano from comment #26)
> Comment on attachment 8424327 [details] [diff] [review]
> patch v1
> 
> New getBookmarksTree should land in the next few days, but as it's a
> dataloss issue, it should be evaluated for aurora & beta.

I think it's safer if we land getBookmarksTree rewrite on top of this (this one will also be uplifted)
Assignee

Comment 28

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/21bf1b5eaa83
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → Firefox 32
https://hg.mozilla.org/mozilla-central/rev/21bf1b5eaa83
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Assignee

Comment 30

5 years ago
Comment on attachment 8424327 [details] [diff] [review]
patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 824433
User impact if declined: Some parts of the bookmark hierarchy (whole subtrees) are missing from the backups, possible dataloss
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk patch with automated test
String or IDL/UUID changes made by this patch: none
Attachment #8424327 - Flags: approval-mozilla-beta?
Attachment #8424327 - Flags: approval-mozilla-aurora?
Attachment #8424327 - Flags: approval-mozilla-beta?
Attachment #8424327 - Flags: approval-mozilla-beta+
Attachment #8424327 - Flags: approval-mozilla-aurora?
Attachment #8424327 - Flags: approval-mozilla-aurora+
Reporter

Comment 32

5 years ago
I can confirm that this patch fixes this bug. Export with JSON and HTML works as expected. Firefox Account based Sync (1.5) synchronizes all my bookmarks.

Thank you, Marco!
I've tried to reproduce this issue by creating a profile with more than 1000 bookmarks in several folders, exporting/backup them and restore/import them in new profiles. Firefox Account based Sync synchronizes all my bookmarks. I was unable to reproduce this on the affected versions so I cannot confirm the fix. 
This might be due to the fact that bookmarks were created in the same day and not in time.

Morpheus3k, could you please verify if this is fixed for you using:
- Firefox 30 beta 6: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/30.0b6-candidates/build2/
- Latest Nightly: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/
- Latest Aurora: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/
Flags: needinfo?(morpheus3k+bugzilla)
Assignee

Comment 34

5 years ago
(In reply to Camelia Badau, QA [:cbadau] from comment #33)
> I've tried to reproduce this issue by creating a profile with more than 1000
> bookmarks in several folders, exporting/backup them and restore/import them
> in new profiles. Firefox Account based Sync synchronizes all my bookmarks. I
> was unable to reproduce this on the affected versions so I cannot confirm
> the fix. 
> This might be due to the fact that bookmarks were created in the same day
> and not in time.

the setup to create the issue is a little bit more complicated cause the database must be in a given situation (that is more common using a profile for a long time than for a short time), you should:

1. create a bookmark somewhere, for example on the toolbar
2. create a folder on the toolbar
3. drag the bookmark inside the folder
4. create another folder on the toolbar
5. drag the previously created folder inside the last created one
6. backup to json/html

The backup/html should miss some of these bookmarks (likely all of them)

All of this is cause we need to create a situation where a bookmark is returned before its parent in our code.
QA Contact: camelia.badau
There seems to be an automated test for this already: https://hg.mozilla.org/releases/mozilla-beta/diff/3fb029a11c05/toolkit/components/places/tests/bookmarks/test_992901-backup-unsorted-hierarchy.js.

Given the more complicated setup for this, should we still try to verify this manually or is the automated testing enough?
Reporter

Comment 37

5 years ago
I've tested the following with my places.sqlite file:

1. Export and import via JSON
2. Export and import via HTML
3. Firefox Account based Sync (1.5)

on the following Channels:
- Firefox Nightly 32.0a1 (2014-05-22)
- Firefox Aurora 31.0a2 (2014-05-22)
- Firefox 30 Beta 6

and I can confirm that the issue is fixed for Nightly and Aurora.

Firefox 30 Beta 6 has not been fixed. As far I can see from the pushlog, this patch will be part of Beta 7. I can retest it, when its ready, if you want?
Flags: needinfo?(morpheus3k+bugzilla)
Assignee

Comment 38

5 years ago
I think the automated test is enough to cover the specific issue that caused this bug, I agree that manually reproducing the test doesn't sound very useful.
Morpheus3k, if you can please verified the bug also on Firefox Beta 7: http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/30.0b7-candidates/build1/
Flags: needinfo?(morpheus3k+bugzilla)
Reporter

Comment 40

5 years ago
I've retested Firefox 30 Beta 7 and export / import with JSON and HTML as well as Sync 1.5 works now.

Thanks a lot :)
Flags: needinfo?(morpheus3k+bugzilla)
Thank you, Morpheus3k!
Status: RESOLVED → VERIFIED
Whiteboard: [diamond][good-next-bug][mentor=mak][lang=js] p=3 s=it-32c-31a-30b.2 [qa+] → [diamond][good-next-bug][mentor=mak][lang=js] p=3 s=it-32c-31a-30b.2 [qa!]
You need to log in before you can comment on or make changes to this bug.