Closed Bug 791447 Opened 8 years ago Closed 7 years ago

Nightly 2012-09-14 resets all changes made to the new tab page

Categories

(Firefox :: General, defect, blocker)

defect
Not set
blocker

Tracking

()

VERIFIED FIXED
Firefox 20
Tracking Status
firefox17 + unaffected
firefox18 + verified
firefox19 + verified

People

(Reporter: ttaubert, Assigned: mak)

References

Details

(4 keywords)

Attachments

(1 file, 5 obsolete files)

The changeset must be in here:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f36b93c70d05&tochange=e5af3d785252

This might even reset everything contained in the localStorage and be not specific to the new tab page (that sounds more plausible).
Looks like this is caused by bug 776416, as stated in the bug.
Blocks: 776416
Assignee: nobody → Jan.Varga
https://tbpl.mozilla.org/?tree=Try&rev=2c1fcaff6c0f

This patch moves the two pieces of information currently stored in localStorage into preferences. We're already storing two pieces of data in prefs, so it seems to make sense to store the two remaining pieces there too.
That seems to be duplicating the work going on in bug 790178.
Depends on: 790178
Attached patch Patch to fix (obsolete) — Splinter Review
Silly spelling mistake in the previous patch.

Gavin: This patch makes us use prefs rather than localStorage to store pinned and blocked pages. I'd like to land this on aurora so that we can migrate people from localStorage to prefs before the localStorage data goes away.

This solution *does* mean that if someone upgrades from Firefox 16 directly to Firefox 18, they'll lose their blocked and pinned pages. However it makes the localStorage code dramatically simpler since we don't have to forever keep code around to migrate data between various sqlite databases.
Assignee: Jan.Varga → jonas
Attachment #669029 - Flags: review?(gavin.sharp)
Comment on attachment 669029 [details] [diff] [review]
Patch to fix

Sorry I haven't gotten to this - maybe Marco can take a look, since he's been investigating similar issues with about:home.
Attachment #669029 - Flags: review?(gavin.sharp) → review?(mak77)
For a lucky conincidence about:home has been fixed by using attributes in the same version, bug 776416 would have broken it as well.

FWIW test_domstorage_aboutpages.js existed exactly to ensure we didn't remove about storage with cookies, the test has just been ignored and removed, failing tests should be investigated, not removed.  I honestly think that bug should have been backed out as soon as this issue was found. That said, looking at the patch now.

Was a bug filed to remove the orphan chromeappsstore.sqlite from the profile folders or was that already taken care in bug 776416? If not please file a bug to do that.
(In reply to Marco Bonardo [:mak] from comment #7)
> Was a bug filed to remove the orphan chromeappsstore.sqlite from the profile
> folders or was that already taken care in bug 776416? If not please file a
> bug to do that.

this could probably be done in nsBrowserGlue.js migrationUI
also http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#559 should be fixed to remove chromeappstore.sqlite from the list
There could be another issue with bug 776416, IIRC chromeappsstore was also skipping PB checks, so it's possible storage doesn't work anymore in PB mode now. Didn't test this though, reporting just as a note.
Comment on attachment 669029 [details] [diff] [review]
Patch to fix

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

So, there's a big problem with the preferences approach: it will be fine with pinnedLinks (at a maximum 9 pages) but blockedLinks can be any size, hundreds or even thousands (well the latter is unlikely, but it's theoretically possible). I don't have smart ideas atm, was thinking maybe to store a gz stream, or instead of storing full urls use some kind of hashes and compare hashes in the blocklist code.
moving to indexedDB may be the right thing, but any code using Storage should be async-ready, and that may take far more time than a simple prefs approach.

An alternative idea could be to liste to sanitize events, before it happens cache current data, then write it back after the sanitize.

::: browser/modules/NewTabUtils.jsm
@@ +76,5 @@
>        value = JSON.parse(this.domStorage.getItem(aKey));
>      } catch (e) {}
>  
>      return value || aDefault;
>    },

So, this patch is removing the nice Storage object abstraction, or better it's keeping it as a mock to migrate old data.
I'd rather make it still be an abstraction wrapping whatever storage strategy we want to take, preferences, indexedDB or anything else. Would be nice to keep this single point of IO.

The migration should instead be done elsewhere as a one-shot migration... we could introduce in this Storage object a versioning (version stored in a pref) and consider an undefined version as the DOMStorage ones, go through a migrate method, then bump up the version. this would allow easier migration to indexedDB or whatever in future.

in the prefs Storage a Storage.get("pinnedLinks") would just query browser.newtabpage.pinnedLinks pref.. so basically the code stays the same.

clear would just clear the prefs.
Attachment #669029 - Flags: review?(mak77)
(In reply to Jonas Sicking (:sicking) from comment #4)
> This solution *does* mean that if someone upgrades from Firefox 16 directly
> to Firefox 18, they'll lose their blocked and pinned pages.

if we don't remove chromeappsstore.sqlite, how may the data go away? That would just happen if the user clears cookies right? if this patch manages migration we should be fine.
well, provided we uplift it to Aurora.
Severity: normal → blocker
Keywords: dataloss
I'm a little confused by all the comments above.

First off, I think this bug should only cover making the about:newtab page work since that is currently causing dataloss and will need to be uplifted to aurora.

Deleting the old chromeappsstore.sqlite file and making the telemetry code should certainly be done, but in a separate bug. And it can be done only on mozilla-central.

Does this sound ok?

(In reply to Marco Bonardo [:mak] from comment #12)
> (In reply to Jonas Sicking (:sicking) from comment #4)
> > This solution *does* mean that if someone upgrades from Firefox 16 directly
> > to Firefox 18, they'll lose their blocked and pinned pages.
> 
> if we don't remove chromeappsstore.sqlite, how may the data go away? That
> would just happen if the user clears cookies right? if this patch manages
> migration we should be fine.

The reason the user experiences dataloss even if we haven't removed the sqlite file is that the localStorage code was simplified and now reads from webappsstore.sqlite for pages from all URLs. This because we removed all the special codepaths for chrome which was causing various complexity in the code.

So the data is there, but we're not reading it.

So what I'd like to do is to write a patch that's simple enough that we can land it for Firefox 17. This means that the patch has to be simple and safe enough that we can take it on the mozilla-beta branch.

The current patch was the safest solution I could think of. Ideally I would have liked to write something which migrated the data directly from localStorage to indexedDB, but that was a lot more complicated since indexedDB is asynchronous.

I agree that in theory storing this in prefs can be a problem if the user blacklists hundreds or thousands of websites. But it also seems to me that this is something that would be relatively unlikely.

A user would have to have hundreds or thousands of websites get into the top 9 of most visited sites, but yet the user didn't want to list those sites in the newtab page. But I agree that it could happen.

Do you think it would be ok to take use the current approach for now, but then work on code to migrate the data from prefs to indexedDB where we can store basically unlimited amounts of data?
And I am of course happy to make any changes that are needed in the actual patch, even if we do keep the current approach of using prefs.
(In reply to Jonas Sicking (:sicking) from comment #14)
> So what I'd like to do is to write a patch that's simple enough that we can
> land it for Firefox 17. This means that the patch has to be simple and safe
> enough that we can take it on the mozilla-beta branch.
[...]
> Do you think it would be ok to take use the current approach for now, but
> then work on code to migrate the data from prefs to indexedDB where we can
> store basically unlimited amounts of data?

Can we just back out bug 776416 and coded that landed on top of it (bug 781866?) from aurora?
That would break localStorage in B2G. The data jars that B2G depends on were implemented on top of the simplifications that caused this bug.
(In reply to Jonas Sicking (:sicking) from comment #14)
> Deleting the old chromeappsstore.sqlite file and making the telemetry code
> should certainly be done, but in a separate bug. And it can be done only on
> mozilla-central.

yes, I suggested filing bugs apart.

> (In reply to Marco Bonardo [:mak] from comment #12)
> I agree that in theory storing this in prefs can be a problem if the user
> blacklists hundreds or thousands of websites. But it also seems to me that
> this is something that would be relatively unlikely.

Well, there's another problem as well, when we store urls, those are basically a privacy hit since they are never removed, so suppose a user removes a url from the page cause it's offending, we store its url forever...
I think my suggestion to go with a list of hashes is probably the right one.

> Do you think it would be ok to take use the current approach for now, but
> then work on code to migrate the data from prefs to indexedDB where we can
> store basically unlimited amounts of data?

I'd take a patch that keeps the Storage abstraction object, adds versioning to it (with migration of old data) and uses hashes of urls to blocklist.
I honestly don't care if we overwrite the current webappstore data with the old chromeappasstore data, nightly testers accept that there may be bugs in the nightlies.
This really needs to be fixed in FF17 in order to not break things in FF18.
We're already past the second-to-last beta :(
In the worst case, we may add hacky code in Firefox 18 that does direct querying from chromeappsstore.sqlite.  I think at this point in the Firefox 17 beta we only accept really safe patches and backouts...
(In reply to Marco Bonardo [:mak] from comment #21)
> In the worst case, we may add hacky code in Firefox 18 that does direct
> querying from chromeappsstore.sqlite.  I think at this point in the Firefox
> 17 beta we only accept really safe patches and backouts...

That's correct - we'd be happy to take an uplift nomination for Aurora still but we're past the point of risky patches to 17.  If there's a possible safe backout we can do to 17 please bring it up, otherwise we'll have to leave this unfixed in 17.
Jonas, what's the status here, we are basically at the beginning of a new beta cycle without a patch, we need to fix this along this week imo.
If you wish I may steal this bug and give it some priority, I will need a reviewer though, but I'm confident we can find one.

Just let me know so we proceed.
Stealing, this should be done asap, so will try to have a patch up soon.
Assignee: jonas → mak77
Status: NEW → ASSIGNED
Attached patch wip (obsolete) — Splinter Review
This is basically the fix I have in mind, I didn't test it and I still have to write an automated test for the migration, but I'm attaching it nonetheless since someone may be interested in taking an early look.
I basically hooked into the "API"s we already have available, just doing some internal conversion.
Attachment #669029 - Attachment is obsolete: true
Attached patch patch v1.0 (obsolete) — Splinter Review
This passes manual and automated tests.
I need a reviewer, not sure who may have cycles for this. Will try to ask in #fx-team, but feel free to offer here.

highlights:

1. I use MD5 hashes for the blocklist, for privacy reasons. I may use shorter hashes, but we lack a native implementation of them, so it would be uselessly slow.

2. I use complex prefs cause titles may contain unicode. I evaluated using unicode for hashes too, to save on chars, but the \u00XX encodings, in the end, deplete all of the advantages, so I stick with base64.

3. Upgrading to 18, downgrading to 17 and upgrading again to 18 is unable to bring back and forth data. The first upgrade will migrate to the new storage, the downgrade will use old storage, the new upgrade won't do anything, will just start from where it was at the first step. This breaks my migration rules (see 5.) but I have not yet found a solution, since I can't change 17. The only solution would be to always run this, and I don't want to do that, for perf reasons. As a workaround, the user may reset the version number in prefs to force a migration.

4. I can't make an automated test for migration, since I cannot ensure that a previous b-c test won't use about:newTab, migration runs just once per profile and the Storage object is (properly) hidden in the module scope and inaccessible. Thus, I manually tested it.

5. The migration involves a couple rules: upgrades must be backwards compatible (downgrading should not break functionality), any upgrade after a downgrade runs again the migration steps. This latter rule has an exception for the first migration, see 3.

6. This is, in practice, simpler than it looks from the 5 above points, the code is basically changing just behind the existing APIs and changes are well contained.
Attachment #684429 - Attachment is obsolete: true
Comment on attachment 684716 [details] [diff] [review]
patch v1.0

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

::: browser/base/content/test/newtab/head.js
@@ +182,5 @@
>      });
>    }
>  
> +  Services.prefs.setCharPref("browser.newtabpage.pinned",
> +                             JSON.stringify(links));

self review comment, this should be a complex pref, fixed locally.
(In reply to Marco Bonardo [:mak] from comment #26)
> 1. I use MD5 hashes for the blocklist, for privacy reasons. I may use
> shorter hashes, but we lack a native implementation of them, so it would be
> uselessly slow.

Hrm, I'm not sure I see the point in this. What's the privacy concern? Is it just that using about:config is an easier way for a local user to see the data than reading from the localStorage DB? I guess the hashing should be quick, but I'm not sure it's worth complicating the code this way just for that...
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #28)
> Hrm, I'm not sure I see the point in this. What's the privacy concern? Is it
> just that using about:config is an easier way for a local user to see the
> data than reading from the localStorage DB? I guess the hashing should be
> quick, but I'm not sure it's worth complicating the code this way just for
> that...

Yes, the first reasons is that prefs are much easier to access than localstorage, we will likely have to fix this shortly after if we'd not take care of that now. For example We have already various bugs about localstore.rdf containing domains removed from history, that is not exactly an easy to access file. This is something we expose in the UI, even if not primary or secondary.

But mostly, pinned links can be a lot and urls may be quite long, so there's concrete risk the pref grows without control. Hashes are in average quite shorter than urls (fixed length) so the pref can contain more data and be faster to access.

Btw, the code complication for hashing is minimal, just where we were using urls, now we have toHash(url), it's 3 lines of code thanks to the nice abstraction that was already in place.
sorry, where I wrote pinned links I meant blocked links, pinned links are not an issue, either for privacy or length (max 9). The problem are blocked links.
Attached patch patch v1.1 (obsolete) — Splinter Review
Minor cleanup and fix my previous comment.  This is ready for review.
Attachment #684716 - Attachment is obsolete: true
Gavin, any idea who may have cycles to do this review in the team?
Flags: needinfo?(gavin.sharp)
I was going to suggest Drew, but I just remembered he's on PTO for a couple of weeks.

I'll put it in my queue for now, I guess, I may be able to get to it Thursday.
Flags: needinfo?(gavin.sharp)
Attachment #685152 - Flags: review?(gavin.sharp)
Blocks: 790178
No longer depends on: 790178
Comment on attachment 685152 [details] [diff] [review]
patch v1.1

>diff --git a/browser/base/content/test/newtab/head.js b/browser/base/content/test/newtab/head.js

Hmm, do we not have any tests for the blocked links functionality? :/

Testing the storage/migration aspects of NewTabUtils.jsm via xpcshell should be possible, right?
Attachment #685152 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #34)
> Comment on attachment 685152 [details] [diff] [review]
> patch v1.1
> 
> >diff --git a/browser/base/content/test/newtab/head.js b/browser/base/content/test/newtab/head.js
> 
> Hmm, do we not have any tests for the blocked links functionality? :/

browser_newtab_block.js and browser_newtab_bug735987.js seem to leverage the blockedLinks code. I can do a double check here.

> Testing the storage/migration aspects of NewTabUtils.jsm via xpcshell should
> be possible, right?

Ah right, I somehow removed the fact this is a module... Will try to cook up something.
Oh one other thing I forgot to note: landing this will mean users on beta/aurora/nightly will lose any customizations made since bug 776416 landed, right?

Not a huge deal, but maybe worth noting in release notes for beta?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #36)
> Not a huge deal, but maybe worth noting in release notes for beta?

yes, unfortunately they will go back to where they were before we wiped their data the first time. A double dataloss isn't nice, but it may happen in unstable builds :(

A release note may be useful even if I'm not sure what the user could do to workaround that.
(In reply to Marco Bonardo [:mak] from comment #35)
> > Hmm, do we not have any tests for the blocked links functionality? :/
> 
> browser_newtab_block.js and browser_newtab_bug735987.js seem to leverage the
> blockedLinks code. I can do a double check here.

verified these tests are good to cover the change.
Attached patch test v1.0 (obsolete) — Splinter Review
Here is the test for the migration path.
As stated in previous comments, pinned and blocked links functionality is already leveraged by existing tests.
Attachment #687834 - Flags: review?(gavin.sharp)
Comment on attachment 687834 [details] [diff] [review]
test v1.0

>diff --git a/browser/modules/test/unit/test_newtab-migrate-v1.js b/browser/modules/test/unit/test_newtab-migrate-v1.js

>+function promiseLoadChromeAppsStore(aDBFile) {

Funny that we use the sync API for the code, and async API for the test :)

>+    do_check_true(pinnedLinks.length > 0);
>+    do_check_true(pinnedLinks.every(
>+      function(aLink) NewTabUtils.pinnedLinks.isPinned(aLink)
>+    ));
>+
>+    do_check_true(blockedLinks.length > 0);
>+    do_check_true(blockedLinks.every(
>+      function(aLink) NewTabUtils.blockedLinks.isBlocked(aLink)
>+    ));

You could compare blockedLinks/pinnedLinks with NewTabUtils.blockedLinks.links/NewTabUtils.pinnedLinks.links in addition to only checking isBlocked/isPinned for a more comprehensive test, right?

>diff --git a/browser/modules/test/unit/xpcshell.ini b/browser/modules/test/unit/xpcshell.ini

>+[test_newtab-migrate-v1.js]
>+skip-if = os == "android"

Is this really needed? I wouldn't think android would try to run browser/ tests at all...
Attachment #687834 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #40)
> >+function promiseLoadChromeAppsStore(aDBFile) {
> 
> Funny that we use the sync API for the code, and async API for the test :)

heh, the backend is synchronous.

> You could compare blockedLinks/pinnedLinks with
> NewTabUtils.blockedLinks.links/NewTabUtils.pinnedLinks.links in addition to
> only checking isBlocked/isPinned for a more comprehensive test, right?

For pinnedLinks I could, blockedLinks are different though. Actually I don't think it's a good idea doing a direct comparison since this is migration code testing, so what we get in a newer version may not be the same as in the old version, for example blockedLinks.links differs due to hashing.
I also don't think makes any sense to have BlockedLinks.links exposed, since it doesn't expose an array of links like pinnedLinks, it exposes an object that has urls as keys... I should probably file a bug to get rid of it.
Attached patch coalesced patchSplinter Review
Addressed comments, and coalesced patches, ready to push.
Attachment #685152 - Attachment is obsolete: true
Attachment #687834 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/be05a00edc12
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Marco: I really appreciate you cleaning up my regression here!!
Comment on attachment 687871 [details] [diff] [review]
coalesced patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 776416
User impact if declined: All of our users will lose about:newtab data (pinned and blocked links) on upgrade to Firefox 18.
Testing completed (on m-c, etc.): just did the automatic upgrade of Nightly, everything looks fine.
Risk to taking this patch (and alternatives if risky): tried to keep it as slick as possible, there is, as for any patch, possibility of minor bugs, but automated tests cover great part of the newtab page functionality. Luckily existing abstraction allowed to just change the underlying storage without big deal.
String or UUID changes made by this patch: none
Attachment #687871 - Flags: approval-mozilla-beta?
Attachment #687871 - Flags: approval-mozilla-aurora?
Note for drivers: we cannot backout bug 776416 since it's needed for Firefox OS basecamp.
Comment on attachment 687871 [details] [diff] [review]
coalesced patch

Approving for Aurora/Beta since this is the lowest risk change that we expect to prevent about:newtab data loss. If we see regressions around this change, we can consider backing this and bug 776416 out after 12/10, when we branch B2G.
Attachment #687871 - Flags: approval-mozilla-beta?
Attachment #687871 - Flags: approval-mozilla-beta+
Attachment #687871 - Flags: approval-mozilla-aurora?
Attachment #687871 - Flags: approval-mozilla-aurora+
Please land as soon as possible today to make Firefox 18 beta 3.
I think firefox 17 status is better expressed as "unaffected" the problem is in firefox 18, what we didn't fix was changing the store in 17, but there was no bug to "fix" in 17.
Blocks: 739820
Keywords: verifyme
I wasn't able to reproduce this issue on the 2012-09-14 Nightly.

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:18.0) Gecko/18.0 Firefox/18.0
Build ID: 20120914030538.

The changes made to the new tab page aren't affected and the pinned pages appear where they supposed to.

I'm getting the same behaviour with Firefox 18 beta 2, too.
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20121128060531

Marco, could you please tell me if there is another way to verify this bug?
(In reply to Manuela Muntean from comment #52)
> Marco, could you please tell me if there is another way to verify this bug?

The only way I may think of is to use a nightly older than 2012-09-12, set some pinned and blocked pages in newtab page, then install a nightly before 2012-12-02, verify the newtab pages have been lost, then install today's nightly and verify the newtab pages are back...

using beta should be possible to do something similar by installing 17beta, set some pages in newtab, then install 18beta1 and verify pages are lost, then install current beta (3? btw, a beta containing this fix) and verify the pages are back.
where I said 17beta, even 17 release would be fine...
(In reply to Marco Bonardo [:mak] from comment #53)
> (In reply to Manuela Muntean from comment #52)
> > Marco, could you please tell me if there is another way to verify this bug?
> 
> The only way I may think of is to use a nightly older than 2012-09-12, set
> some pinned and blocked pages in newtab page, then install a nightly before
> 2012-12-02, verify the newtab pages have been lost, then install today's
> nightly and verify the newtab pages are back...
> 
> using beta should be possible to do something similar by installing 17beta,
> set some pages in newtab, then install 18beta1 and verify pages are lost,
> then install current beta (3? btw, a beta containing this fix) and verify
> the pages are back.


Marco, thank you very much for your reply. I've tried your suggestions, and here are my results:

1) when trying to reproduce the issue, after using a Nightly older than 2012-09-12 I set some pinned and blocked pages in newtab page (the method I used for blocking pages was with the help of Block Site 1.0.3 add-on; I'm not sure if this is what you have in mind, when you reffer to blocked pages); then I installed a Nightly before 2012-12-02, with the User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:20.0) Gecko/20.0 Firefox/20.0, Build ID: 20121128030742. The problem is that I didn't loose the newtab pages. They were all still there. The only think that I lost were the pinned pages. They appearead in the newtab page, but they weren't pinned anymore.  Moving forward, after installing today's Nightly, the newtab pages looked exactly the same as with the Nightly from 2012-11-28.

2) I received the same results when testing with 17 release, 18 beta 1 and 18 beta 2, as you suggested in the second part of the comment 53.

Any thoughts on this?
(In reply to Manuela Muntean from comment #55)
> 1) when trying to reproduce the issue, after using a Nightly older than
> 2012-09-12 I set some pinned and blocked pages in newtab page (the method I
> used for blocking pages was with the help of Block Site 1.0.3 add-on; I'm
> not sure if this is what you have in mind, when you reffer to blocked
> pages); then I installed a Nightly before 2012-12-02, with the User Agent:
> Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:20.0) Gecko/20.0 Firefox/20.0,
> Build ID: 20121128030742. The problem is that I didn't loose the newtab
> pages. They were all still there. The only think that I lost were the pinned
> pages. They appearead in the newtab page, but they weren't pinned anymore. 

ok so, you indeed lose the pinned pages, likely you pinned the top pages so you didn't notice a "difference" in the pages themselves.

let me clarify, since I probably wasn't clear enough.
What you lose (and then gain back with current nightly) is the pinned pages and the blocked pages. What I mean by blocked pages is pages you remove from the new tab page by clicking the X button in the top right part of the page. No need for add-ons.

A good test you could do to make more evident the loss is to block (remove) the first 9 pages that are suggested to you an then pin the new pages suggested. this way the loss should be quite clear since the pages you blocked should appear again and pinned pages should not be there.

> Moving forward, after installing today's Nightly, the newtab pages looked
> exactly the same as with the Nightly from 2012-11-28.

so you got back the pinned pages? this would basically confirm the fix.
QA Contact: manuela.muntean
(In reply to Marco Bonardo [:mak] from comment #56)
> (In reply to Manuela Muntean from comment #55)
> > 1) when trying to reproduce the issue, after using a Nightly older than
> > 2012-09-12 I set some pinned and blocked pages in newtab page (the method I
> > used for blocking pages was with the help of Block Site 1.0.3 add-on; I'm
> > not sure if this is what you have in mind, when you reffer to blocked
> > pages); then I installed a Nightly before 2012-12-02, with the User Agent:
> > Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:20.0) Gecko/20.0 Firefox/20.0,
> > Build ID: 20121128030742. The problem is that I didn't loose the newtab
> > pages. They were all still there. The only think that I lost were the pinned
> > pages. They appearead in the newtab page, but they weren't pinned anymore. 
> 
> ok so, you indeed lose the pinned pages, likely you pinned the top pages so
> you didn't notice a "difference" in the pages themselves.
> 
> let me clarify, since I probably wasn't clear enough.
> What you lose (and then gain back with current nightly) is the pinned pages
> and the blocked pages. What I mean by blocked pages is pages you remove from
> the new tab page by clicking the X button in the top right part of the page.
> No need for add-ons.
> 
> A good test you could do to make more evident the loss is to block (remove)
> the first 9 pages that are suggested to you an then pin the new pages
> suggested. this way the loss should be quite clear since the pages you
> blocked should appear again and pinned pages should not be there.
> 
> > Moving forward, after installing today's Nightly, the newtab pages looked
> > exactly the same as with the Nightly from 2012-11-28.
> 
> so you got back the pinned pages? this would basically confirm the fix.

Thanks Marco for clarifying this. Based on your suggestions, I can now confirm the fix for this issue.
This issue doesn't seem to work with Firefox 19, while following the steps from comment 56, on a Windows 7 64-bit machine.

I've tried this with Firefox 18.0.1, 19 beta 1 and beta 2.
(In reply to Manuela Muntean [:Manuela] [QA] from comment #58)
> This issue doesn't seem to work with Firefox 19, while following the steps
> from comment 56, on a Windows 7 64-bit machine.
> 
> I've tried this with Firefox 18.0.1, 19 beta 1 and beta 2.

Let me clarify the way I tested this. Following comment 53 and comment 56, here is what I've done:

1) install Firefox 18.0.2, blocked first 9 pages and pinned the following ones

2) after updating to Fireox 19 beta 3, the blocked pages didn't appear in the list and the pinned one did  (and this wasn't supposed to happen)

3) after updating to Firefox 19 beta 4, I get the same results as in step 2).

So, the "problem" is that at step 2) I don't lose the pinned and blocked pages, so I can't say that this is verified on Firefox 19.

Any suggestions?
The problem happens when updating from 17 to 18, not on later versions, as stated in comment 53.
(In reply to Marco Bonardo [:mak] from comment #60)
> The problem happens when updating from 17 to 18, not on later versions, as
> stated in comment 53.

In this case, how can I verify it on 19?
(In reply to Manuela Muntean [:Manuela] [QA] from comment #61)
> (In reply to Marco Bonardo [:mak] from comment #60)
> > The problem happens when updating from 17 to 18, not on later versions, as
> > stated in comment 53.
> 
> In this case, how can I verify it on 19?

no way, why do you want to do that?
The only thing you can do in 19 is to verify the existance of these prefs:
browser.newtabpage.pinned
browser.newtabpage.blocked
and verify their contents get updated when you add more pinned or blocked pages.
(In reply to Marco Bonardo [:mak] from comment #62)
> (In reply to Manuela Muntean [:Manuela] [QA] from comment #61)
> > (In reply to Marco Bonardo [:mak] from comment #60)
> > > The problem happens when updating from 17 to 18, not on later versions, as
> > > stated in comment 53.
> > 
> > In this case, how can I verify it on 19?
> 
> no way, why do you want to do that?
> The only thing you can do in 19 is to verify the existance of these prefs:
> browser.newtabpage.pinned
> browser.newtabpage.blocked
> and verify their contents get updated when you add more pinned or blocked
> pages.

Based on this comment 62, I confirm this issue is verified on Firefox 19 beta 4.

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130130080006
Status: RESOLVED → VERIFIED
Also verified on Mac OSX 10.7.5 with Firefox 19 beta 4.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130130080006
You need to log in before you can comment on or make changes to this bug.