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

VERIFIED FIXED in Firefox 44

Status

()

Firefox
Migration
P1
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: avaida, Assigned: Gijs)

Tracking

Trunk
Firefox 44
All
Windows
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox40 affected, firefox41 affected, firefox42 affected, firefox43 affected, firefox44 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
Flags: firefox-backlog+
Priority: -- → P1
(Assignee)

Comment 1

3 years ago
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)
Flags: needinfo?(andrei.vaida)
(Assignee)

Comment 2

3 years ago
(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...
(Assignee)

Comment 3

3 years ago
Created attachment 8664515 [details]
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
Attachment #8664515 - Flags: review?(MattN+bmo)
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?
Attachment #8664515 - Flags: review?(MattN+bmo)
(Assignee)

Comment 5

3 years ago
(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?
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 6

3 years ago
(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.
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]
status-firefox44: --- → affected
Flags: needinfo?(andrei.vaida)
QA Contact: cornel.ionce
(Assignee)

Comment 8

3 years ago
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?
Flags: needinfo?(cornel.ionce)
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.
Attachment #8664515 - Flags: review+
Flags: needinfo?(MattN+bmo)
Unfortunately, the builds from comment 8 are still affected.
Flags: needinfo?(cornel.ionce)
(Assignee)

Comment 12

3 years ago
(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)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(cornel.ionce)
Keywords: leave-open
(Assignee)

Updated

3 years ago
Attachment #8664515 - Flags: checkin+
I've created the zip file and e-mailed it to you.
Please let me know if I can provide further assistance here.
Flags: needinfo?(cornel.ionce)
(Assignee)

Comment 15

3 years ago
(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.
(Assignee)

Comment 16

3 years ago
(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).
(Assignee)

Comment 17

3 years ago
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
Attachment #8664515 - Attachment description: MozReview Request: Bug 1194692 - audit and fix usage of fileTimeToSecondsSinceEpoch, r?MattN → MozReview Request: Bug 1194692 - fix some cookie parsing issues in the IE/Edge cookie importer code, r?MattN
(Assignee)

Comment 18

3 years ago
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. :-(
Attachment #8664515 - Flags: review?(MattN+bmo)
Attachment #8664515 - Flags: review+
Attachment #8664515 - Flags: checkin+
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~~")`
Attachment #8664515 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 20

3 years ago
(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:
Flags: needinfo?(MattN+bmo)
(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.
Flags: needinfo?(MattN+bmo)
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
status-firefox44: affected → fixed
Target Milestone: --- → Firefox 44
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.
Status: RESOLVED → VERIFIED
status-firefox44: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.