Closed
Bug 1194692
Opened 9 years ago
Closed 9 years ago
IEProfileMigrator.js:410:0 throws an error when importing data from Internet Explorer 11
Categories
(Firefox :: Migration, defect, P1)
Tracking
()
VERIFIED
FIXED
Firefox 44
People
(Reporter: avaida, Assigned: Gijs)
References
Details
Attachments
(1 file)
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.
Updated•9 years ago
|
Flags: firefox-backlog+
Priority: -- → P1
Assignee | ||
Comment 1•9 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•9 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•9 years ago
|
||
Bug 1194692 - audit and fix usage of fileTimeToSecondsSinceEpoch, r?MattN
Attachment #8664515 -
Flags: review?(MattN+bmo)
Comment 4•9 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
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•9 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•9 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.
Comment 7•9 years ago
|
||
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]
Assignee | ||
Comment 8•9 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 9•9 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
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+
Updated•9 years ago
|
Flags: needinfo?(MattN+bmo)
Comment 10•9 years ago
|
||
Unfortunately, the builds from comment 8 are still affected.
Flags: needinfo?(cornel.ionce)
Assignee | ||
Comment 12•9 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•9 years ago
|
Attachment #8664515 -
Flags: checkin+
Comment 14•9 years ago
|
||
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•9 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•9 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•9 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•9 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 19•9 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
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•9 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)
Comment 21•9 years ago
|
||
(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)
Comment 23•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Target Milestone: --- → Firefox 44
Updated•9 years ago
|
Flags: qe-verify+
Comment 25•9 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•