Verdi [:verdi] Best to slack me Reporter
	Description • 9 years ago

Beginning in Firefox 35 (cmore can add more details about that) the migration tool stopped importing cookies from Internet Explorer.

STR:
1. Remove all Fx profiles and uninstall Firefox
2. Browse with IE, add favorites, visit websites like nytimes.com and cnn.com (anything that will set a cookie)
3. Install Firefox and choose to import data from IE
4. Once Firefox is open, notice that your history and bookmarks are present but if you check for cookies you'll only see the ones from mozilla.org, optimizely and google that we set on first run.

	Marco Bonardo [:mak]
	Comment 1 • 9 years ago

This is the cookies import code for IE

http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/IEProfileMigrator.js#332

	Chris More [:cmore]
	Comment 2 • 9 years ago

Michael and I both tested this independently with IE11 and verified that cookies are not importing. I have data to show that this has been working for years up until Firefox was released on Jan 13th and then the data completely stopped. I assume that this is impacting most versions of IE as I am getting almost no data now. I was able to see Google Analytics session data transfer from IE to Firefox when the user imported their cookies and then later hit /firstrun/.

Please let us know as soon as possible if this is a regression or if there was an intentional change.

:gavin: Since this is broken on all Firefox versions from Fx 35 and on, what version branch should this bug be in?

	Chris More [:cmore]
	Comment 3 • 9 years ago

Looks like the last code change was in late Nov 2014:

http://hg.mozilla.org/mozilla-central/filelog/2c9708e6b54d/browser/components/migration/IEProfileMigrator.js

But that probably didn't make the Fx 34 push on Dec 1st thus it could have been that code change that broke it in Fx 35.

	Chris More [:cmore]
	Comment 4 • 9 years ago

Gijs: ^^

	Marco Bonardo [:mak]
	Comment 5 • 9 years ago

(In reply to Chris More [:cmore] from comment #2)
> Please let us know as soon as possible if this is a regression or if there
> was an intentional change.

It's definitely a regression, there's nothing intentional in not migrating cookies.

(In reply to Chris More [:cmore] from comment #3)
> Looks like the last code change was in late Nov 2014:
> 
> http://hg.mozilla.org/mozilla-central/filelog/2c9708e6b54d/browser/
> components/migration/IEProfileMigrator.js
> 
> But that probably didn't make the Fx 34 push on Dec 1st thus it could have
> been that code change that broke it in Fx 35.

Doesn't look like that fix is related to cookies, I suspect some change in cookies API or js itself... But needs to be investigated

	Marco Bonardo [:mak]
	Comment 6 • 9 years ago

also, that fix made Firefox 37, not 35.

	:Gijs (he/him) Assignee
	Comment 7 • 9 years ago

As Marco said, doesn't seem to be related to my change.

Have you tested with older versions of IE on XP, Vista or 7? Seems like this might be IE11-related rather than Fx version N -related.

	Marco Bonardo [:mak]
	Comment 8 • 9 years ago

I can look at this during the next backlog iteration if its a priority (I guess it is) and try to figure out what changed.

	Chris More [:cmore]
	Comment 9 • 9 years ago

(In reply to Marco Bonardo [::mak] from comment #6)
> also, that fix made Firefox 37, not 35.

Ah yes. Very well could be an IE11 issue than something on our side, but regardless the migrator isn't working for cookies. 

I don't have an older IE machine to test, but I am sure we can find someone.

	Chris More [:cmore]
	Comment 10 • 9 years ago

(In reply to Marco Bonardo [::mak] from comment #8)
> I can look at this during the next backlog iteration if its a priority (I
> guess it is) and try to figure out what changed.

Thank you. Mark Mayo had asked me to run a small A/B test on new users and it relied on some portion of the population importing their cookies. The test failed because almost no data was coming through.

	Alice0775 White
	Comment 11 • 9 years ago

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=170ce237e4a0&tochange=fada58fb0996

Regressed by: Bug 1001090

Steps:
1. Open Library
2. Import and Backup > Import Data from Another Browser… > Next > Next > Finish


Error in Browser console:
File is not defined IEProfileMigrator.js:413:0
some cookies did not successfully migrate.
[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.setBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: jar:file:///D:/fuku/MyDownload/firefox-nightly.en-US.win32/browser/omni.ja!/components/IEProfileMigrator.js :: S__set :: line 587"  data: no] MigrationUtils.jsm:247:0
some settings did not successfully migrate.

	Alice0775 White
	Comment 12 • 9 years ago

Error in Browser console on the Bad build fada58fb0996:

[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.setBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource://app/components/IEProfileMigrator.js :: S__set :: line 587"  data: no] MigrationUtils.jsm:250
some settings did not successfully migrate.
Unable to migrate cookie: ReferenceError: can't access let declaration `path' before initialization IEProfileMigrator.js:407
some cookies did not successfully migrate.

	Chris More [:cmore]
	Comment 13 • 9 years ago

(In reply to Alice0775 White from comment #11)
> Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=170ce237e4a0&tochange=fada58fb0996
> 
> Regressed by: Bug 1001090
> 
> Steps:
> 1. Open Library
> 2. Import and Backup > Import Data from Another Browser… > Next > Next >
> Finish
> 
> 
> Error in Browser console:
> File is not defined IEProfileMigrator.js:413:0
> some cookies did not successfully migrate.
> [Exception... "Component returned failure code: 0x8000ffff
> (NS_ERROR_UNEXPECTED) [nsIPrefBranch.setBoolPref]"  nsresult: "0x8000ffff
> (NS_ERROR_UNEXPECTED)"  location: "JS frame ::
> jar:file:///D:/fuku/MyDownload/firefox-nightly.en-US.win32/browser/omni.ja!/
> components/IEProfileMigrator.js :: S__set :: line 587"  data: no]
> MigrationUtils.jsm:247:0
> some settings did not successfully migrate.

alice0775: what version of IE did you have on your machine when you got this error?

	Shu-yu Guo [:shu]
	Comment 14 • 9 years ago

Alice, could you see if this patch fixes the issue? I don't have a Windows
machine handy.

	Shu-yu Guo [:shu]
	Comment 15 • 9 years ago

(In reply to Chris More [:cmore] from comment #13)
> (In reply to Alice0775 White from comment #11)
> > Pushlog:
> > https://hg.mozilla.org/integration/mozilla-inbound/
> > pushloghtml?fromchange=170ce237e4a0&tochange=fada58fb0996
> > 
> > Regressed by: Bug 1001090
> > 
> > Steps:
> > 1. Open Library
> > 2. Import and Backup > Import Data from Another Browser… > Next > Next >
> > Finish
> > 
> > 
> > Error in Browser console:
> > File is not defined IEProfileMigrator.js:413:0
> > some cookies did not successfully migrate.
> > [Exception... "Component returned failure code: 0x8000ffff
> > (NS_ERROR_UNEXPECTED) [nsIPrefBranch.setBoolPref]"  nsresult: "0x8000ffff
> > (NS_ERROR_UNEXPECTED)"  location: "JS frame ::
> > jar:file:///D:/fuku/MyDownload/firefox-nightly.en-US.win32/browser/omni.ja!/
> > components/IEProfileMigrator.js :: S__set :: line 587"  data: no]
> > MigrationUtils.jsm:247:0
> > some settings did not successfully migrate.
> 
> alice0775: what version of IE did you have on your machine when you got this
> error?

This is a TDZ error, and shouldn't be tied to any particular version of IE.

	Chris More [:cmore]
	Comment 16 • 9 years ago

(In reply to Shu-yu Guo [:shu] from comment #15)
> 
> This is a TDZ error, and shouldn't be tied to any particular version of IE.

Thanks for the clarification.

	Chris More [:cmore]
	Comment 17 • 9 years ago

Alice: see comment 14.

	Alice0775 White
	Comment 18 • 9 years ago

I will try.

	Alice0775 White
	Comment 19 • 9 years ago

(In reply to Shu-yu Guo [:shu] from comment #14)
> Created attachment 8592052 [details] [diff] [review]
> Fix TDZ issue in IEProfileMigrator.js
> 
> Alice, could you see if this patch fixes the issue? I don't have a Windows
> machine handy.


The patch fixes the problem until cset e93c40d4344f.
However, After landing Bug 1047483, The migration broken again, even if the patch applied.

Error in Browser Console after click Done.
File is not defined IEProfileMigrator.js:413:0


2nd regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e93c40d4344f&tochange=b72ab356368f

2nd regressed by: Bug 1047483

	:Gijs (he/him) Assignee
	Comment 20 • 9 years ago

Comment on attachment 8592052 [details] [diff] [review]
Fix TDZ issue in IEProfileMigrator.js

rs=me to at least land this part.

	:Gijs (he/him) Assignee
	Comment 21 • 9 years ago

(In reply to Alice0775 White from comment #19)
> (In reply to Shu-yu Guo [:shu] from comment #14)
> > Created attachment 8592052 [details] [diff] [review]
> > Fix TDZ issue in IEProfileMigrator.js
> > 
> > Alice, could you see if this patch fixes the issue? I don't have a Windows
> > machine handy.
> 
> 
> The patch fixes the problem until cset e93c40d4344f.
> However, After landing Bug 1047483, The migration broken again, even if the
> patch applied.
> 
> Error in Browser Console after click Done.
> File is not defined IEProfileMigrator.js:413:0
> 
> 
> 2nd regression window:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=e93c40d4344f&tochange=b72ab356368f
> 
> 2nd regressed by: Bug 1047483

Seems like the File constructor used to be available in component scopes by default and now isn't anymore. AIUI Cu.importGlobalProperties(["File"]); should fix this, see e.g. specialPowersAPI.js . That said, I'm a little sad that apparently nobody audited existing component code when this change happened. I don't know how hard it is to write marionette tests for this profile migrator thing, but we should probably look into that (again).

	:Gijs (he/him) Assignee
	Comment 23 • 9 years ago

(tested, and this wfm locally with both patches applied)

	Marco Bonardo [:mak]
	Comment 24 • 9 years ago

I think we should try to write a test for cookies import at this point... 2 regressions in a row.

It should be possible by using the InternetSetCookie API through ctypes (https://msdn.microsoft.com/en-us/library/windows/desktop/aa385107%28v=vs.85%29.aspx) and then verifying we have the given cookie after the import.
I didn't try, but sounds like it might work.

	Marco Bonardo [:mak]
	Comment 25 • 9 years ago

we don't need marionette for this, xpcshell would be enough.

	:Gijs (he/him) Assignee
	Comment 26 • 9 years ago

(In reply to Marco Bonardo [::mak] from comment #24)
> I think we should try to write a test for cookies import at this point... 2
> regressions in a row.
> 
> It should be possible by using the InternetSetCookie API through ctypes
> (https://msdn.microsoft.com/en-us/library/windows/desktop/aa385107%28v=vs.
> 85%29.aspx) and then verifying we have the given cookie after the import.
> I didn't try, but sounds like it might work.

Do you expect this as part of the patch right now, or can I file a followup? Using ctypes from a test like this seems like it'd take a few days to get right also on try, and I don't know that that's necessarily worth doing for this regression which I think we would ideally like to uplift.

	Marco Bonardo [:mak]
	Comment 27 • 9 years ago

I actually added this bug to the backlog with the hope to write a test finally :)

So if we file a follow-up, we should add it to this backlog and I'd like to experiment with that. What I'd not be comfortable with is to land these with just the promise soon or later someone will write a test...

	Marco Bonardo [:mak]
	Comment 28 • 9 years ago

I filed bug 1154294 and will ask Marco to add it to the backlog.

	:Gijs (he/him) Assignee
	Comment 29 • 9 years ago

remote:   https://hg.mozilla.org/integration/fx-team/rev/94f4016b176d
remote:   https://hg.mozilla.org/integration/fx-team/rev/349ec1e6b247

(In reply to Marco Bonardo [::mak] from comment #27)
> I actually added this bug to the backlog with the hope to write a test
> finally :)
> 
> So if we file a follow-up, we should add it to this backlog and I'd like to
> experiment with that. What I'd not be comfortable with is to land these with
> just the promise soon or later someone will write a test...

Fair! Thanks for picking that up. Feel free to flag me for reviews.

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 9 years ago

> That said, I'm a little sad that apparently nobody audited existing component code when
> this change happened.

Andrea certainly changed a bunch of components, but it might have been based on test run results, not audits...

	Chris More [:cmore]
	Comment 31 • 9 years ago

If anyone wants to know how many users this bug is affecting, ping me on IRC (cmore) or shoot me an email (cmore@mozilla.com). I have a clear data signal on how often this is failing on a daily/weekly basis.

	:Gijs (he/him) Assignee
	Comment 32 • 9 years ago

(In reply to Chris More [:cmore] from comment #31)
> If anyone wants to know how many users this bug is affecting, ping me on IRC
> (cmore) or shoot me an email (cmore@mozilla.com). I have a clear data signal
> on how often this is failing on a daily/weekly basis.

Can you clarify "how" it is affecting them, though? How many people migrate serious (as opposed to dummy default) data like this? I've mostly seen people import the default IE bookmarks which have always seemed useless to me. :-\

	Alice0775 White
	Comment 33 • 9 years ago

One question: is this bug?


After migrate cookies from IE, Cockie data is empty.

Steps to reproduce:
1. Migrate cookies from IE
2. Show cookies (Option > Privacy > Use custom..) or Open chrome://browser/content/preferences/cookies.xul
3. Choose a migrated cookie

Actual Results:
Name/Content/Domain/Path/Send For/Expires indicate  <no cookie selected>

And when Choose a migrated cookie, error in Browser console: 

NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIScriptableDateFormat.FormatDateTime] cookies.js:489:0

	Chris More [:cmore]
	Comment 34 • 9 years ago

> Can you clarify "how" it is affecting them, though?

No idea how it is affecting people. We would have to do a funnelcake test with it enabled and disabled for two cohorts and then using FHR data to see if those groups act any differently. I don't know if I can even guess, but it seems like only session states and website personalizations that are stored in cookies could be affected, but not sure if that is a good or bad thing.

> How many people migrate serious (as opposed to dummy default) data like this?

"serious data" is hard to say given that the import is a mix of junk and maybe their personal customizations of IE. What percentage is serious, I am not sure. I don't think users know either.

I just know that I use the www.mozilla.org 1st party cookie often to do acquisition funnel A/B tests and I am blocked doing that for IE users now.

	:Gijs (he/him) Assignee
	Comment 35 • 9 years ago

(In reply to Alice0775 White from comment #33)
> One question: is this bug?
> 
> 
> After migrate cookies from IE, Cockie data is empty.
> 
> Steps to reproduce:
> 1. Migrate cookies from IE
> 2. Show cookies (Option > Privacy > Use custom..) or Open
> chrome://browser/content/preferences/cookies.xul
> 3. Choose a migrated cookie
> 
> Actual Results:
> Name/Content/Domain/Path/Send For/Expires indicate  <no cookie selected>
> 
> And when Choose a migrated cookie, error in Browser console: 
> 
> NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057
> (NS_ERROR_ILLEGAL_VALUE) [nsIScriptableDateFormat.FormatDateTime]
> cookies.js:489:0

Yes, that sounds like a bug to me, but it might be in the cookies dialog ? Might also have to do with your locale and/or the specific cookies (although I'm guessing you tried several different ones?)... it's hard to say without diving in further (also, this issue could definitely be related to which version of IE you're using if the cookie storage format for IE changed...). That said, I only checked that the cookies showed up in the cookies dialog and assumed that that was the end of the breakage here... which may have been naive. :-(

I'll try to investigate more tomorrow, but it should probably be a new bug unless the fixes so far turn out to be totally insufficient.

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/94f4016b176d
https://hg.mozilla.org/mozilla-central/rev/349ec1e6b247

	:Gijs (he/him) Assignee
	Comment 37 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #35)
> (In reply to Alice0775 White from comment #33)
> > One question: is this bug?
> > 
> > 
> > After migrate cookies from IE, Cockie data is empty.
> > 
> > Steps to reproduce:
> > 1. Migrate cookies from IE
> > 2. Show cookies (Option > Privacy > Use custom..) or Open
> > chrome://browser/content/preferences/cookies.xul
> > 3. Choose a migrated cookie
> > 
> > Actual Results:
> > Name/Content/Domain/Path/Send For/Expires indicate  <no cookie selected>
> > 
> > And when Choose a migrated cookie, error in Browser console: 
> > 
> > NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057
> > (NS_ERROR_ILLEGAL_VALUE) [nsIScriptableDateFormat.FormatDateTime]
> > cookies.js:489:0
> 
> Yes, that sounds like a bug to me, but it might be in the cookies dialog ?
> Might also have to do with your locale and/or the specific cookies (although
> I'm guessing you tried several different ones?)... it's hard to say without
> diving in further (also, this issue could definitely be related to which
> version of IE you're using if the cookie storage format for IE changed...).
> That said, I only checked that the cookies showed up in the cookies dialog
> and assumed that that was the end of the breakage here... which may have
> been naive. :-(
> 
> I'll try to investigate more tomorrow, but it should probably be a new bug
> unless the fixes so far turn out to be totally insufficient.

Filed bug 1154472.

	Chris More [:cmore]
	Comment 38 • 9 years ago

Just to confirm. Is this specific bug resolved and what fx train would it be on? Is this a candidate for uplifting?

	Sylvestre Ledru [:Sylvestre]
	Comment 39 • 9 years ago

Chris, Gavin used the tracking flags for 38, the patches seem low risk. So, I am pretty sure we want to uplift it.
Moreover, 38 is an ESR release.

Gijs, could you fill the uplift request? Thanks

	Marco Bonardo [:mak]
	Comment 40 • 9 years ago

I think it would be better to uplift the whole cookie-import-fix package, made by this, bug 1154472 and eventually bug 1154294. A single coalesced patch would be simpler to track.

	Sylvestre Ledru [:Sylvestre]
	Comment 41 • 9 years ago

What ever works better for you!

We should also consider having tests for this.

	:Gijs (he/him) Assignee
	Comment 42 • 9 years ago

(In reply to Marco Bonardo [::mak] from comment #40)
> I think it would be better to uplift the whole cookie-import-fix package,
> made by this, bug 1154472 and eventually bug 1154294. A single coalesced
> patch would be simpler to track.

Why easier? We can just uplift every bit as soon as it's ready, right? We're not that far from the 38 release, so if we can fix at least the import inasmuch as this bug fixes it, that'll be a step forward...

	Marco Bonardo [:mak]
	Comment 43 • 9 years ago

it's easier for the drivers to approve a single patch, it's easier for the sherrifs to push and backout it. We're almost one month from the merge, doesn't look like we need to rush anything. That said, I never intended to stop you, it was just a suggestion.

	Marco Bonardo [:mak]
	Comment 44 • 9 years ago

has a test in bug 1154294

	Sylvestre Ledru [:Sylvestre]
	Comment 45 • 9 years ago

guys, it is possible to have an uplift request? Thanks

	Marco Bonardo [:mak]
	Comment 46 • 9 years ago

Sure, let me produce a coalesced patch.

	Marco Bonardo [:mak]
	Comment 47 • 9 years ago

Bug 1153900 - Fix TDZ issue in IEProfileMigrator.js, r=gijs
* * *
Bug 1153900 - fix use of 'File' constructor in IEProfileMigrator.js, r=mak
* * *
Bug 1154472 - fix expiry dates in IE cookie imports, r=mak
* * *
Bug 1154294 - write a test for IE cookies migration. r=Gijs

Approval Request Comment
[Feature/regressing bug #]: bug 1001090, bug 1047483 plus pre-existing bug
[User impact if declined]: cookies are not imported from IE
[Describe test coverage new/current, TreeHerder]: m-c, automated test
[Risks and why]: low risk, simple changes, has test
[String/UUID change made/needed]: none

	Ryan VanderMeulen [:RyanVM]
	Comment 48 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/662ee7a4e947

	Ryan VanderMeulen [:RyanVM]
	Comment 49 • 9 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/55837b9aa111

	Chris More [:cmore]
	Comment 50 • 9 years ago

Just want to verify that this will go out with the Firefox 38.0 release on May 12th.

	Sylvestre Ledru [:Sylvestre]
	Comment 51 • 9 years ago

(In reply to Chris More [:cmore] from comment #50)
> Just want to verify that this will go out with the Firefox 38.0 release on
> May 12th.
Yes, that is the plan!

	Petruta Horea [:phorea], Desktop QA
	Comment 52 • 9 years ago

Reproduced the initial issue with Firefox 37.0.1 after deleting all existing profiles and started the browser. The option to import from other browsers was shown, but only history and bookmarks were added.

Verified under Win 8.1 64-bit that cookies are imported from IE using Firefox 38 beta 8, latest Dev Ed 39.0a2 and latest Nightly 40.0a1 2015-04-28.

Cookies are imported into all Firefox versions when choosing "Import Data from Another Browser" option from Library window.

When no profile is created, only Firefox Beta and Nightly display the option to import from other browsers at start-up. This option is not present in Developer Edition which creates "default" and "dev-edition-default" profiles when it is started. 
Should this be filled separately or is it expected behavior? Thank you

	[SV Manager] Florin Mezei, QA (:FlorinMezei)
	Comment 53 • 9 years ago

Removing qe-verify flag as verification on Firefox 38 Beta and 40 Nightly should suffice.

Gijs can you answer Petruta's question in comment 52?

	:Gijs (he/him) Assignee
	Comment 54 • 9 years ago

(In reply to Petruta Rasa [QA] [:petruta] from comment #52)
> When no profile is created, only Firefox Beta and Nightly display the option
> to import from other browsers at start-up. This option is not present in
> Developer Edition which creates "default" and "dev-edition-default" profiles
> when it is started. 
> Should this be filled separately or is it expected behavior? Thank you

File a separate bug and needinfo :bgrins, please.

	Petruta Horea [:phorea], Desktop QA
	Comment 55 • 9 years ago

Thanks, Gijs.

Filled bug 1159732 for Developer Edition. 
Marking as verified since I managed to also verify this using firefox -migration option.