992901 - Not all bookmarks are saved in the backup JSON or HTML

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Morpheus3k]

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).

Comment 1

[Paul Silaghi, QA [:pauly]]

(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

Comment 2

[Morpheus3k]

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.

Comment 3

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

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).

Comment 4

[Morpheus3k]

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

Comment 5

[Morpheus3k]

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.

Comment 6

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

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.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Morpheus, did you email to Marco? We'd love help in confirming this or otherwise moving towards resolving it :)

Comment 8

[Morpheus3k]

Yep, I sent Marco my places DB. 

Marco, were you able to reproduce this bug?

Comment 9

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

didn't have the time yet, but I got the mail.

Comment 10

[Morpheus3k]

(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?

Comment 11

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

please just wait patiently, sorry but this period is quite busy.

Comment 12

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

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

Comment 13

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

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.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Morpheus and mak, wow, nice detective work!  :)

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(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?

Comment 17

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

(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.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 19

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

(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).

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Ok, fair enough. That's a very valid point :)

Comment 21

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

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.

Comment 22

[Mike Hoye [:mhoye]]

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?

Comment 23

[David Bruant]

(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

Comment 24

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

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.

Comment 25

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

Attachment: patch v1

Comment 26

[Asaf Romano (gone)]

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.

Comment 27

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

(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)

Comment 28

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

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21bf1b5eaa83

Comment 30

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

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

Comment 31

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/14ad073ea2a2
https://hg.mozilla.org/releases/mozilla-beta/rev/3fb029a11c05

Comment 32

[Morpheus3k]

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!

Comment 33

[Camelia Badau [:cbadau], Desktop QA]

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/

Comment 34

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

(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.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/3fb029a11c05

Comment 36

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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?

Comment 37

[Morpheus3k]

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?

Comment 38

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

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.

Comment 39

[Camelia Badau [:cbadau], Desktop QA]

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/

Comment 40

[Morpheus3k]

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 :)

Comment 41

[Camelia Badau [:cbadau], Desktop QA]

Thank you, Morpheus3k!