1194692 - IEProfileMigrator.js:410:0 throws an error when importing data from Internet Explorer 11

Status: VERIFIED FIXED

(Firefox :: Migration, defect, P1)

Description

[Andrei Vaida [:avaida]]

Reproducible on:
* Nightly 43.0a1 (2015-08-13)
* Aurora 42.0a2 (2015-08-13)
* 41.0b1
* 40.0.2 (20150812163655) 
* 39.0 (20150630154324)

Affected platforms:
Windows 7 (x64)

Preconditions:
* Internet Explorer 11 is installed
* IE has a custom homepage set
* IE has a reasonable amount of browsing history available
* IE has a few saved bookmarks/favorites

Steps to reproduce:
1. Launch Firefox.
2. Click "Show all bookmarks" and select "Import Settings and Data" from the top menu of the Library.
3. Follow the import process and make sure you select "Internet Explorer" from the list of browsers.
4. Check the Browser Console.

Expected result:
All the selected data is successfully imported to Firefox and no errors are thrown for this process.

Actual result:
An error is thrown in the Browser Console by IEProfileMigrator.js:410:0:
> Unable to migrate cookie: [Exception... "Component returned failure 
> code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsICookieManager2.add]"  
> nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: 
> "JS frame :: jar:file:///C:/Users/andrei.vaida/Desktop/m-a
> /firefox/browser/omni.ja!/components/IEProfileMigrator.js
> :: C__parseCookieBuffer :: line 464"  data: no]

Notes:
* This might be a regression or a side-effect from Bug 1153900.

Comment 1

[:Gijs (he/him)]

I wonder if I fixed this in bug 1205053. It will depend on what cookies you have available, so Andrei, if you can still reproduce on current nightly on some machine, can you test with the build off http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-team-win32/1442936766/ or http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-team-win64/1442936766/ ? (32 and 64-bit respectively)

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #1)
> I wonder if I fixed this in bug 1205053. It will depend on what cookies you
> have available, so Andrei, if you can still reproduce on current nightly on
> some machine, can you test with the build off
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-team-
> win32/1442936766/ or
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-team-
> win64/1442936766/ ? (32 and 64-bit respectively)

Hm, actually, that might not break as badly now but the expiry time will be wonky, that should be a division by 1000, not a multiplication...

Comment 3

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1194692 - fix some cookie parsing issues in the IE/Edge cookie importer code, r?MattN

Bug 1194692 - audit and fix usage of fileTimeToSecondsSinceEpoch, r?MattN

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8664515 [details]
MozReview Request: Bug 1194692 - fix some cookie parsing issues in the IE/Edge cookie importer code, r?MattN

https://reviewboard.mozilla.org/r/19975/#review18125

::: browser/components/migration/MSMigrationUtils.jsm:872
(Diff revision 1)
> +            // login manager wants time in milliseconds since epoch, so convert
> +            // to seconds since epoch and multiply to get milliseconds:
> +            creation = ctypesKernelHelpers.
>                           fileTimeToSecondsSinceEpoch(item.contents.highLastModified,
>                                                       item.contents.lowLastModified) * 1000;
> +          } catch (ex) {
> +            // Ignore exceptions in the dates and just create the login for right now.

Have you actually seen this fail in the wild or are you guessing?

I guess I'm a bit confused since the commit message says it's fixing usage of fileTimeToSecondsSinceEpoch but it doesn't seem to be changing any callers.

Can you explain the rationale for the change and/or update the commit message?

Comment 5

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (behind on mail) from comment #4)
> Comment on attachment 8664515 [details]
> MozReview Request: Bug 1194692 - audit and fix usage of
> fileTimeToSecondsSinceEpoch, r?MattN
> 
> https://reviewboard.mozilla.org/r/19975/#review18125
> 
> ::: browser/components/migration/MSMigrationUtils.jsm:872
> (Diff revision 1)
> > +            // login manager wants time in milliseconds since epoch, so convert
> > +            // to seconds since epoch and multiply to get milliseconds:
> > +            creation = ctypesKernelHelpers.
> >                           fileTimeToSecondsSinceEpoch(item.contents.highLastModified,
> >                                                       item.contents.lowLastModified) * 1000;
> > +          } catch (ex) {
> > +            // Ignore exceptions in the dates and just create the login for right now.
> 
> Have you actually seen this fail in the wild or are you guessing?

I haven't, no, so I am guessing a little bit, in part because of comment #0 and the suspected regressing bug. Hence also asking Andrei if this is still reproducing...

> I guess I'm a bit confused since the commit message says it's fixing usage
> of fileTimeToSecondsSinceEpoch but it doesn't seem to be changing any
> callers.
> 
> Can you explain the rationale for the change and/or update the commit
> message?

Callers need to check for exceptions, and in that sense one of the callers is being modified here. The fallback value in the case of the exception needs to be provided by the caller and be correct. I can change the commit message if you have a suggestion for something that would be clearer?

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Matthew N. [:MattN] (behind on mail) from comment #4)
> > Comment on attachment 8664515 [details]
> > MozReview Request: Bug 1194692 - audit and fix usage of
> > fileTimeToSecondsSinceEpoch, r?MattN
> > 
> > https://reviewboard.mozilla.org/r/19975/#review18125
> > 
> > ::: browser/components/migration/MSMigrationUtils.jsm:872
> > (Diff revision 1)
> > > +            // login manager wants time in milliseconds since epoch, so convert
> > > +            // to seconds since epoch and multiply to get milliseconds:
> > > +            creation = ctypesKernelHelpers.
> > >                           fileTimeToSecondsSinceEpoch(item.contents.highLastModified,
> > >                                                       item.contents.lowLastModified) * 1000;
> > > +          } catch (ex) {
> > > +            // Ignore exceptions in the dates and just create the login for right now.
> > 
> > Have you actually seen this fail in the wild or are you guessing?
> 
> I haven't, no, so I am guessing a little bit, in part because of comment #0
> and the suspected regressing bug.

Oh, and bug 1205053 comment 2.

Comment 7

[Cornel Ionce [:noni] [Hubs QA]]

I'm still seeing this on Windows 7 64-bit using latest Nightly, build ID: 20150923030230.

Also, I've verified with the build provided in comment 1 (build ID: 20150922084606) and the error is still there:
> Unable to migrate cookie: [Exception... "Component returned failure 
> code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsICookieManager2.add]" 
> nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  >location: 
> "JS frame :: resource:///modules/MSMigrationUtils.jsm ::
>_parseCookieBuffer :: line 698"  >data: no]

Comment 8

[:Gijs (he/him)]

Cornel, can you try a build off https://treeherder.mozilla.org/#/jobs?repo=try&revision=11970dcb67a1 once it's finished to see if that fixes it?

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8664515 [details]
MozReview Request: Bug 1194692 - fix some cookie parsing issues in the IE/Edge cookie importer code, r?MattN

https://reviewboard.mozilla.org/r/19975/#review18285

(In reply to :Gijs Kruitbosch from comment #6)
> Oh, and bug 1205053 comment 2.

OK, I don't think the fact that we saw a value of 0 for typed URLs means we may see one for logins but I'll leave this up to you.

Comment 10

[Cornel Ionce [:noni] [Hubs QA]]

Unfortunately, the builds from comment 8 are still affected.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8ab4bb0ba655

Comment 12

[:Gijs (he/him)]

(In reply to Cornel Ionce [QA] from comment #10)
> Unfortunately, the builds from comment 8 are still affected.

Is this a test profile on IE that has no private data? If so, can you create a zip file of the cookies directory and attach it or send it to me? Without access to the cookie files I'm not sure how to fix this, because it's not clear what's going wrong.

( the dir in question should be what you get when you evaluate

Services.dirsvc.get("CookD", Ci.nsIFile).path

in the browser console)

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8ab4bb0ba655

Comment 14

[Cornel Ionce [:noni] [Hubs QA]]

I've created the zip file and e-mailed it to you.
Please let me know if I can provide further assistance here.

Comment 15

[:Gijs (he/him)]

(In reply to Cornel Ionce [QA] from comment #14)
> I've created the zip file and e-mailed it to you.
> Please let me know if I can provide further assistance here.

So there are cookies in here with very crazy data in them. It seems like Skype (or maybe this is the IE embedding stuff, implicitly) stores data with hostname "~~local~~". And then a local path to the directory where the Skype stuff is (either in appdata or program files). We can just drop those entries.

Another issue I found through the set of cookies in that archive is that cookies with values ending in "*" get truncated and therefore break, because we think that splitting on "*\n" is a good way of distinguishing the different cookie records in a single cookie file, which turns out not to be the case, because... yeah. :-\

I have a patch coming up that fixes both of these issues.

Comment 16

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #15)
> (In reply to Cornel Ionce [QA] from comment #14)
> > I've created the zip file and e-mailed it to you.
> > Please let me know if I can provide further assistance here.
> 
> So there are cookies in here with very crazy data in them. It seems like
> Skype (or maybe this is the IE embedding stuff, implicitly) stores data with
> hostname "~~local~~". And then a local path to the directory where the Skype
> stuff is (either in appdata or program files). We can just drop those
> entries.

Just to expand a bit on this for clarity, the cookie service then tries to extract a toplevel domain for them and that throws the NS_ERROR_ILLEGAL_VALUE (which the cookie service forwards).

Comment 17

[:Gijs (he/him)]

Comment on attachment 8664515 [details]
MozReview Request: Bug 1194692 - fix some cookie parsing issues in the IE/Edge cookie importer code, r?MattN

Bug 1194692 - fix some cookie parsing issues in the IE/Edge cookie importer code, r?MattN

Comment 18

[:Gijs (he/him)]

Comment on attachment 8664515 [details]
MozReview Request: Bug 1194692 - fix some cookie parsing issues in the IE/Edge cookie importer code, r?MattN

Err, right, reviewboard... one bug, one patch series, apparently. Sorry, Matt. :-(

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8664515 [details]
MozReview Request: Bug 1194692 - fix some cookie parsing issues in the IE/Edge cookie importer code, r?MattN

https://reviewboard.mozilla.org/r/19975/#review18873

::: browser/components/migration/MSMigrationUtils.jsm:686
(Diff revision 2)
> +      // IE sometimes has cookies created by apps that use "~~local~~" as
> +      // the host, ignore those:
> +      if (hostpath.startsWith("~~local~~"))

'that use "~~local~~" as the host' doesn't perfectly align with `.startsWith("~~local~~")`

Comment 20

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (behind on mail) from comment #19)
> Comment on attachment 8664515 [details]
> MozReview Request: Bug 1194692 - fix some cookie parsing issues in the
> IE/Edge cookie importer code, r?MattN
> 
> https://reviewboard.mozilla.org/r/19975/#review18873
> 
> ::: browser/components/migration/MSMigrationUtils.jsm:686
> (Diff revision 2)
> > +      // IE sometimes has cookies created by apps that use "~~local~~" as
> > +      // the host, ignore those:
> > +      if (hostpath.startsWith("~~local~~"))
> 
> 'that use "~~local~~" as the host' doesn't perfectly align with
> `.startsWith("~~local~~")`

It does, I think? hostpath also includes the path, so it looks like ~~local~~/file/path/goes/here.

Or am I misunderstanding you? I can probably clarify the comment, what would you prefer? Would this be better:

// IE sometimes has cookies created by apps that use "~~local~~/local/file/path"
// as a hostpath, so ignore those:

Comment 21

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #20)
> (In reply to Matthew N. [:MattN] (behind on mail) from comment #19)
> > Comment on attachment 8664515 [details]
> > MozReview Request: Bug 1194692 - fix some cookie parsing issues in the
> > IE/Edge cookie importer code, r?MattN
> > 
> > https://reviewboard.mozilla.org/r/19975/#review18873
> > 
> > ::: browser/components/migration/MSMigrationUtils.jsm:686
> > (Diff revision 2)
> > > +      // IE sometimes has cookies created by apps that use "~~local~~" as
> > > +      // the host, ignore those:
> > > +      if (hostpath.startsWith("~~local~~"))
> > 
> > 'that use "~~local~~" as the host' doesn't perfectly align with
> > `.startsWith("~~local~~")`
> 
> It does, I think? hostpath also includes the path, so it looks like
> ~~local~~/file/path/goes/here.

Ah, I didn't notice the "path" part.

> Or am I misunderstanding you? I can probably clarify the comment, what would
> you prefer? Would this be better:
> 
> // IE sometimes has cookies created by apps that use
> "~~local~~/local/file/path"
> // as a hostpath, so ignore those:

That's probably better.

Comment 22

[Pulsebot]

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

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 25

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 43 beta 4 (20151116155110) under Windows 10 64-bit.

Verified fixed on Firefox 44 beta 8 (20160111185352) under Windows 10 64-bit. The cookies are successfully imported and there are no errors thrown in Browser Console.