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

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Looks like this is caused by bug 776416, as stated in the bug.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

That seems to be duplicating the work going on in bug 790178.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

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.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Try results:

https://tbpl.mozilla.org/?tree=Try&rev=74db14e4b669

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Marco Bonardo [:mak]]

(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

Comment 9

[Marco Bonardo [:mak]]

also http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#559 should be fixed to remove chromeappstore.sqlite from the list

Comment 10

[Marco Bonardo [:mak]]

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 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Marco Bonardo [:mak]]

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

Comment 13

[Marco Bonardo [:mak]]

well, provided we uplift it to Aurora.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 16

[Dão Gottwald [:dao]]

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

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That would break localStorage in B2G. The data jars that B2G depends on were implemented on top of the simplifications that caused this bug.

Comment 18

[Marco Bonardo [:mak]]

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

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This really needs to be fixed in FF17 in order to not break things in FF18.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

We're already past the second-to-last beta :(

Comment 21

[Marco Bonardo [:mak]]

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

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 23

[Marco Bonardo [:mak]]

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.

Comment 24

[Marco Bonardo [:mak]]

Stealing, this should be done asap, so will try to have a patch up soon.

Comment 25

[Marco Bonardo [:mak]]

Attachment: wip

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.

Comment 26

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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.

Comment 27

[Marco Bonardo [:mak]]

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.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

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

Comment 29

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 30

[Marco Bonardo [:mak]]

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.

Comment 31

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Minor cleanup and fix my previous comment.  This is ready for review.

Comment 32

[Marco Bonardo [:mak]]

Gavin, any idea who may have cycles to do this review in the team?

Comment 33

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 35

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 37

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 38

[Marco Bonardo [:mak]]

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

Comment 39

[Marco Bonardo [:mak]]

Attachment: test v1.0

Here is the test for the migration path.
As stated in previous comments, pinned and blocked links functionality is already leveraged by existing tests.

Comment 40

[:Gavin Sharp [email: [e-mail]]]

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

Comment 41

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 42

[Marco Bonardo [:mak]]

Attachment: coalesced patch

Addressed comments, and coalesced patches, ready to push.

Comment 43

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be05a00edc12

Comment 44

[Ryan VanderMeulen [:RyanVM]]

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

Comment 45

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marco: I really appreciate you cleaning up my regression here!!

Comment 46

[Marco Bonardo [:mak]]

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

Comment 47

[Marco Bonardo [:mak]]

Note for drivers: we cannot backout bug 776416 since it's needed for Firefox OS basecamp.

Comment 48

[Alex Keybl [:akeybl]]

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.

Comment 49

[Alex Keybl [:akeybl]]

Please land as soon as possible today to make Firefox 18 beta 3.

Comment 50

[Marco Bonardo [:mak]]

done, thanks!

https://hg.mozilla.org/releases/mozilla-aurora/rev/317c31ddf1a5
https://hg.mozilla.org/releases/mozilla-beta/rev/ff1aebd2c706

Comment 51

[Marco Bonardo [:mak]]

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.

Comment 52

[Manuela Muntean [Away]]

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?

Comment 53

[Marco Bonardo [:mak]]

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

Comment 54

[Marco Bonardo [:mak]]

where I said 17beta, even 17 release would be fine...

Comment 55

[Manuela Muntean [Away]]

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

Comment 56

[Marco Bonardo [:mak]]

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

Comment 57

[Manuela Muntean [Away]]

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

Comment 58

[Manuela Muntean [Away]]

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.

Comment 59

[Manuela Muntean [Away]]

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

Comment 60

[Marco Bonardo [:mak]]

The problem happens when updating from 17 to 18, not on later versions, as stated in comment 53.

Comment 61

[Manuela Muntean [Away]]

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

Comment 62

[Marco Bonardo [:mak]]

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

Comment 63

[Manuela Muntean [Away]]

(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

Comment 64

[Manuela Muntean [Away]]

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